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

Add GP-relative relocations #394

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Conversation

luismarques
Copy link
Collaborator

Following-up on @kito-cheng's suggestion to add the ePIC proposal content to the existing psABI documents, this initial PR adds the GP-relative relocations. These GP-relocations should be useful for purposes besides ePIC (e.g. see previous proposals of the compact/large code models).

For ease of review, I will make a separate PR to add R_RISCV_GPREL_ADD, used for relaxation purposes, unless you prefer otherwise.

I'm not sure these GP relocations merit a GP-relative addressing section, such as the existing "PC-Relative Symbol Addresses", as there aren't the same kind of complications to comment on. If you feel it's needed, I will add one to this PR.

riscv-elf.adoc Outdated Show resolved Hide resolved
@kito-cheng
Copy link
Collaborator

Did you mind post the link of the lld or ld.bfd implementation for the process in the psABI - we require PoC for either linker :)

@luismarques
Copy link
Collaborator Author

Did you mind post the link of the lld or ld.bfd implementation for the process in the psABI - we require PoC for either linker :)

Here's a link to the first implementation I am aware of of these GPREL relocations, by Evandro and Nelson / SiFive, for the bfd linker: ebahapo/riscv-binutils-gdb@615efc0.

@kito-cheng
Copy link
Collaborator

cc @Nelson1225 @MaskRay linker experts

Copy link
Collaborator

@kito-cheng kito-cheng left a comment

Choose a reason for hiding this comment

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

LGTM from my side, just wait few more response from linker experts

@Nelson1225
Copy link
Collaborator

LGTM, too. It's good to see this series is still going on :)

@luismarques
Copy link
Collaborator Author

cc @Nelson1225 @MaskRay linker experts

ping @MaskRay and other linker experts

@rui314
Copy link
Collaborator

rui314 commented Sep 12, 2023

This is looking good, but we are not going to use these relocations without relaxation because the size matters for the use case, no? If so, I think we want to define the relaxation scheme at the same time.

@luismarques
Copy link
Collaborator Author

This is looking good, but we are not going to use these relocations without relaxation because the size matters for the use case, no? If so, I think we want to define the relaxation scheme at the same time.

I have a use case where relaxation is not essential. I've decided to split relaxation into a separate PR for ease of review but if you really want I can extend this PR.

@rui314
Copy link
Collaborator

rui314 commented Sep 15, 2023

I feel more comfortable with relaxation because we want to make sure that these relocations are actually relaxable. But I think that's not really a concern, as they can obviously be relaxable in the same way as R_RISCV_TPREL_* relocs. So I'm fine with this. What do you think @MaskRay?

Copy link
Collaborator

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

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

Looks fine to me as well. These relocations can be used when the segment containing GP does not have a link-time constant distance from the program counter.

@kito-cheng
Copy link
Collaborator

I am OK to add relaxation later, let moving forward :)

@kito-cheng kito-cheng merged commit df8b80b into riscv-non-isa:master Sep 25, 2023
@sorear
Copy link
Collaborator

sorear commented Feb 18, 2024

I'd like to revert this before it gets into a tagged release. Neither ePIC nor FDPIC will use it except possibly for linker-internal purposes after relaxation, and we don't assign numbers to linker-internal relocation types. My apologies for missing this earlier.

@MaskRay
Copy link
Collaborator

MaskRay commented Feb 18, 2024

I'd like to revert this before it gets into a tagged release. Neither ePIC nor FDPIC will use it except possibly for linker-internal purposes after relaxation, and we don't assign numbers to linker-internal relocation types. My apologies for missing this earlier.

I think S + A - GP can be used for non-preemptible data symbols (*_symbol_binds_local_p in GCC) with FDPIC, which is the reason I think this PR is acceptable.

I am indeed concerned with the rest part of ePIC, so I cannot sign off on them.

@sorear
Copy link
Collaborator

sorear commented Feb 18, 2024

You can only use it for non-preemptible symbols if you know for a fact at the reference site that the referenced symbol is allocated to writable memory. I don't think it's going to get anywhere near the amount of usage required to justify 1/48 of the RV32 relocation space.

@MaskRay
Copy link
Collaborator

MaskRay commented Feb 19, 2024

You can only use it for non-preemptible symbols if you know for a fact at the reference site that the referenced symbol is allocated to writable memory. I don't think it's going to get anywhere near the amount of usage required to justify 1/48 of the RV32 relocation space.

I agree with you. I have also checked GCC arm and sh4's codegen for -mfdpic. I apologize for accepting this previously. Since no upstream toolchain/libc has ever used these relocation types, it should be fine to revert the change.

GOT-indirect addressing is required for accessing data symbols under two conditions:

  • Preemptible symbols: Traditional GOT requirement.
  • Non-preemptible symbols with potential data segment placement: This includes
    • Writable data symbols: This covers both locally declared (int var;) and externally declared (extern int var;) non-const variables.
    • Potential dynamic initialization: const A a; extern const int var;
    • Certain guaranteed constant initialization: extern constinit const int *const extern_const;. Constant initialization may also require a relocation, e.g. constinit const int *const extern_const = &var;

If the referenced data symbol is non-preemptible and guaranteed to be in the text segment, we can use PC-relative addressing.
However, this scenario is remarkably rare in practice. The most reasonable use case is like the following:

const int ro_array[] = {1, 2, 3, 4}; // text segment

int get_ro_array_elem(int i) { return ro_array[i]; }

GOT-indirect addressing does not necessarily require GOT. The linker can optimize it, rendering the 3 relocation code less useful.

@sorear
Copy link
Collaborator

sorear commented Feb 24, 2024

Revert PR is #427.

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.

7 participants