-
Notifications
You must be signed in to change notification settings - Fork 166
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
Conversation
I am currently going through |
riscv-rt/link.x.in
Outdated
/* .eh_frame (INFO) : { KEEP(*(.eh_frame)) } */ | ||
/* .eh_frame_hdr (INFO) : { *(.eh_frame_hdr) } */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* .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.
There was a problem hiding this comment.
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
Some differences I found compared to
In |
Also, |
d69ea9b
to
2ce267a
Compare
For the new release of |
riscv-rt/src/lib.rs
Outdated
@@ -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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
riscv-rt/src/lib.rs
Outdated
@@ -1,126 +1,44 @@ | |||
//! Minimal startup / runtime for RISC-V CPU's | |||
//! Startup code and minimal runtime for RISC-V microcontrollers |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
c45de98
to
8f57be8
Compare
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."); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 #196I still need to play around with this, but this is a good first step
Closes #196