Skip to content

arch/riscv: fix incorrect ASFLAGS#21672

Merged
crasbe merged 1 commit intoRIOT-OS:masterfrom
AnnsAnns:riscv_asm
Aug 21, 2025
Merged

arch/riscv: fix incorrect ASFLAGS#21672
crasbe merged 1 commit intoRIOT-OS:masterfrom
AnnsAnns:riscv_asm

Conversation

@AnnsAnns
Copy link
Copy Markdown
Member

@AnnsAnns AnnsAnns commented Aug 20, 2025 •
edited
Loading

Contribution description

Coming in with a staggering 1 singular line changed this PR is surprisingly important (mostly to me 😛).

The riscv_common and by that the arch makefile for riscv in general sets the proper flags to build a riscv application, as used in the pico 2 support (soon ™️).

There is however a potential oversight, as the ASFLAGS gets incorrectly set to contain the C flags. In general this should be fine except that the GNU assembler does not have all the options later set. For example, while quite important in the context of IoT devices, -mcmodel=medlow appears to be something that is only relevant with parsed code, as opposed to the pure assembly that you'd write when passing it to the assembler, given that all these options don't affect pure assembly, thus causing the assembler to crash/fail.

Same goes for every other flag set starting at https://github.com/AnnsAnns/RIOT/blob/6c36aceada9ee8dcf0e6d83bfeff52c5d8033e98/makefiles/arch/riscv.inc.mk#L70

"make" -C /home/ann/projects/nondocsriot/cpu/rp2350_riscv
ASFLAGS are: -march=rv32imac -mabi=ilp32 -misa-spec=2.2 -mcmodel=medlow -msmall-data-limit=8 -malign-data=natural -g3
riscv32-unknown-elf-as: unrecognized option '-mcmodel=medlow'
make[2]: *** [/home/ann/projects/nondocsriot/Makefile.base:188: /home/ann/projects/nondocsriot/examples/basic/blinky/bin/rpi-pico-2-riscv/cpu/picobin_block.o] Error 1
make[1]: *** [/home/ann/projects/nondocsriot/Makefile.base:31: ALL--/home/ann/projects/nondocsriot/cpu/rp2350_riscv] Error 2
make: *** [/home/ann/projects/nondocsriot/examples/basic/blinky/../../../Makefile.include:763: application_blinky.module] Error 2

The potential reason this wasn't caught, even though riscv_common is really well written and also has a fairly important assembly file, is that it uses start.S not start.s as the filename.

.S (uppercase) files are not actually handled by riscv32-unknown-elf-as and thus never actually use ASFLAGS but are simply passed to gcc (which also allows you to use macros, directives, etc) while .s (lowercase) files are pure assembly code handled by riscv32-unknown-elf-as. (See Makefile.base ~L178)

A lot of yapping cut short, as of now .s (lowercase) pure assembly code can not be compiled/linked, which did cause me a lot of confusion as I was trying to figure out why exactly picobin.s which I use for the RISC-V support of the pico2 refused to compile while start.S compiles just fine.

Note that I might also just be completely wrong, but this was my best guess in fixing this. @nmeum appears to be the original author based on git blame so I'm quite open to hear whether or not my theory is anywhere close to reality 😅

Testing procedure

Try to include a .s assembly file in a RISC-V project.

Issues/PRs references

@github-actions github-actions bot added Area: build system Area: Build system Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Aug 20, 2025
@crasbe crasbe added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 20, 2025
@crasbe crasbe requested a review from nmeum August 20, 2025 11:56
crasbe
crasbe reviewed Aug 20, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@crasbe crasbe left a comment

Choose a reason for hiding this comment

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

What about the -misa-spec=2.2 flag in line 64? Would that also be relevant for the assembler?

I would also add a comment to the flags telling why it is set at this point exactly, otherwise one might assume that there are ASFLAGS missing.

makefiles/arch/riscv.inc.mk Outdated
GCC_DEFAULTS_TO_NEW_RISCV_ISA ?= 0

CFLAGS_CPU := -march=rv32imac -mabi=ilp32
ASFLAGS := $(CFLAGS_CPU)
Copy link
Copy Markdown
Contributor

@crasbe crasbe Aug 20, 2025

Choose a reason for hiding this comment

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

Suggested change
ASFLAGS := $(CFLAGS_CPU)
# set ASFLAGS before incompatible flags are added to CFLAGS_CPU
ASFLAGS := $(CFLAGS_CPU)

@riot-ci
Copy link
Copy Markdown

riot-ci commented Aug 20, 2025 •
edited
Loading

Murdock results

✔️ PASSED

abc02fa arch/riscv: fix incorrect ASFLAGS

Success Failures Total Runtime
10536 0 10538 19m:58s

Artifacts

maribu
maribu reviewed Aug 20, 2025
View reviewed changes
makefiles/arch/riscv.inc.mk Outdated
@@ -87,7 +88,6 @@ LINKER_SCRIPT ?= $(CPU_MODEL).ld
LINKFLAGS += -T$(LINKER_SCRIPT)

CFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT) $(CFLAGS_LINK)
Copy link
Copy Markdown
Member

@maribu maribu Aug 20, 2025

Choose a reason for hiding this comment

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

I would introduce a new CFLAGS_NO_ASM variable and add -mcmodel=medlow to that on when using GCC. E.g. like this:

Suggested change
CFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT) $(CFLAGS_LINK)
CFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT) $(CFLAGS_LINK)
ASFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG)
# add CFLAGS that should not be added to the assembler
CFLAGS += CFLAGS_NO_ASM

As @crasbe pointed out, not specifying the ISA level is bound to cause regressions with older toolchains.

@AnnsAnns
Copy link
Copy Markdown
Member Author

AnnsAnns commented Aug 20, 2025

Integrated both reviews, ty :)

@AnnsAnns
Copy link
Copy Markdown
Member Author

AnnsAnns commented Aug 20, 2025

squashy squash squashed :)

@maribu maribu added this pull request to the merge queue Aug 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 20, 2025
@AnnsAnns
Copy link
Copy Markdown
Member Author

AnnsAnns commented Aug 20, 2025

That is not related to me whatsoever, not even on RISC-V 🤨

I guess you could say ... I had bad timing 🥁

@crasbe crasbe added this pull request to the merge queue Aug 20, 2025
@crasbe
Copy link
Copy Markdown
Contributor

crasbe commented Aug 20, 2025

The xtimer tests are somewhat prone to failure on the CI system, depending on the system load.

Merged via the queue into RIOT-OS:master with commit a76186e Aug 21, 2025
25 checks passed
@AnnsAnns
Copy link
Copy Markdown
Member Author

AnnsAnns commented Aug 21, 2025

ty

@benpicco benpicco added this to the Release 2025.10 milestone Dec 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crasbe crasbe crasbe left review comments

@maribu maribu maribu approved these changes

@nmeum nmeum Awaiting requested review from nmeum

Assignees

No one assigned

Labels

Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2025.10

Development

Successfully merging this pull request may close these issues.

5 participants

@AnnsAnns @riot-ci @crasbe @maribu @benpicco