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
Advise, but don't yet require, use of the GOT for medany weak undef #201
Conversation
Cc @MaskRay (can't request a review from you as you're not a member of the repo... might be worth granting you explicit read access, I think that lets you be added as a reviewer?) |
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.
LG.
I think "undefined weak" should not be on a performance bottleneck of any applications (even embedded ones), so using GOT for compiler codegen and leaving the optional instruction rewriting for the linker (even if it is implemented in no implementation right now) is perfectly fine. We just can't help users who want to shoot themselves in the foot...
|
||
> NOTE: This is not yet a requirement as existing toolchains predating this | ||
> part of the specification do not adhere to this, and without improvements to | ||
> linker relaxation support doing so would regress performance and code size. |
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 see that the "regress performance and code size" part is for @jim-wilson ....
I still hope that we can just remove this sentence as I don't see how usage of undefined weak symbols can become a bottleneck of anything... (Even if it is in a loop, loop-invariant code motion can optimize it. If it is in a function called numerous times, the function call amortizes the (hardly any) indirection cost.)
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.
Loop-invariant code motion is essentially irrelevant here. Jim's contention is about code size, not dynamic instruction count. Jim's concern remains relevant.
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.
The newlib crt0.S contains an undefined weak. This means that anyone building a riscv64-elf compiler from riscv-gnu-toolchain and compiling the official RISC-V embench testsuite will see code size increases. That is a big problem in the embedded world.
There is no problem in the Linux world. Anyone that cares about code size or performance of undefined weak is probably confused.
The GNU toolchain has been supporting weak for static linking since the 1990s, and this is commonly used in embedded programming. I don't know if clang supports this. But in the GNU world this is a feature that needs to work efficiently.
Having GNU ld relaxations to optimize the code will mitigate the code size and performance regressions, but then there is the problem of who will write the code and when. There is a shortage of people actively working upstream in the GNU toolchain. So until that is done, we can't mandate the change, and there is no way to know when the work will be done.
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.
Hang on, are we talking about a single reference to a single variable? I.e. at worst a code size increase of 6 bytes (2-byte compressed to 2*4-byte uncompressed) and data size increase of 4 bytes on RV32 and 8 bytes on RV64?
This is only OK to me if the change remains optional until someone writes the gnu ld relaxation support to optimize the result back to what it was before the ABI change. |
I think we all agree that would be better way for non-embedded world part like Linux, but I am in the same line with Jim for the embedded world part - few bytes is matter in the embedded world, I am OK with this for optional and no deprecated in the ABI doc. |
Still LGTM. This will give clang license to switch to the correct model. |
LLVM patch: https://reviews.llvm.org/D107280 |
@jim-wilson Are you happy with the current wording? |
When building with clang -mcmodel=medany and linking with ld.lld, we get out-of-range relocation errors for undefined __start_<section> symbols since 0 cannot be represented as a PC-relative offset). This is not a problem with ld.bfd since ld.bfd rewrites the instructions to avoid the out-of-range PC-relative relocation. For now, the simplest workaround is to build with -fpie -mcmodel=medany (thus indirecting these symbols via the GOT). This will be done automatically once clang includes https://reviews.llvm.org/D107280. Without this change I get the following linker errors: ld.lld: error: dev/driver.c:21:(.text+0x1E): relocation R_RISCV_PCREL_HI20 out of range: -524295 is not in [-524288, 524287]; references __start_devices See riscv-non-isa/riscv-elf-psabi-doc#126 and riscv-non-isa/riscv-elf-psabi-doc#201.
When building with clang -mcmodel=medany and linking with ld.lld, we get out-of-range relocation errors for undefined __start_<section> symbols since 0 cannot be represented as a PC-relative offset). This is not a problem with ld.bfd since ld.bfd rewrites the instructions to avoid the out-of-range PC-relative relocation. For now, the simplest workaround is to build with -fpie -mcmodel=medany (thus indirecting these symbols via the GOT). This will be done automatically once clang includes https://reviews.llvm.org/D107280. Without this change I get the following linker errors: ld.lld: error: dev/driver.c:21:(.text+0x1E): relocation R_RISCV_PCREL_HI20 out of range: -524295 is not in [-524288, 524287]; references __start_devices See riscv-non-isa/riscv-elf-psabi-doc#126 and riscv-non-isa/riscv-elf-psabi-doc#201.
When building with clang -mcmodel=medany and linking with ld.lld, we get out-of-range relocation errors for undefined __start_<section> symbols since 0 cannot be represented as a PC-relative offset). This is not a problem with ld.bfd since ld.bfd rewrites the instructions to avoid the out-of-range PC-relative relocation. For now, the simplest workaround is to build with -fpie -mcmodel=medany (thus indirecting these symbols via the GOT). This will be done automatically once clang includes https://reviews.llvm.org/D107280. Without this change I get the following linker errors: ld.lld: error: dev/driver.c:21:(.text+0x1E): relocation R_RISCV_PCREL_HI20 out of range: -524295 is not in [-524288, 524287]; references __start_devices See riscv-non-isa/riscv-elf-psabi-doc#126 and riscv-non-isa/riscv-elf-psabi-doc#201.
When building with clang -mcmodel=medany and linking with ld.lld, we get out-of-range relocation errors for undefined __start_<section> symbols since 0 cannot be represented as a PC-relative offset). This is not a problem with ld.bfd since ld.bfd rewrites the instructions to avoid the out-of-range PC-relative relocation. For now, the simplest workaround is to build with -fpie -mcmodel=medany (thus indirecting these symbols via the GOT). This will be done automatically once clang includes https://reviews.llvm.org/D107280. Without this change I get the following linker errors: ld.lld: error: dev/driver.c:21:(.text+0x1E): relocation R_RISCV_PCREL_HI20 out of range: -524295 is not in [-524288, 524287]; references __start_devices See riscv-non-isa/riscv-elf-psabi-doc#126 and riscv-non-isa/riscv-elf-psabi-doc#201.
Closes: #126