arch/arm: Reserve r10 via ARCHCFLAGS, and hoist the duplicated PIC module flags.#19510
Open
casaroli wants to merge 1 commit into
Open
arch/arm: Reserve r10 via ARCHCFLAGS, and hoist the duplicated PIC module flags.#19510casaroli wants to merge 1 commit into
casaroli wants to merge 1 commit into
Conversation
Contributor
Author
acassis
requested changes
Jul 23, 2026
acassis
left a comment
Contributor
There was a problem hiding this comment.
@casaroli please document about it at https://nuttx.apache.org/docs/latest/components/nxflat.html
Contributor
|
@casaroli since you are using AI, please add after the Signed-off-by: Assisted-by: Claude Code (Opus 4.8) This is a new requirement with using AI to contribute on NuttX |
casaroli
force-pushed
the
arch-arm-pic-flags-hoist
branch
from
July 23, 2026 14:47
2f8dd7b to
02696d8
Compare
acassis
previously approved these changes
Jul 23, 2026
casaroli
force-pushed
the
arch-arm-pic-flags-hoist
branch
from
July 23, 2026 17:03
02696d8 to
e58841d
Compare
Contributor
Author
|
@xiaoxiang781216 please let me know if this fix works for you and I will close #19508 |
Contributor
let's continue in this pr. |
Toolchain.defs adds --fixed-r10 to CFLAGS under CONFIG_PIC, but nearly
every board Make.defs includes that file and then assigns
CFLAGS := $(ARCHCFLAGS) $(ARCHOPTIMIZATION) $(ARCHCPUFLAGS) ...
with ':=', which discards it. 266 of the 269 ARM board files assign
CFLAGS that way; the remaining three delegate to a shared makefile that
does the same thing.
The flag is what keeps the base firmware from allocating r10, the
register a PIC module reaches its own data through. Losing it is silent
and the symptom is remote from the cause: the build succeeds, and only a
callback from base firmware into module code -- qsort() with a module
comparison function is the standard case -- misbehaves, reading its data
through a register the firmware has since felt free to reuse.
Adding it to ARCHCFLAGS puts it on the far side of that ':=', which
re-expands ARCHCFLAGS, so every board picks it up with no board changes
at all.
A module is the other side of the --fixed-r10 contract: it gets r10 via
-mpic-register=r10, and GCC rejects both on one command line with
"unable to use 'r10' for PIC register". Every ARM board carried the
same three lines to set that up:
ARCHPICFLAGS = -fpic -msingle-pic-base -mpic-register=r10
CPICFLAGS = $(ARCHPICFLAGS) $(CFLAGS)
CXXPICFLAGS = $(ARCHPICFLAGS) $(CXXFLAGS)
They move to arch/arm/src/common/Toolchain.defs, which also filters
--fixed-r10 back out of CPICFLAGS, CXXPICFLAGS, CELFFLAGS and
CXXELFFLAGS in one place instead of each board having to know about the
interaction. ARCHPICFLAGS uses '?=' and the derived flags use deferred
'=', so a board can still override or append after including the file,
and CFLAGS is whatever the board finally set it to.
Two boards keep a definition because they genuinely differ: am67 adds
-ffixed-r10 and tiva conditionally adds -mno-pic-data-is-text-relative.
tlsr82 previously appended -fpic to an unset variable, so only -fpic
ever reached its compiler; it now inherits the standard set, verified
against tc32-elf-gcc 4.5.1.tc32-elf-1.5 (Telink TC32 v2.0), whose
ARM-derived backend honors -msingle-pic-base and -mpic-register=r10.
The mps2, mps3, qemu-armv7a, qemu-armv7r, fvp and mcx-nxxx families
referenced ARCHPICFLAGS without ever defining it, so their CPICFLAGS
carried no PIC flags at all; they get the standard set too.
Also adds the missing space in
CXXELFFLAGS = $(CXXFLAGS)-fvisibility=hidden -mlong-calls
which ran the last token of CXXFLAGS into -fvisibility=hidden, yielding
a single malformed token such as -DNDEBUG-fvisibility=hidden.
Also exempts the pre-existing "*.siz" gsize comment in Toolchain.defs
from codespell, which reads "siz" as a misspelling and fires for any
patch touching the file, since checkpatch scans whole files rather than
changed lines.
Before, with CONFIG_PIC=y: CFLAGS has --fixed-r10: NO
After: CFLAGS has --fixed-r10: YES
CPICFLAGS/CELFFLAGS: filtered out
Assisted-by: Claude Code:claude-opus-4-8
Assisted-by: Claude Code:claude-fable-5
Signed-off-by: Marco Casaroli <marco.casaroli@gmail.com>
casaroli
force-pushed
the
arch-arm-pic-flags-hoist
branch
from
July 24, 2026 11:20
e58841d to
72f088e
Compare
xiaoxiang781216
approved these changes
Jul 24, 2026
acassis
approved these changes
Jul 24, 2026
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.
Summary
One commit, in three parts (originally three commits; squashed on review request).
1. Stop boards silently discarding
--fixed-r10Toolchain.defsadds--fixed-r10toCFLAGSunderCONFIG_PIC, but nearlyevery board
Make.defsincludes that file and then assignswith
:=, which discards it. 266 of the 269 ARM board files assignCFLAGSthat way; the remaining three delegate to a shared makefile that doesthe same thing.
The flag is what stops the base firmware allocating r10 — the register a PIC
module reaches its own data through. Losing it is silent and the symptom is
remote from the cause: the build succeeds, and only a callback from firmware
into module code (
qsort()with a module comparison function is the standardcase) misbehaves, reading its data through a register the firmware has since
reused.
Moving it to
ARCHCFLAGSputs it on the far side of that:=, whichre-expands
ARCHCFLAGS. No board changes are needed.2. Hoist the duplicated PIC module flags into
Toolchain.defsEvery ARM board carried the same three lines — 258, 267 and 267 copies:
They move to
arch/arm/src/common/Toolchain.defs.ARCHPICFLAGSuses?=and the derived flags use deferred
=, so a board can still override orappend after including the file, and
CFLAGSis whatever the board finallyset it to.
This is also what makes part 1 safe in general. A module is the other side
of the
--fixed-r10contract: it gets r10 via-mpic-register=r10, and GCCrejects both on one command line with "unable to use 'r10' for PIC
register". Because
CPICFLAGS,CXXPICFLAGS,CELFFLAGSandCXXELFFLAGSall derive from
CFLAGS, the flag now gets filtered back out in one placeinstead of each board having to do it.
Two boards keep a definition because they genuinely differ:
am67adds-ffixed-r10andtivaconditionally adds-mno-pic-data-is-text-relative.tlsr82previously appended-fpicto an unset variable, so only-fpicever reached its compiler; its line is removed and it now inherits the
standard set, verified against the tc32 toolchain (see Testing).
3. Add the missing space in
CXXELFFLAGSruns the last token of
CXXFLAGSinto the first of the additions. Onstm32f4discovery:nshthat yields the single token-DNDEBUG-fvisibility=hidden, so a C++ ELF module gets neither a usableNDEBUGnor the-fvisibility=hiddenit was meant to be built with. Whichtoken is mangled depends on what
CXXFLAGSends with, so it varies by board.Impact
change in flags at all — verified below.
ARCHPICFLAGSwithout ever defining it — themps2/mps3,qemu-armv7a/armv7r,fvpandmcx-nxxxfamilies, sevenin total: their
CPICFLAGSpreviously carried no PIC flags whatsoever, soany PIC module they built could not have worked. They now get the standard
set.
tlsr82: previously only-fpicreached its compiler (it appended toan unset variable); it now inherits the standard set. Its tc32 toolchain
was verified to accept and honor the added flags — see Testing.
These two bullets are the only behavioural changes in the series.
CONFIG_PICconfigurations: the base firmware now genuinely reservesr10, and module flags no longer carry the contradictory
--fixed-r10.mps3-an547:picostestis the only defconfig in the tree withCONFIG_PIC=y.NDEBUGand-fvisibility=hiddeninstead of one malformed token.
and a board enabling
CONFIG_PICno longer needs to know about the--fixed-r10interaction.(one file,
nucleo-n657x0-q, additionally has its CRLF line endingsnormalized by checkpatch).
arm64has its own
Toolchain.defsand is untouched.Testing
Host:
Flag equivalence, before and after
The risk in a change this wide is silently altering a board's flags, so
ARCHPICFLAGS,CPICFLAGS,CXXPICFLAGSandCFLAGSwere dumped for aspread of boards on
masterand on this branch and compared token by token.Boards were chosen to cover the standard case, the ones that differ,
and the ones that never defined
ARCHPICFLAGS:stm32f4discovery:nshlm3s6965-ek:nsh(conditional append)tlsr8278adk80d:nsh(tc32 toolchain)+[-msingle-pic-base -mpic-register=r10]t3-gem-o1:nsh(-ffixed-r10)nucleo-l552ze:nshmps3-an547:picostest+[-fpic -msingle-pic-base -mpic-register=r10],CPICFLAGS -[--fixed-r10]mps2-an521:nsh+[-fpic -msingle-pic-base -mpic-register=r10]qemu-armv7a:nsh+[-fpic -msingle-pic-base -mpic-register=r10]The non-identical boards are exactly the ones that referenced
ARCHPICFLAGSwithout defining it, plustlsr82, as described underImpact.
The flag actually reaching the compiler
On master:
On this branch:
A PIC module compiles
Compiling a file with the exact
CPICFLAGSthe build computes, onmps3-an547:picostest:That error is what makes parts 1 and 2 a pair. Applying the
ARCHCFLAGSmove on its own is fine for
mps3-an547as it stands today (it has no-mpic-register=r10to clash with), but becomes a build failure the momentthat board gains the standard
ARCHPICFLAGS— which part 2 gives it.tlsr82 and the tc32 toolchain
tlsr82gains-msingle-pic-base -mpic-register=r10, so those flags weretested against its compiler,
tc32-elf-gcc 4.5.1.tc32-elf-1.5(Telink TC32v2.0 build). Its ARM-derived backend accepts and honors them: a global
access compiles to
tadd r3, sl(r10) instead of the pc-relative GOTsequence, and a bogus register is rejected with "unable to use 'r99' for
PIC register", confirming the option is parsed rather than ignored. The
board's
Make.defswas also evaluated on this branch to confirmARCHPICFLAGSandCPICFLAGSnow carry the standard set.CXXELFFLAGS
stm32f4discovery:nsh, before:after:
Builds
lm3s6965-ek:nshwas not built: its configuration selects thearm-nuttx-elf-buildroot toolchain, which is not installed on this host(
arm-nuttx-elf-gcc failed: 127). Its flags are covered by the equivalencecheck above.
Tree checks
The patch also adds one line to
.codespell-ignore-lines: the pre-existing"*.siz"gsize comment inToolchain.defs, which codespell reads as amisspelling of "size". It fires for any patch touching that file, because
checkpatch scans whole files rather than changed lines.