Fix spurious per-region size deltas when a target's regions are renamed - #11948
Open
sensei-hacker wants to merge 1 commit into
Open
sensei-hacker wants to merge 1 commit into
sensei-hacker wants to merge 1 commit into
Conversation
diffSizeReports() computed a per-region delta table whenever both the PR and baseline had a non-empty `regions` breakdown, defaulting a region missing from either side to 0 bytes. If a target's region *names* changed between the two commits (e.g. a linker script split one region into two, or renamed one, with the same total bytes used), this reported the old name's entire usage as a large fictitious loss and the new name(s) as an equally large fictitious gain - misleading reviewers with a "notable" delta for a change that never happened. Only build regionDeltas when both sides name the exact same set of regions; otherwise fall back to the existing combined RAM delta path, with a short note that the region layout itself changed so reviewers aren't left wondering why the breakdown disappeared. Qodo flagged this on PR iNavFlight#11945 (a routine release/9.1->master sync that surfaced it); reproduced and fixed directly on release/9.1, where the size-report feature actually lives.
Contributor
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record |
PR Summary by QodoPrevent false size deltas when memory regions are renamed
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Merged
|
RAM / Flash usage vs. base commit
See RAM/flash optimization guide for techniques to reduce usage. |
|
Test firmware build ready — commit Download firmware for PR #11948 245 targets built. Find your board's
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a real bug flagged by Qodo on #11945 (a routine
release/9.1->mastersync PR that carried the size-report region-breakdown feature, and surfaced two findings in the process — see below for why only one is addressed here).diffSizeReports()in.github/scripts/size-diff-comment.js(the CI job that posts a "RAM/Flash usage delta" comment on PRs) computed a per-region delta table whenever both the PR and baseline size reports had a non-emptyregionsbreakdown, taking the union of region names and defaulting a region missing from either side to 0 bytes. If a target's set of region names changes between the two commits being compared — e.g. a linker script splits one region into two, or renames one, with the exact same total bytes used and zero real growth — this reports the old name's entire usage as a large fictitious "loss" and the new name(s) as an equally large fictitious "gain," and can trigger theReproduced concretely: PR regions
{SRAM1: 60000, SRAM2: 70000}vs. baseline regions{RAM: 130000}(same 130000 total, zero growth) producedRAM: -130000,SRAM1: +60000,SRAM2: +70000, flagged notable.Changes
diffSizeReports()now only builds a per-region delta table when both sides name the exact same set of regions (this already matched the function's own documented intent — the code just didn't implement it). Otherwise it falls back to the existing (already-tested) combined RAM delta path.regionsentirely), the RAM cell now shows a short "region layout changed since baseline" note, so a genuine linker-layout change isn't silently invisible to reviewers even though no per-region numbers are shown.diffSizeReports-level unit test for the newregionSetChangedflag.On the other Qodo finding from #11945
Qodo also flagged
.github/scripts/extract-size-report.sh's map-path construction (map="${elf}.map"), claiming CMake >= 3.15'sTARGET_FILE_BASE_NAMEgenerator expression strips the.elfsuffix, so the real map file would be<target>.maprather than<target>.elf.map. I checked this against this project's actualcmake/stm32.cmake/cmake/at32.cmake: both define the executable target itself asset(elf_target ${args_NAME}.elf)— i.e..elfis part of the CMake target name string, not a build-system-imposed suffix — soTARGET_FILE_BASE_NAMEreturns the name with.elfstill attached regardless of CMake version, and both branches of the version-conditional ingenerate_map_file()produce the same<target>.elf.mapresult in practice. Verified directly against real build artifacts (build/bin/AOCODARCF722AIO.elf.map,build/bin/AOCODARCF4V2.elf.mapboth exist and match${elf}.mapexactly). Not a real issue for this codebase — no change made for it.Testing
node --test .github/scripts/size-diff-comment.test.js— 29/29 pass (27 pre-existing + 2 new... actually see below)diffSizeReports()/renderComment()calls with the exact SRAM1/SRAM2 scenario aboveCode Review
Reviewed with the
inav-code-reviewagent — approved, with two minor test-coverage suggestions (adding a missing assertion, and a combined-growth-plus-rename test case) which were incorporated, and one product-decision suggestion (surfacing that the region layout changed even without a breakdown) which was also incorporated as the "region layout changed since baseline" note.