Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix linker file for RISCV-64 targets #236

Merged
merged 2 commits into from
Nov 8, 2024
Merged

Conversation

romancardenas
Copy link
Contributor

@romancardenas romancardenas commented Oct 29, 2024

Looks like the current linker file contains some things that were probably required when using binary blobs in riscv-rt. Now that we use inline assembly, they are not needed. Furthermore, some of them appear to provoke errors.

This PR (still WIP) aims to fix these issues. For instance, just commenting out the eh_frame part of the linker script fixes the compilation of the example provided in #196

I still need to play around with this, but this is a good first step

Closes #196

@romancardenas
Copy link
Contributor Author

I am currently going through cortex-m-rt linker file and checking the differences. I will also assess the size of binaries etc. to evaluate the side effects of any change.

Comment on lines 173 to 174
/* .eh_frame (INFO) : { KEEP(*(.eh_frame)) } */
/* .eh_frame_hdr (INFO) : { *(.eh_frame_hdr) } */
Copy link
Contributor

@rmsyn rmsyn Oct 30, 2024

Choose a reason for hiding this comment

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

Suggested change
/* .eh_frame (INFO) : { KEEP(*(.eh_frame)) } */
/* .eh_frame_hdr (INFO) : { *(.eh_frame_hdr) } */
.eh_frame : { KEEP(*(.eh_frame)) } > REGION_TEXT
.eh_frame_hdr (INFO) : { *(.eh_frame_hdr) } > REGION_TEXT

This is the suggestion from the linked issue, and also works to resolve the issue.

I think the main problem is that the INFO attribute clears the SHF_ALLOC bit in sh_flags: https://lld.llvm.org/ELF/linker_script.html#output-section-type

The INFO attribute is what looks to have been causing the error, since it was telling rust-lld to not allocate space for the eh_frame and eh_frame_hdr sections. Then, when referenced by another part of code, they were seen as being outside the allocated memory.

So, removing the eh_frame and eh_frame_hdr sections entirely, or removing the INFO section attribute resolves the issue.

Completely removing the sections may be the better option, as it allows downstream users to define them when necessary, without forcing them to place them in the REGION_TEXT region.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan to work on this PR tomorrow. I hope I can push something reviewable by the end of the day

@romancardenas
Copy link
Contributor Author

Some differences I found compared to cortex-m-rt:

  1. In cortex-m-rt, link.x.in includes memory.x to get the device memory layout. In riscv-rt, BSPs have a board.x linker file that includes both memory.x from PACs and link.x from riscv-rt.
  2. In cortex-m-rt, there is only FLASH and RAM regions. In riscv-rt, we have REGION_TEXT (usually an alias of FLASH), REGION_RODATA (usually an alias of FLASH), REGION_DATA (usually an alias of RAM), REGION_BSS (usually an alias of RAM, this region is ficticious), REGION_HEAP (usually an alias of RAM, this region is fictitious), and REGION_STACK (usually an alias of RAM, this region is fictitious).
  3. In cortex-m-rt, the linker file usually does things like:
.text _stext :
  {
    __stext = .;
  ...
  . = ALIGN(4); /* Pad .text to the alignment to workaround overlapping load section bug in old lld */
   __etext=.;
  } > FLASH

In riscv-rt, we just use the _stext symbol and forget about the __stext and __etext markers.

@romancardenas
Copy link
Contributor Author

Also, cortex-m-rt includes an uninit section at the bottom of the RAM. We don't add anything after the stack

@romancardenas
Copy link
Contributor Author

For the new release of riscv-rt, I would focus on the current changes in the linker file. We can explore other changes for a next iteration, if we think they are valuable

@@ -177,72 +123,189 @@
//! REGION_ALIAS("REGION_DATA", RAM);
//! REGION_ALIAS("REGION_BSS", RAM);
//! REGION_ALIAS("REGION_HEAP", RAM);
//! REGION_ALIAS("REGION_STACK", L2_LIM);
//! REGION_ALIAS("REGION_STACK", RAM);
Copy link
Contributor

Choose a reason for hiding this comment

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

L2_LIM is a region distinct from main ram. At least on SiFive U74 cores, this is the SRAM region.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can rescue the previous example in which L2_LIM is used.

@@ -1,126 +1,44 @@
//! Minimal startup / runtime for RISC-V CPU's
//! Startup code and minimal runtime for RISC-V microcontrollers
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these changes imply incompatibility with application SoCs with multiple HARTs?

Why the change from CPU to microcontroller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copy/pasted the docs in cortex-m-rt, the liker should present the exact same configuration as before

Comment on lines +238 to +241
ERROR(riscv-rt): .got section detected in the input files. Dynamic relocations are not
supported. If you are linking to C code compiled using the `cc` crate then modify your
build script to compile the C code _without_ the -fPIC flag. See the documentation of
the `cc::Build.pic` method for details.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to support PIC relocation in the future? If it is, this is something I would be interested in working on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an interesting research line. Especially for RISC-V 64 targets which currently face issues with static linking (see #153). However, it would need specific code to deal with dynamic linking etc.

Copy link
Contributor

@rmsyn rmsyn left a comment

Choose a reason for hiding this comment

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

LGTM

@romancardenas romancardenas added this to the riscv 0.13.0 milestone Nov 7, 2024
@romancardenas romancardenas added this pull request to the merge queue Nov 8, 2024
Merged via the queue into master with commit fdc3bb6 Nov 8, 2024
101 checks passed
@romancardenas romancardenas deleted the clean_linker_file branch December 11, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

riscv-rt: Broken eh_frame relocations on QEMU
2 participants