casaroli commented on code in PR #19584:
URL: https://github.com/apache/nuttx/pull/19584#discussion_r3700211227
##########
arch/arm/src/common/Toolchain.defs:
##########
@@ -612,11 +612,42 @@ CELFFLAGS = $(filter-out --fixed-r10,$(CFLAGS))
-fvisibility=hidden \
CXXELFFLAGS = $(filter-out --fixed-r10,$(CXXFLAGS)) -fvisibility=hidden \
-mlong-calls
+ifeq ($(CONFIG_FDPIC),y)
+ # An FDPIC module reaches its own data through r9, and that register has
+ # to survive a call into the base firmware. A base firmware routine that
+ # calls back into module code -- qsort() with a module comparison
+ # function is the standard case -- must arrive there with the module's
+ # GOT still in r9, which it will not if the compiler was free to allocate
+ # r9 here.
+ #
+ # This goes in ARCHCPUFLAGS rather than CFLAGS because almost every board
+ # Make.defs assigns
+ #
+ # CFLAGS := $(ARCHCFLAGS) $(ARCHOPTIMIZATION) $(ARCHCPUFLAGS) ...
+ #
+ # with ':=' after including this file, which discards anything added to
+ # CFLAGS here but re-expands ARCHCPUFLAGS. The failure is silent and the
+ # symptom is remote from the cause: the build succeeds and only a callback
+ # from firmware into module code misbehaves, reading its data through a
+ # register the firmware has since reused.
+ #
+ # CPICFLAGS derives from CFLAGS, so this reaches module compiles too.
+ # That is harmless: GCC only rejects --fixed-rN when it names the same
+ # register as -mpic-register, and PIC modules use r10.
+
+ ARCHCPUFLAGS += --fixed-r9
+endif
+
ifeq ($(CONFIG_PIC),y)
+ifneq ($(CONFIG_FDPIC),y)
# ARCHCFLAGS, not CFLAGS: board Make.defs reassign CFLAGS with ':='
# after including this file, which would discard the flag.
+ #
+ # FDPIC reserves r9 above instead; reserving both would cost a register
+ # for nothing.
ARCHCFLAGS += --fixed-r10
Review Comment:
Agreed, doing it — as its own PR ahead of the loader work, because it
touches every NXFLAT and PIC board and shouldn't be bisect-tangled with a new
binfmt.
The tree already uses both registers, which is the strongest argument for
unifying: Toolchain.defs:594 gives CONFIG_BUILD_PIC -mpic-register=r9 while
:620 gives CONFIG_PIC r10, twenty-five lines apart, and arm_initialstate.c sets
REG_R9 from inline assembly under one and REG_PIC under the other, with the
comment reading "Set the PIC base register (probably R10)".
r9 is also the right register rather than an arbitrary one: it is the AAPCS
platform register, the static base, and it is what GCC picks for
-msingle-pic-base on an EABI target. r10 is the non-EABI default.
And it fixes a combination that cannot build today. Stack checking adds
-ffixed-r10 in armv7-m/Toolchain.defs:149 and armv8-m/Toolchain.defs:168 while
CONFIG_PIC adds -mpic-register=r10, and GCC rejects the pair with "unable to
use 'r10' for PIC register". The comment above REG_PIC has always said the
register "can be R9 if stack checking is enabled", but the definition was
unconditionally REG_R10, so it would have named the wrong register even had the
build succeeded.
One thing worth flagging before that PR appears, because it is not obvious.
Switching the register on its own breaks NXFLAT, and silently. The import
thunks are generated by mknxflat, which had the register baked into its
template as add ip,ip,sl, so a module compiled -mpic-register=r9 reaches its
data through r9 while its import stubs still add r10, and it branches to a wild
address on its first call into the base firmware. I confirmed this on
lm3s6965-ek:qemu-nxflat under QEMU: master passes the nxflat example, the
register change alone takes it to a HardFault lockup, and patching the thunk
template to r9 restores it exactly.
mknxflat lives outside this repository, in the buildroot NXFLAT toolchain,
so the kernel and the tool could drift with no way to detect it — nxflat.h has
only h_magic and no version field. So that PR brings mknxflat in-tree first, as
tools/nxflat/, with libbfd replaced by reading the ELF symbol table directly.
libbfd was never used for anything but opening the file and enumerating
symbols, and it is GPL, which we cannot depend on anyway. The thunks are then
regenerated from a single NXFLAT_PIC_REG on every build and the two cannot
drift.
That import is a no-op on its own: against the upstream tool, for both ARM
and Thumb-2, with and without -w, over modules exercising the plain, weak and
non-returning thunk paths, the generated thunk fi
ldnxflat does not need to move for this — its only sl refemments — though
bringing it in-tree afterwards is worth doing, since it would let us version
the format so a stale module fails to load cleanly instead of crashing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
