Page MenuHomePhabricator

[lld] Support dynamic linking in RISC-V
ClosedPublic

Authored by PkmX on Oct 26 2017, 1:25 AM.

Details

Reviewers
ruiu
asb
espindola
Summary

This patch adds support for handling of PIC, generation of GOT/PLT and creation of PIE executables or DSO.

Some comments:

  • It's currently missing the first entry of GOT which should be the address of .dynamic section. This seems to be the case for other targets as well, for example x86_64 ABI requires the address in .got so _GLOBAL_OFFSET_TABLE[0] == &_DYNAMIC but lld puts it in .got.plt instead. For now this doesn't appear to be used as well.
  • RISC-V uses ADDN/SETN/SUBN relocation pairs to calculate differences of address in DWARF. This means that lld tries to create dynamic relocations for those while they are actually link-time constants as a pair, so we set those in RISCV::usesOnlyLowPageBits.

Diff Detail

Event Timeline

PkmX created this revision.Oct 26 2017, 1:25 AM
ruiu added inline comments.Oct 26 2017, 2:37 PM
ELF/Arch/RISCV.cpp
76–77 ↗(On Diff #120368)

clang-format-diff please.

82–83 ↗(On Diff #120368)

Use Config->Wordsize instead of sizeof(typename ELFT::uint);

107–111 ↗(On Diff #120368)

I added a new file ELF/Bits.h. Please use writeUint and readUint defined in that file.

PkmX updated this revision to Diff 120542.Oct 26 2017, 11:38 PM
PkmX marked 3 inline comments as done.
ruiu added inline comments.Oct 27 2017, 11:18 AM
ELF/Arch/RISCV.cpp
62–72 ↗(On Diff #120542)

Since now we assume it is little-endian, we don't write it in a generic way. You can use just write32le and read32le.

I wonder if we even had to make this class a template class because the difference between RISCV64 and RISCV32 seem pretty small, and we can easily handle differences with Config->Is64.

PkmX updated this revision to Diff 120795.Oct 30 2017, 4:25 AM

De-template RISCV class and assume little-endian.

ruiu added inline comments.Oct 30 2017, 1:38 PM
ELF/Arch/RISCV.cpp
110 ↗(On Diff #120795)

Remove const

136–142 ↗(On Diff #120795)

Please apply clang-format-diff.

ELF/Relocations.cpp
416–418 ↗(On Diff #120795)

I think a better way of doing it is to define __global_pointer$ as a relative symbol in the first place. That's what we are doing for linker-synthesized symbols such as _etext or _edata. Please take a look at Writer.cpp.

PkmX updated this revision to Diff 120940.Oct 30 2017, 11:54 PM
PkmX marked 3 inline comments as done.
  • Use write32le.
  • Since __global_pointer$ is no longer absolute, remove workaround to make it a static link-time constant in PIE.
PkmX updated this revision to Diff 121107.Nov 1 2017, 1:49 AM

clang-format-diff RISCV::writePLT.

PkmX updated this revision to Diff 121838.Nov 6 2017, 11:13 PM
  • Rebase onto latest master
  • Clean up #includes
PkmX updated this revision to Diff 127857.Dec 21 2017, 3:22 AM

Rebase onto latest master.

PkmX updated this revision to Diff 133980.Feb 12 2018, 7:33 PM

Rebase onto master.

PkmX updated this revision to Diff 158927.Aug 2 2018, 11:50 PM

Rebased.

RISCV now writes its own GOT header, with one entry that stores the address of .dynamic section. _GLOBAL_OFFSET_TABLE_ should be set to the head of .got so that _GLOBAL_OFFSET_TABLE_[0] == &_DYNAMIC.

jrtc27 added a subscriber: jrtc27.Oct 30 2018, 7:47 AM

This needs rebasing again as NoneRel = R_RISCV_NONE was added to the constructor. Otherwise, is there anything blocking this?

ruiu added a comment.Oct 30 2018, 1:12 PM

Ditto -- if you can write tests in assembly, please convert them from YAML to assembly.

PkmX added a comment.Nov 1 2018, 12:31 AM

Ditto -- if you can write tests in assembly, please convert them from YAML to assembly.

LLVM RISC-V MC does not support generating PIC yet, so we can only use YAML at the moment.

ruiu added inline comments.Nov 1 2018, 11:13 AM
ELF/Arch/RISCV.cpp
33–43 ↗(On Diff #158927)

Remove "virtual" for consistency. "override" implies it.

107 ↗(On Diff #158927)

What is this field for? Perhaps a brief explanation of the structure of .got.plt header for RISC-V would be helpful for readers.

122 ↗(On Diff #158927)

Side note: it is nice to see that many instructions for RISCV32 and RISCV64 are represented by the same bit pattern. I guess it's a beauty of the new clean ISA.

156 ↗(On Diff #158927)

nit: indentation

PkmX marked 4 inline comments as done.Nov 1 2018, 7:35 PM
PkmX added inline comments.
ELF/Arch/RISCV.cpp
107 ↗(On Diff #158927)

It is a placeholder for the dynamic linker to fill with the address of _dl_runtime_resolve function. The content of .got.plt section is not explicitly described in the ABI spec, but you can sort of infer that from the PLT header:

https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#program-linkage-table

The first entry of .got.plt will be the address of _dl_runtime_resolve, and the second entry is the link map, passed as an argument to _dl_runtime_resolve. I don't think the initial values matter here, so I just take them from bfd's implementation: first entry is all ones and the second is all zeros.

PkmX updated this revision to Diff 172307.Nov 1 2018, 11:35 PM
PkmX marked an inline comment as done.

Rebased onto trunk.

MaskRay added inline comments.Nov 2 2018, 5:42 PM
ELF/Arch/RISCV.cpp
108 ↗(On Diff #172307)

Verified the first entry is -1 set by bfd/elfnn-riscv.c

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=e8bf1ce461df242811e49de807f85c2e7ae17b77;f=bfd/elfnn-riscv.c#l2585

This was introduced in the initial import commit e23eba971dd409b999dd83d8df0f842680c1c642 , no history to help understand why -1 (a bit odd) was chosen.

In other bfd's other architectures and lld's (GotPltHeaderEntriesNum), _dl_runtime_resolve is 0 at link time.

115 ↗(On Diff #172307)

The check if (ElfSym::GlobalOffsetTable) may be redundant.

ruiu added inline comments.Nov 5 2018, 1:23 AM
ELF/Arch/RISCV.cpp
107 ↗(On Diff #158927)

We are trying to avoid cargo-culting and we should avoid writing values if the only reason to do so is to write the same value as ld.bfd. Let's just leave them as-is (i.e. leaving them as zero).

PkmX, any chance of addressing these points soon, please?

jrtc27 added inline comments.Nov 13 2018, 7:19 AM
ELF/Arch/RISCV.cpp
115 ↗(On Diff #172307)

If you want to write to the GOT header, you do need this guard (PPC64 doesn't, likely because of its TOCs and so the symbol is always used), as the GotSection constructor only adds GotHeaderEntriesNum to NumEntries if ElfSym::GlobalOffsetTable && !Target->GotBaseSymInGotPlt. Otherwise you risk clobbering the first GOT entry if it's a static link time constant.

However, I don't think we even need this header. BFD will emit this header on, for example, AArch64, if the GOT is non-empty (as will gold), but we never do this as far as I can tell. Thus unless RISC-V is special and the runtime really assumes this header entry is present, I think we should drop this header as it hasn't seemed to cause any problems with AArch64; we can always add it if we later find it necessary.

ruiu added inline comments.Nov 13 2018, 9:55 PM
ELF/Arch/RISCV.cpp
115 ↗(On Diff #172307)

That' s a really good point. I remember at some point of the lld development, we wondered why we were adding this header even though it is not technically necessary. I turned out that we are doing it because the ABI standard requires it, but we couldn't find any technical reason to do so. Rafael wanted we stop adding this header because this is just a cargo-culting, but we decided to continue doing it for the sake of ABI compliance.

If you are defining a new ABI, I recommend you not blindly copy this header. I recommend you drop this header.

MaskRay added inline comments.Nov 14 2018, 12:49 PM
ELF/Arch/RISCV.cpp
115 ↗(On Diff #172307)

Putting _GLOBAL_OFFSET_TABLE_[0] = _DYNAMIC in .got or .got.plt is a trick used by glibc/sysdeps/{aarch64,x86_64,riscv,...}/dl-machine.h.
It subtracts the link time address from the runtime time (usually relocated by a R_*_RELATIVE) to get the load address of ld.so

It seems to me a bit unnecessary that some old ABIs mandate the field as I think most userspace applications don't need this symbol.

jrtc27 added inline comments.Nov 14 2018, 1:25 PM
ELF/Arch/RISCV.cpp
115 ↗(On Diff #172307)

Ah, I see, it's needed when you're linking ld.so (and it uses the _GLOBAL_OFFSET_TABLE_ symbol as a variable containing a word, hence why we only do this when the symbol is referenced). So I guess we can't link glibc's ld.so on AArch64? If this is an issue affecting more than just RISC-V, I'd still be inclined to drop this from the patch, but then later fix both AArch64 and RISC-V (along with any other architectures also needing this) at the same time as a separate bug fix.

@PkmX Ping, any chance you could address the remaining few points, please?

PkmX updated this revision to Diff 194177.Apr 8 2019, 11:15 AM

Rebased onto master and:

  • Removed the GOT header per discussion
  • Rewrote all tests in MC
ruiu added a comment.Apr 10 2019, 3:26 AM

Overall looking good.

ELF/Arch/RISCV.cpp
116 ↗(On Diff #194177)

It looks odd to me at first because you are setting the same value to all .got.plt slots. But I guess this is correct; jalr in .plt sets the return address to t3, so we can calculate a .plt index by t3 minus .plt start address. Nice. A little explanation would help readers understand this code, though.

121 ↗(On Diff #194177)

Please use error() instead of fatal(). We use fatal() only to report corrupted files. After you report an error using error(), you can just return from this function.

PkmX updated this revision to Diff 194473.Apr 10 2019, 4:30 AM
PkmX marked 2 inline comments as done.

Addressed the review comments:

  • Added comments to writeGotPlt.
  • Use error instead of fatal when generating PLT on RV32E.
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2019, 4:30 AM
ruiu accepted this revision.Apr 10 2019, 5:34 AM

LGTM

This revision is now accepted and ready to land.Apr 10 2019, 5:34 AM
lenary added a subscriber: lenary.Aug 15 2019, 2:49 AM

Am I correct in thinking that this functionality has already been upstreamed into LLD, and that this patch can now been abandoned?

MaskRay closed this revision.Aug 15 2019, 3:14 AM

Am I correct in thinking that this functionality has already been upstreamed into LLD, and that this patch can now been abandoned?

Yes, the functionality has already been upstreamed. I somehow forgot this one while I was doing that... Mainly D63076 (GOT,PLT,COPY,relative), D63220 (TLS) and a few other fixups.
The functionality should be pretty complete in the llvm 9 release: I built libc-test against glibc riscv64 and musl riscv64.

R_RISCV_RELAX/R_RISCV_ALIGN has not been implemented. There is some technical difficulty - shrinkable sections do not fit in lld's current framework. It can be implemented but generally I think R_RISCV_RELAX can have other problems... e.g. it breaks split dwarf, it disallows section symbols in object files, etc.