Page MenuHomePhabricator

[ELF] Change GOT*_FROM_END (relative to end(.got)) to GOTPLT* (start(.got.plt))
ClosedPublic

Authored by MaskRay on Mar 20 2019, 3:14 AM.

Details

Summary

This should address remaining issues discussed in PR36555.

Currently R_GOT*_FROM_END are exclusively used by x86 and x86_64 to
express relocations types relative to the GOT base. We have
_GLOBAL_OFFSET_TABLE_ (GOT base) = start(.got.plt) but end(.got) !=
start(.got.plt)

This can have problems when _GLOBAL_OFFSET_TABLE_ is used as a symbol, e.g.
glibc dl_machine_dynamic assumes _GLOBAL_OFFSET_TABLE_ is start(.got.plt),
which is not true.

extern const ElfW(Addr) _GLOBAL_OFFSET_TABLE_[] attribute_hidden;
return _GLOBAL_OFFSET_TABLE_[0]; // R_X86_64_GOTPC32

In this patch, we

  • Change all GOT*_FROM_END to GOTPLT* to fix the problem.
  • Add HasGotPltOffRel to denote whether .got.plt should be kept even if the section is empty.
  • Simply GotSection::empty and GotPltSection::empty a bit by setting HasGotOffRel and HasGotPltOffRel early.

The change of R_386_GOTPC makes X86::writePltHeader simpler as we don't
have to compute the offset start(.got.plt) - Ebx (it is constant 0).

We still diverge from ld.bfd (at least in most cases) and gold in that
.got.plt and .got are not adjacent, but the advantage doing that is
unclear.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

MaskRay created this revision.Mar 20 2019, 3:14 AM
MaskRay updated this revision to Diff 191468.Mar 20 2019, 4:06 AM
MaskRay marked an inline comment as done.

Improve 3 tests

sivachandra accepted this revision.Mar 20 2019, 10:00 AM
This revision is now accepted and ready to land.Mar 20 2019, 10:00 AM
ruiu added inline comments.Mar 20 2019, 3:35 PM
ELF/Arch/X86.cpp
195–197

Is this comment still correct? You are no longer modifying these instructions' operands, so I'm guessing they now have different operands.

MaskRay marked 2 inline comments as done.Mar 20 2019, 4:36 PM
MaskRay added inline comments.
ELF/Arch/X86.cpp
195–197

Ebx = GotPlt now so the 4 lines below can be deleted.

test/ELF/plt-i686.s
148–149

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp cannot recognize our unusual GOT base (end(.got)). It is good now :)

// The jmp instruction at the beginning of each PLT entry jumps to the
// address of the base of the .got.plt section plus the immediate.
uint32_t Imm = support::endian::read32le(PltContents.data() + Byte + 2);
Result.push_back(
    std::make_pair(PltSectionVA + Byte, GotPltSectionVA + Imm));
Byte += 6;
ruiu added inline comments.Mar 20 2019, 4:40 PM
ELF/Arch/X86.cpp
195–197

Then I guess you need to update that comment to

pushl 4(%ebx)
jmp *8(%ebx)

no?

MaskRay updated this revision to Diff 191616.Mar 20 2019, 6:06 PM
MaskRay marked 2 inline comments as done.

Update the comment to

pushl 4(%ebx)
jmp *8(%ebx)

MaskRay marked an inline comment as not done.Mar 20 2019, 6:06 PM
MaskRay marked an inline comment as done.Mar 20 2019, 6:09 PM
ruiu added a comment.Mar 21 2019, 1:24 PM

I don't remember why we defined _FROM_END relocation types in the first place. That was done by Rafael. It looks to me that this change is looking generally good. I hope there's no hidden reason we did that at that time...

ELF/Relocations.cpp
1073–1075

Is this logically x86-64 only? I think it is fine to create a .got when you create .got.plt, but the comment should explain that.

ELF/Relocations.h
34–35

Sort.

sivachandra added inline comments.Mar 21 2019, 3:29 PM
ELF/Relocations.cpp
1073–1075

One need not emit .got even if emitting .got.plt. For example, for non-PIE but statically linked excutables, ld.bfd does not emit .got but still emits .got.plt.

That said, I am not sure if not emitting a .got (when emitting .got.plt) for other kinds of executables is a good/bad thing. IMO, emitting a .got even when not required will only add a few bytes at most.

MaskRay marked an inline comment as done.Mar 21 2019, 4:35 PM
MaskRay added inline comments.
ELF/Relocations.cpp
1073–1075

R_GOTPLT, R_GOTPLTONLY_PC, and R_GOTPLTREL are x86-32 x86-64 only. It currently emits .got because we need to compute the distance between .got and .got.plt. The empty .got will serve as an anchor.

I read some history in the past and I remember that *_FROM_END was chosen to make the GOT base in the middle of .got

AMD64 psABI p.75

The symbol _GLOBAL_OFFSET_TABLE_ may reside in the middle of the .got section,

allowing both negative and non-negative offsets into the array of addresses.

But in reality that rule doesn't have to be followed strictly.. @sivachandra mentioned https://fedoraproject.org/wiki/Changes/.got.plt_Isolation Now I'm thinking ld.bfd may in some configuration allow .got and .got.plt to be separated. If that is true, The start of .got.plt would still be the GOT base as end(.got) would break things...

MaskRay updated this revision to Diff 191799.Mar 21 2019, 4:49 PM
MaskRay marked an inline comment as done.

Update

sivachandra added inline comments.Mar 21 2019, 8:45 PM
ELF/Relocations.cpp
1073–1075

Note that the ABI doc does not prescribe .got.plt. In fact, AFAICT, there is no mention of such a section in the doc.

The way I view/interpret it is this:

  1. .got.plt is a section which linkers have been emitting as a convenient logical separation: everything .got pertains to global variables, and every thing after that supports .plt.
  2. As a convention, they have been making the address of GOT = start(.got.plt)
  3. So, by placing .got.plt right after .got, they still adhere to the ABI which says GOT may reside in the middle of .got. That is, .got.plt is merely an extension of .got.

Just curious, for what purposes does one need to compute the distance between .got and .got.plt?

MaskRay updated this revision to Diff 191823.Mar 21 2019, 10:20 PM

Update code/tests related to R_GOTPLT R_TLSGD_GOTPLT

MaskRay updated this revision to Diff 191824.Mar 21 2019, 10:39 PM

Simplify In.GotPlt->HasGotPltOffRel = true; after reading the interaction again...

This gives me more confidence..

ninja -C Release clang lld  # build lld with this change

cmake -GNinja -H. -BGOT -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER=$HOME/llvm/Release/bin/clang++ -DCMAKE_C_COMPILER=$HOME/llvm/Release/bin/clang -DLLVM_ENABLE_LLD=On -DLLVM_INCLUDE_GO_TESTS=OFF -DLLVM_INCLUDE_DOCS=OFF -DLLVM_INCLUDE_EXAMPLES=OFF -DBUILD_SHARED_LIBS=On
ninja -C GOT check-all  # all (llvm+clang+extra+lld+compiler-rt) passed

This gives me more confidence..

ninja -C Release clang lld  # build lld with this change

cmake -GNinja -H. -BGOT -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER=$HOME/llvm/Release/bin/clang++ -DCMAKE_C_COMPILER=$HOME/llvm/Release/bin/clang -DLLVM_ENABLE_LLD=On -DLLVM_INCLUDE_GO_TESTS=OFF -DLLVM_INCLUDE_DOCS=OFF -DLLVM_INCLUDE_EXAMPLES=OFF -DBUILD_SHARED_LIBS=On
ninja -C GOT check-all  # all (llvm+clang+extra+lld+compiler-rt) passed

I've tested this patch on our internal code base and I don't find any issues.

ruiu accepted this revision.Mar 25 2019, 2:45 PM

LGTM

Let's give it a shot. Thank you for doing this.

This revision was automatically updated to reflect the committed changes.