Fix : gh #130 : Use -O0 not -o0 to set optimisation level for TARGET=linux - #131
Open
Ulrond wants to merge 2 commits into
Open
Fix : gh #130 : Use -O0 not -o0 to set optimisation level for TARGET=linux#131Ulrond wants to merge 2 commits into
Ulrond wants to merge 2 commits into
Conversation
-o0 was replaced with -O0 to correctly set optimisation level. This issue only happens with build target LINUX Fixes #130
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Pull request overview
Updates the linux-target build defaults in ut-control’s Makefile to use the correct GCC optimization flag (-O0 instead of -o0) when setting the default CC command.
Changes:
- Fixes the linux default compiler flags to use
-O0(optimization level 0) rather than-o0(output file option).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
116
to
118
| ifeq ($(TARGET),linux) | ||
| CC ?= gcc -ggdb -o0 -Wall | ||
| CC ?= gcc -ggdb -O0 -Wall | ||
| endif |
Adds check_makefile_flags to the tests suite, run before build.sh so a flag typo is reported in seconds rather than after a full build. It walks the build files for a lowercase -o glued to an optimisation level (-o0, -os, -og). gcc accepts these silently and a later -o $@ on the same command line wins, so nothing ever fails - which is why #130 went unnoticed. Comments are stripped before matching, so prose describing the typo does not trip the check. The script also supports asserting what a variable expands to (--expect CC=-ggdb,-O0,-Wall) via make printenv. That is deliberately not wired up yet: 'CC ?=' in the top level Makefile never fires, since GNU make treats ?= as ifeq (origin CC,undefined) and the built-in CC has origin default. The Makefile records this so the expectation can be turned on once that is resolved. Verified by reintroducing -o0: the check fails, exit 1. Refs #130
Comment on lines
+88
to
+90
| check_makefile_flags: | ||
| @$(ECHOE) UT [$@] | ||
| @$(FLAGS_SCRIPT) $(ROOT_DIR)/.. |
Comment on lines
+74
to
+79
| try: | ||
| with open(path, "r", encoding="utf-8", errors="replace") as handle: | ||
| lines = handle.readlines() | ||
| except OSError as error: | ||
| print(f"{RED}Error: cannot read '{path}': {error}{RESET}") | ||
| return None |
|
I have read the CLA Document and I hereby sign the CLA |
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
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.
Fixes #130
Applies the patch supplied by @looopTools in the issue, with authorship preserved via
git am.-o0is not an optimisation flag — lowercase-ois "write output to this file", so-o0asks for a file named0. TheTARGET=linuxdefault is now-O0.CC ?= gcc -ggdb -O0 -WallTARGET=armis untouched — it takes$(CC)from the SDK and never carried the flag.Note for reviewers: this line is currently a no-op
Worth flagging while the line is under the microscope —
Makefile:117uses?=, which GNU make defines asifeq ($(origin CC),undefined). Make's built-inCC = cchas origindefault, neverundefined, so the assignment never fires:So a standalone
make TARGET=linuxin ut-control compiles with barecc— no-ggdb, no-Wall, no-O0— and when driven from ut-core,CCarrives on the command line and overrides the line regardless. This PR is therefore a correctness fix with no behavioural change on its own.The real
-o0damage came from ut-core passingCC="gcc -ggdb -o0 -Wall"down to this sub-make; that side is fixed in rdkcentral/ut-core#258.If maintainers want the intended defaults to actually apply, the idiom that works while still honouring environment and command-line overrides is:
Deliberately left out of this PR — happy to raise it separately if wanted.
Verification
grep -rn '\-o0' --include=Makefile --include=*.mk --include=*.sh .returns no hits.make TARGET=linuxon this branch builds clean, and no file named0appears in the tree afterwards.Second commit: a regression guard in the tests suite
The typo is two characters, but it survived because no functional test could ever have caught it — gcc accepted
-o0, a later-o $@on the same command line won, and-O0is gcc's default. Every build stayed green. Only a static check on the build files can see it.tests/check_makefile_flags.pyis wired into the suite ascheck_makefile_flagsand runs beforebuild.sh, so a flag typo costs seconds rather than a full build. It walks everyMakefile/*.mk/*.shfor a lowercase-oglued to an optimisation level (-o0,-os,-og,-ofast), skipping third-party trees. Comments are stripped before matching, so prose describing the typo does not trip the check.Verified by reintroducing
-o0on top of the fix:On the fixed tree: 9 build files scanned, 0 findings.
The value assertion is deliberately left off
The script also supports asserting what a variable actually expands to via
make printenv—--expect CC=-ggdb,-O0,-Wall— which is the stronger check, since it catches flags being dropped as well as mistyped. It is not wired up here, because it would fail immediately:CC ?=never fires (see above), soCCexpands to barecc. Turning it on therefore means deciding the?=question first. The reason is recorded in a comment intests/Makefilenext to the target so the next person does not have to rediscover it.The same script is added to ut-core in rdkcentral/ut-core#258, where the value assertion is enabled (
COMPILERandUT_CONTROL_COMPILERmust contain-ggdb -O0 -Wall).