Skip to content

cpu/riscv_common: fix undeclared memory region linker error#17581

Merged
benpicco merged 1 commit intoRIOT-OS:masterfrom
nmeum:riscv-linker-rom-region
Jan 27, 2022
Merged

cpu/riscv_common: fix undeclared memory region linker error#17581
benpicco merged 1 commit intoRIOT-OS:masterfrom
nmeum:riscv-linker-rom-region

Conversation

@nmeum
Copy link
Copy Markdown
Member

@nmeum nmeum commented Jan 27, 2022 •
edited
Loading

Since commit 3a11b1f (#16972) building RIOT applications with BOARD=hifive1 causes the following linker warning to be emitted on my system:

/opt/rv32imc/lib/gcc/riscv32-unknown-elf/10.2.0/../../../../riscv32-unknown-elf/bin/ld:riscv_base.ld:220: warning: memory region `rom' not declared

This is due to the fact that the RISC-V linker script doesn't have a rom memory region. While many other ARM-based boards have a rom memory region defined in the linker script, the corresponding region name in the RISC-V linker script is flash and rom is not declared as a memory region hence the warning.

I think this was accidentally overlooked in 3a11b1f. It is fixed in this commit by replacing the rom region with the flash region. The linker script identifiers (e.g. _srom and _erom) are not renamed.

@Ollrogge can you confirm that the changes proposed here are aligned with 3a11b1f?

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Jan 27, 2022
@nmeum nmeum requested a review from Ollrogge January 27, 2022 12:14
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jan 27, 2022 •
edited
Loading

@nmeum how do you get this error? I would expect this to have been caught by the CI, do you have a flashpage implementation for riscv?

Never mind I though it was an error, but it's a warning, I also get it.

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Jan 27, 2022

I've seen the warning here too.

A different solution would be to use a region alias and keep the names in our linker scripts in sync with the rest of RIOT. I'll leave it up to you to decide what you prefer.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 27, 2022
@benpicco benpicco requested a review from bergzand January 27, 2022 13:13
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Jan 27, 2022 •
edited
Loading

Or just s/flash/rom/g if we want consistent names.

@nmeum
cpu/riscv_common: fix undeclared memory region linker error
e41063d 
Since commit 3a11b1f (RIOT-OS#16972)
building RIOT applications with `BOARD=hifive1` causes the following
linker error to be emitted on my system:

	/opt/rv32imc/lib/gcc/riscv32-unknown-elf/10.2.0/../../../../riscv32-unknown-elf/bin/ld:riscv_base.ld:220: warning: memory region `rom' not declared

This is due to the fact that the RISC-V linker script doesn't have a rom
memory region. While many other ARM-based boards have a rom memory
region defined in the linker script, the corresponding region name in
the RISC-V linker script is flash and rom is not declared as a memory
region hence the warning.

I think this was accidentally overlooked in
3a11b1f. It is fixed in this commit by
replacing the rom region with the flash region. The linker script
identifiers (e.g. _srom and _erom) are not renamed.
@Ollrogge
Copy link
Copy Markdown
Member

Ollrogge commented Jan 27, 2022

Since commit 3a11b1f (#16972) building RIOT applications with BOARD=hifive1 causes the following linker warning to be emitted on my system:

/opt/rv32imc/lib/gcc/riscv32-unknown-elf/10.2.0/../../../../riscv32-unknown-elf/bin/ld:riscv_base.ld:220: warning: memory region `rom' not declared

This is due to the fact that the RISC-V linker script doesn't have a rom memory region. While many other ARM-based boards have a rom memory region defined in the linker script, the corresponding region name in the RISC-V linker script is flash and rom is not declared as a memory region hence the warning.

I think this was accidentally overlooked in 3a11b1f. It is fixed in this commit by replacing the rom region with the flash region. The linker script identifiers (e.g. _srom and _erom) are not renamed.

@Ollrogge can you confirm that the changes proposed here are aligned with 3a11b1f?

Looks good. Thanks for fixing !

@benpicco benpicco enabled auto-merge January 27, 2022 14:33
@benpicco benpicco merged commit 4ed9d8f into RIOT-OS:master Jan 27, 2022
nmeum referenced this pull request Apr 1, 2022
@Ollrogge
cpu/riscv_common: add flash_writable section to linker script
74d086c
This was referenced Apr 1, 2022
Merged
Merged
@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@Ollrogge Ollrogge Awaiting requested review from Ollrogge

@bergzand bergzand Awaiting requested review from bergzand

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports 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

Projects

None yet

Milestone

Release 2022.04

Development

Successfully merging this pull request may close these issues.

6 participants

@nmeum @fjmolinas @bergzand @benpicco @Ollrogge @OlegHahm