This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Emit relocation for uleb128
AbandonedPublic

Authored by kito-cheng on Jan 30 2023, 2:50 AM.

Details

Reviewers
asb
jrtc27
MaskRay
Summary

Support for the uleb128 related relocation[1], ULEB128 format has used
by DWARF and exception handling table, which used ULEB128 to record data
and the distance of two symbols, however the linker relaxation might
change the symbol address, so we need to insert relocations for those
data to make sure they are still correct after linker relaxation.

[1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/361

Diff Detail

Event Timeline

kito-cheng created this revision.Jan 30 2023, 2:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 2:50 AM
kito-cheng requested review of this revision.Jan 30 2023, 2:50 AM

Changes:

  • Add testcase.
craig.topper added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
295

Looks like most other places in RISCV use MCFixupKind(RISCV::fixup_riscv_set_uleb128) instead of the static_cast.

jrtc27 added inline comments.Jan 30 2023, 8:49 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
283

encodeULEB128 takes a uint64_t

287

Why not just use encodeULEB128's PadTo and encode 0?

MaskRay added inline comments.Jan 30 2023, 11:29 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
277

delete the blank line. The prevailing LLVM style does not prefer inserting a blank line after a declaration.

287

-1ll is not tested. This needs a test with llvm-readelf -x $name or llvm-objdump -s dumping the section content.

llvm/test/MC/RISCV/fixups-expr.s
30

Suggest: create a new file for .uleb128. Add a test where .uleb128 precedes the label definition (it currently crashes).

Add a test with labels defined relative to two different sections.

I don't have a very strong opinion about whether non-local symbols should be allowed. For psABI, it may be safer to start with more restriction.

craig.topper added inline comments.Mar 8 2023, 2:54 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
57

59 has been claimed by R_RISCV_PLT32 now.

kito-cheng marked an inline comment as done.

Changes:

  • Update relocation number to 60 and 61.
  • Move testcase to fixups-expr-uleb128.s
  • Reserved enough bytes when the symbol can't evaluated at assembly time.
kito-cheng marked 5 inline comments as done.Mar 23 2023, 1:12 AM
kito-cheng added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
283

evaluateAsAbsolute take int64_t &...added a check to make sure that won't overflow.

kito-cheng edited the summary of this revision. (Show Details)May 24 2023, 9:03 AM

Changes:

  • Rebase
  • Add new option to control the relocation emition for ULEB128: -riscv-enable-uleb128
  • Default disable to prevent potential incompatible issue with old binutils, will default turn on in future.
NOTE: psABI spec has merged and binutils support also merged into trunk.
evandro removed a subscriber: evandro.May 24 2023, 1:48 PM

This patch looks fine as to implement the functionality, but I am afraid that the used requiresFixup and evaluateAsAbsolute are not really reliable (https://maskray.me/blog/2021-03-14-the-dark-side-of-riscv-linker-relaxation "Label differences related to text section symbols"). Ideally we should not need`EnableULEB128Reloc`.

I plan to improve the reliability shortly.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
32
kito-cheng added inline comments.Jun 27 2023, 5:59 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
32

The intention of this option is considering the binutils compatibility, binuilts has support that on trunk which will be part of binutils 2.41, but it's still not release yet.

Anyway personally I don't have strong opinion on the default value of this option.

Changes:

  • Rebase to trunk.
  • Apply @MaskRay's suggesion.
MaskRay added inline comments.Jul 13 2023, 8:26 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
277

evaluateAsAbsolute has now been correct (D153097). I am now mainly concerned with our inaccurate requiresFixups and whether there will be users rely on a possibly buggy behavior...

llvm/test/MC/RISCV/fixups-expr-uleb128.s
60 ↗(On Diff #538013)

Add -NEXT whenever applicable. Check the preceding line .rela.xxx { and the the pairing } to assert that there is no other relocations.

Changes:

  • Update testcase to make sure no other unexpected relocation.
kito-cheng marked an inline comment as done.Jul 13 2023, 11:46 PM
kito-cheng added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
277

Did you have suggestion on this, I guess I am not really family enough around that :(

MaskRay added inline comments.Jul 14 2023, 11:50 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
277

I took quite some time today to investigate it.... See D155357

I think we need a custom fixup to represent the fixup_riscv_set_uleb128 and fixup_riscv_sub_uleb128 pair.

In .handleAddSubRelocations, split the fixup into 2 relocations.

MaskRay added inline comments.Jul 14 2023, 11:51 PM
llvm/test/MC/RISCV/fixups-expr-uleb128.s
59 ↗(On Diff #540288)

The interesting case is when we use a linker-relaxable instruction here.

+relax and -relax can check the behavior. See my llvm/test/MC/RISCV/fixups-expr.s change.

Changes:

kito-cheng marked an inline comment as done.Jul 17 2023, 7:06 AM
kito-cheng added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
277

Thanks, I am based on D155357 now :)

I am out of town for a few days, but I'll try to read this soon.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
199

Only used once. Inline the variable where it is used.

203

Delete blank line here.

llvm/test/MC/RISCV/fixups-expr-uleb128.s
18 ↗(On Diff #541010)

For a cl::opt option that defaults to false, we usually don't test the case when we explicitly set it to false. This additional case doesn't pull its weight.

kito-cheng updated this revision to Diff 548129.Aug 8 2023, 2:39 AM

Changes:

MaskRay added inline comments.Aug 9 2023, 5:03 PM
llvm/include/llvm/MC/MCFixup.h
28 ↗(On Diff #548129)

We do not need this generic fixup.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
161

These fixup_riscv_{set,add,sub}_* from D103539 are unneeded. I did not put the cleanup into D155357 to minimize the changes in D155357 . I have now removed the unneeded fixup kinds in c8ed138c34ddb22610f18468c1b7938f9e2abae5.

I think you can just use FirstLiteralRelocationKind + ELF::R_RISCV_SET_ULEB128 instead of adding new fixup kinds.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
214

Prefer getContext().reportError for possible error paths. report_fatal_error should be avoided if possible.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h
107

Unneeded. See my other comment.

MaskRay added a comment.EditedAug 9 2023, 6:09 PM

I think the right implementation will be quite different from this patch. We need to modify quite a few more places including shouldForceRelocation, MCAssembler::relaxLEB (new function, similar to relaxDwarf*), and MCObjectStreamer::emitULEB128Value.
I am working on an alternative (current draft: https://github.com/MaskRay/llvm-project/tree/mc-uleb128)

@MaskRay Really appropriate your help! it seems like I didn't handle the dwarf stuffs right according your version, and ULEB128 relocation support might be a compatible issue between GNU and LLVM toolchain, so I wondering do you think it's better to split this patch into add relocation number only and add the lld support(D142880) first for LLVM 17 branch?

@MaskRay Really appropriate your help! it seems like I didn't handle the dwarf stuffs right according your version, and ULEB128 relocation support might be a compatible issue between GNU and LLVM toolchain, so I wondering do you think it's better to split this patch into add relocation number only and add the lld support(D142880) first for LLVM 17 branch?

Thanks:) Having the ELF_RELOC(R_RISCV_SET_ULEB128, 60) ... 61 change in release/17.x will be useful.

Having the lld change will be good for our compatibility story if we can catch the release schedule. But it seems that the change is quite complex and I am unsure we have sufficient time...

kito-cheng abandoned this revision.Sun, Nov 19, 10:24 PM

MaskRay has landed a better version :)