Skip to content

arch/arm: Reserve r10 via ARCHCFLAGS, and hoist the duplicated PIC module flags.#19510

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
casaroli:arch-arm-pic-flags-hoist
Jul 24, 2026
Merged

arch/arm: Reserve r10 via ARCHCFLAGS, and hoist the duplicated PIC module flags.#19510
xiaoxiang781216 merged 1 commit into
apache:masterfrom
casaroli:arch-arm-pic-flags-hoist

Conversation

@casaroli

@casaroli casaroli commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

One commit, in three parts (originally three commits; squashed on review request).

1. Stop boards silently discarding --fixed-r10

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 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 standard
case) misbehaves, reading its data through a register the firmware has since
reused.

Moving it to ARCHCFLAGS puts it on the far side of that :=, which
re-expands ARCHCFLAGS. No board changes are needed.

2. Hoist the duplicated PIC module flags into Toolchain.defs

Every ARM board carried the same three lines — 258, 267 and 267 copies:

ARCHPICFLAGS = -fpic -msingle-pic-base -mpic-register=r10
CPICFLAGS = $(ARCHPICFLAGS) $(CFLAGS)
CXXPICFLAGS = $(ARCHPICFLAGS) $(CXXFLAGS)

They move to arch/arm/src/common/Toolchain.defs. 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.

This is also what makes part 1 safe in general. 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"
. Because CPICFLAGS, CXXPICFLAGS, CELFFLAGS and CXXELFFLAGS
all derive from CFLAGS, the flag now gets filtered back out in one place
instead of each board having to do it.

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; its line is removed and it now inherits the
standard set, verified against the tc32 toolchain (see Testing).

3. Add the missing space in CXXELFFLAGS

CXXELFFLAGS = $(CXXFLAGS)-fvisibility=hidden -mlong-calls

runs the last token of CXXFLAGS into the first of the additions. On
stm32f4discovery:nsh that yields the single token
-DNDEBUG-fvisibility=hidden, so a C++ ELF module gets neither a usable
NDEBUG nor the -fvisibility=hidden it was meant to be built with. Which
token is mangled depends on what CXXFLAGS ends with, so it varies by board.

Impact

  • Boards that defined the standard three lines (the large majority): no
    change in flags at all — verified below.
  • Boards that referenced ARCHPICFLAGS without ever defining it — the
    mps2/mps3, qemu-armv7a/armv7r, fvp and mcx-nxxx families, seven
    in total: their CPICFLAGS previously carried no PIC flags whatsoever, so
    any PIC module they built could not have worked. They now get the standard
    set.
  • tlsr82: previously only -fpic reached its compiler (it appended to
    an 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_PIC configurations: the base firmware now genuinely reserves
    r10, and module flags no longer carry the contradictory --fixed-r10.
    mps3-an547:picostest is the only defconfig in the tree with
    CONFIG_PIC=y.
  • C++ ELF modules: gain a correct NDEBUG and -fvisibility=hidden
    instead of one malformed token.
  • Board maintainers: a new board no longer needs to copy the three lines,
    and a board enabling CONFIG_PIC no longer needs to know about the
    --fixed-r10 interaction.
  • Diff size: 270 files, +118/−1085. All board changes are pure deletions
    (one file, nucleo-n657x0-q, additionally has its CRLF line endings
    normalized by checkpatch).
  • Documentation, security and non-ARM architectures are unaffected. arm64
    has its own Toolchain.defs and is untouched.

Testing

Host:

macOS 26.5.1 (Darwin 25.5.0, arm64)
Arm GNU Toolchain 15.2.Rel1 (arm-none-eabi-gcc 15.2.1)
GNU make

Flag equivalence, before and after

The risk in a change this wide is silently altering a board's flags, so
ARCHPICFLAGS, CPICFLAGS, CXXPICFLAGS and CFLAGS were dumped for a
spread of boards on master and 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:

Board Result
stm32f4discovery:nsh identical
lm3s6965-ek:nsh (conditional append) identical
tlsr8278adk80d:nsh (tc32 toolchain) +[-msingle-pic-base -mpic-register=r10]
t3-gem-o1:nsh (-ffixed-r10) identical
nucleo-l552ze:nsh identical
mps3-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
ARCHPICFLAGS without defining it, plus tlsr82, as described under
Impact.

The flag actually reaching the compiler

$ ./tools/configure.sh mps3-an547:picostest
$ make -f /tmp/show.mk TOPDIR=$PWD show      # prints $(CFLAGS) etc.

On master:

CONFIG_PIC = y
CFLAGS    has --fixed-r10 : NO

On this branch:

CONFIG_PIC = y
CFLAGS    has --fixed-r10 : YES
CPICFLAGS has --fixed-r10 : NO
CELFFLAGS has --fixed-r10 : NO

A PIC module compiles

Compiling a file with the exact CPICFLAGS the build computes, on
mps3-an547:picostest:

$ arm-none-eabi-gcc $CPICFLAGS -c t.c -o t.o
cc1: error: unable to use 'r10' for PIC register     # part 1 alone
                                                     # part 2: compiles

That error is what makes parts 1 and 2 a pair. Applying the ARCHCFLAGS
move on its own is fine for mps3-an547 as it stands today (it has no
-mpic-register=r10 to clash with), but becomes a build failure the moment
that board gains the standard ARCHPICFLAGS — which part 2 gives it.

tlsr82 and the tc32 toolchain

tlsr82 gains -msingle-pic-base -mpic-register=r10, so those flags were
tested against its compiler, tc32-elf-gcc 4.5.1.tc32-elf-1.5 (Telink TC32
v2.0 build). Its ARM-derived backend accepts and honors them: a global
access compiles to tadd r3, sl (r10) instead of the pc-relative GOT
sequence, 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.defs was also evaluated on this branch to confirm
ARCHPICFLAGS and CPICFLAGS now carry the standard set.

CXXELFFLAGS

stm32f4discovery:nsh, before:

-DNDEBUG-fvisibility=hidden

after:

-DNDEBUG
-fvisibility=hidden

Builds

stm32f4discovery:nsh                       make exit=0
mps3-an547:picostest (CONFIG_PIC=y)        make exit=0

lm3s6965-ek:nsh was not built: its configuration selects the
arm-nuttx-elf- buildroot toolchain, which is not installed on this host
(arm-nuttx-elf-gcc failed: 127). Its flags are covered by the equivalence
check above.

Tree checks

$ ./tools/checkpatch.sh -c -u -m -g master..HEAD
Used config files:
    1: .codespellrc
✔️ All checks pass.

The patch also adds one line to .codespell-ignore-lines: the pre-existing
"*.siz" gsize comment in Toolchain.defs, which codespell reads as a
misspelling of "size". It fires for any patch touching that file, because
checkpatch scans whole files rather than changed lines.

@casaroli
casaroli requested a review from jerpelea as a code owner July 23, 2026 09:44
@casaroli

Copy link
Copy Markdown
Contributor Author

This PR is an alternative to #19508. If we merge this, #19508 can be closed.

@github-actions github-actions Bot added Arch: arm Issues related to ARM (32-bit) architecture Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. Board: arm labels Jul 23, 2026
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

MemBrowse Memory Report

No memory changes detected for:

@acassis acassis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acassis

acassis commented Jul 23, 2026

Copy link
Copy Markdown
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
casaroli force-pushed the arch-arm-pic-flags-hoist branch from 2f8dd7b to 02696d8 Compare July 23, 2026 14:47
@casaroli
casaroli requested a review from hartmannathan as a code owner July 23, 2026 14:47
acassis
acassis previously approved these changes Jul 23, 2026
@github-actions github-actions Bot added the Area: Documentation Improvements or additions to documentation label Jul 23, 2026
@casaroli
casaroli force-pushed the arch-arm-pic-flags-hoist branch from 02696d8 to e58841d Compare July 23, 2026 17:03
@casaroli casaroli changed the title arch/arm: reserve r10 via ARCHCFLAGS, and hoist the duplicated PIC module flags arch/arm: Reserve r10 via ARCHCFLAGS, and hoist the duplicated PIC module flags. Jul 23, 2026
@casaroli

Copy link
Copy Markdown
Contributor Author

@xiaoxiang781216 please let me know if this fix works for you and I will close #19508

Comment thread boards/arm/tlsr82/tlsr8278adk80d/scripts/Make.defs Outdated
Comment thread arch/arm/src/common/Toolchain.defs Outdated
Comment thread Documentation/components/nxflat.rst
@xiaoxiang781216

xiaoxiang781216 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

@xiaoxiang781216 please let me know if this fix works for you and I will close #19508

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
casaroli requested a review from acassis July 24, 2026 12:10
@xiaoxiang781216
xiaoxiang781216 merged commit db011db into apache:master Jul 24, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Area: Documentation Improvements or additions to documentation Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants