This is an archive of the discontinued LLVM Phabricator instance.

[ELF2] R_X86_64_COPY relocation implemented
ClosedPublic

Authored by grimar on Oct 26 2015, 1:22 PM.

Details

Reviewers
ruiu
rafael
Summary

R_X86_64_COPY relocation implemented.

It fixes (at least I checked the mandelbrot set case):
https://llvm.org/bugs/show_bug.cgi?id=25283

It also fixed:
https://llvm.org/bugs/show_bug.cgi?id=25175

Diff Detail

Event Timeline

grimar updated this revision to Diff 38453.Oct 26 2015, 1:22 PM
grimar retitled this revision from to [ELF2] R_X86_64_COPY relocation implemented.
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
grimar added inline comments.Oct 26 2015, 1:29 PM
ELF/Writer.cpp
414

I just not sure what to use here for RoundUpToAlignment. May be should find the largest symbol and use its size for that ?

ruiu added inline comments.Oct 26 2015, 2:46 PM
ELF/InputSection.cpp
86

E->Sym.getType() is accessible from Body, so you can only pass Body to relocNeedsCopy. Then you can write.

if (Target->relocNeedsCopy(Type, Body)) {
  Type = Target->getCopyReloc();
} else if (Target->relocNeedsPlt(Type, Body)) {
...
ELF/OutputSections.cpp
171

Shared is also derived from Body, so you don't have to pass to that function.

194–196
} else if (NeedsCopy) {
203–206

Can you return here?

628

Revert the condition in if

if (SS.NeedsCopy)
  return Out...;
return 0;
1005

Remove {}. What's the point of setting Bss to OutSec? I just don't understand what that code is for.

ELF/Symbols.h
287–288
uintX_t OffsetInBss = 0;
bool NeedsCopy = false;
ELF/Target.cpp
346–348

We only create copy relocations, no? Copy relocations are consumed only by the dynamic linker.

ELF/Writer.cpp
210–211

What does this code do?

216

If Body needs a copy relocation, it never need a GOT, no? If so, relocNeedsGot should take care of that.

grimar updated this revision to Diff 38536.Oct 27 2015, 6:31 AM

Review comments addressed.

ELF/InputSection.cpp
86

No, its not. It accessible from ELFSymbolBody, which is template and inherits SymbolBody. So anyways dyn_cast with using of concrete ELFT will be needed inside relocNeedsCopy. Did you mean that is correct way ?
I changed it in this way in updated patch.

ELF/OutputSections.cpp
171

Ditto, changed.

194–196

Ok.

203–206

No. Because below that ifs section there is a code which I need:

if (IsRela)
  static_cast<Elf_Rela *>(P)->r_addend = Addend;
628

Ok.

1005

Point is to have correct Ndx in .symtab. Below output of readelf with this code and without:
with:

55: 00000000000121f8     8 OBJECT  GLOBAL DEFAULT   26 stdout

w/o:

55: 00000000000121f8     8 OBJECT  GLOBAL DEFAULT  UND stdout

26 is bss section.
I know you dont want to emulate ld behavior but thats just 3 lines of code that not only make behavor consistent with ld, which is good here I believe, but also make more clear symbol status for those who look into that table. Bss section index looks much better that UND. At least it gives surety that symbol was found and processed.

ELF/Symbols.h
287–288

Ok.

ELF/Target.cpp
346–348

Ok, that code was called because of reassigning type of relocation in InputSection.cpp:

line 87:

Type = Target->getCopyReloc();

That`s really not nessecary to do. Fixed.

ELF/Writer.cpp
210–211
  1. Founds if relocNeedsCopy and sets is to SharedSymbol->NeedsCopy which is used in void SymbolTableSection<ELFT>::writeGlobalSymbols to write correct Ndx of body section.
  1. GNU ld places symbols that needs copy relocations to .dynbss and its relocations to .rel[a].bss. Then "linker script puts the .dynbss section into the .bss section of the final image". I dont see the reasons to do the equal things if we can just allocate space in bss directly. But what we really need is to be sure that we dont create .rela.dyn relocations for one symbol twice. Thats why we have to use InRelaBss flag. It prevents to call Out<ELFT>::RelaDyn->addReloc({C, RI}); (which is at the end of this method) twice or more.

Its like Out<ELFT>::Got->addEntry(Body) sets got index and then if (Body->isInGot()) continue; called for the same.

216

Probably you`re right. The same should be applied for relocNeedsPlt then.

grimar updated this object.Oct 27 2015, 8:21 AM

About https://llvm.org/bugs/show_bug.cgi?id=25175:

I found that this patch fixes it for -O3/O2/O1, so both ld and lld produces R_X86_64_COPY _ZSt4cout and executable works fine.
But when I compile with -O0:

#include <iostream>
int main() { std::cout << "hello\n"; }

~/LLVM/build/bin/clang++ -fuse-ld=lld2 -o out -O0 main.c
~/LLVM/build/bin/clang++ -o out-ld -O0 main.c

then out contains R_X86_64_64 _ZSt4cout, but out-ld still has R_X86_64_COPY _ZSt4cout. Is it clang issue of lld one ?
Anyways I think that fix is for another patch.

then out contains R_X86_64_64 _ZSt4cout, but out-ld still has R_X86_64_COPY _ZSt4cout. Is it clang issue of lld one ?
Anyways I think that fix is for another patch.

And I mean that out crashes but out-ld works.

ruiu added inline comments.Oct 27 2015, 12:01 PM
ELF/OutputSections.cpp
203–206

This makes me think whether this function as a whole is correct or not. If IsRela is false, we compute an addend and then discard that value? If that information is discardable, what's the point of computing that value here?

1002

I think you don't want to call repl(). Body->repl() may not be of type SharedSymbol if the same symbol is resolved to something else.

if (cast<SharedSymbol<ELFT>>(Body)->NeedsCopy)
  Outsec = Out<ELFT>::Bss;
break;
ELF/Target.cpp
253

Remove this blank line.

254

Do you need "lld::elf2::" namespace specifiers?

254–259

You can make this a bit shorter.

if (Type == R_X86_64_32S || Type == R_X86_64_32 || Type == R_X86_64_PC32)
  if (auto *SS = dyn_cast<SharedSymbol<ELFT>>(&S))
    return SS->Sym.getType() == STT_OBJECT;
return false;
ELF/Writer.cpp
414

st_size is too large to align. (If a copy relocation points to an array, st_size can be very large.) Maybe we should align to some reasonable boundary, say, 32-byte boundaries for now. I don't think we are going to have tons of copy relocations, so doing something simple should suffice.

grimar added inline comments.Oct 27 2015, 12:31 PM
ELF/OutputSections.cpp
203–206

Probably it needs early return after

uintX_t OrigAddend = 0;

if IsRela false. But that is not for this patch I guess ?

ELF/Writer.cpp
414

I found the next comment in gnu code (http://www.opensource.apple.com/source/gdb/gdb-213/src/bfd/elf64-x86-64.c):

/* We need to figure out the alignment required for this symbol.  I
   have no idea how ELF linkers handle this.	16-bytes is the size
   of the largest type that requires hard alignment -- long double.  */
/* FIXME: This is VERY ugly. Should be fixed for all architectures using
   this construct.  */

So it looks we can use 16.
Also I guess even if we take 32 bytes then alignment should be calculated as max(Sym.st_size, 32) ?

grimar added inline comments.Oct 27 2015, 12:35 PM
ELF/OutputSections.cpp
203–206

*before

ruiu added inline comments.Oct 27 2015, 12:41 PM
ELF/OutputSections.cpp
203–206

Right, this is not for this patch. It's just that this is something we want to take a look (after this patch).

ELF/Writer.cpp
414

Do you mean min(Sym.st_size, 32)?

There are actually some instructions that require 32 -byte alignment such as some AVX instructions, but we probably don't have to take care of them for the copy relocation.

There seems to be no "right thing to do" here. Saving a few bytes per one copy relocation using a clever technique is very unlikely to worth the effort. How about this: Always align to 16-byte boundaries. This is very easy to understand.

grimar added inline comments.Oct 27 2015, 1:03 PM
ELF/Writer.cpp
414

Yes, "min" certainly.
Ok, I`ll align to 16 then.

grimar updated this revision to Diff 38598.Oct 27 2015, 2:35 PM

Review comments addressed.

ELF/OutputSections.cpp
1002

Fixed.

ELF/Target.cpp
253

Fixed.

254

Fixed.

254–259

Fixed.

ruiu added inline comments.Oct 27 2015, 2:50 PM
ELF/Symbols.h
96

Can you move this to SharedSymbol? This increases the size of SymbolBody at least one byte, but this field is not going to be used except for SharedSymbol.

ELF/Writer.cpp
209

No else after continue.

412

Add a brief comment here to say that we don't know the exact alignment requirement for the data copied by a copy relocation, so align that to 16 byte boundaries (which we think is large enough) unconditionally.

grimar updated this revision to Diff 38639.Oct 28 2015, 3:09 AM

Review comments addressed.

ELF/Symbols.h
96

I just removed it at all. SharedSymbol has NeedsCopy flag which is sufficient to replace the single use of InRelaBss.

ELF/Writer.cpp
209

Done.

412

Done.

grimar updated this revision to Diff 38650.Oct 28 2015, 4:43 AM

Added R_X86_64_64 to X86_64TargetInfo::relocNeedsCopy.
It can be met if compile sample from https://llvm.org/bugs/show_bug.cgi?id=25175 with -O0.
This is one of prerequisites to fix -O0 crash. Other part of fix is for another following patch since relative to lazy relocations.

ruiu added inline comments.Oct 28 2015, 8:30 AM
ELF/OutputSections.cpp
627

Drop const.

ELF/Writer.cpp
203–209

I'm trying to understand this. So, if two or more relocations exist for a shared symbol, the last relocation's NeedsCopy may override the previous results? Is this correct?

grimar added inline comments.Oct 28 2015, 8:52 AM
ELF/Writer.cpp
203–209

What kind of relocations ? According to current logic plt relocations will never happen because it is not function and so the only possible case is relocNeedsGot() which implemented as

bool X86_64TargetInfo::relocNeedsGot(uint32_t Type, const SymbolBody &S) const {

return Type == R_X86_64_GOTPCREL || relocNeedsPlt(Type, S);

}

that gives us the only case is R_X86_64_GOTPCREL.
Is it possible that symbol will require both R_X86_64_GOTPCREL and copy relocation ? If I understand correctly that case - we can use copy relocation here. So I see nothing wrong, please correct me if I am mistaken.

grimar added inline comments.Oct 28 2015, 9:01 AM
ELF/Writer.cpp
203–209

I am wrong here I think, I need to investigate that case, pelase ignore my comment.

ruiu added inline comments.Oct 28 2015, 9:05 AM
ELF/Writer.cpp
203–209

Is that the same as this?

if (auto *E = dyn_cast<SharedSymbol<ELFT>>(Body)) {
  E->NeedsCopy = E->NeedsCopy || Target->relocNeedsCopy(Type, *Body);
  if (E->NeedsCopy)
    continue;
}
grimar added inline comments.Oct 28 2015, 9:13 AM
ELF/Writer.cpp
203–209

No, its not. Because in your code any relocation that needs copy will lead to continue and that will not add the relocation to reladyn at the bottom of method.
I am trying to add the copy relocation reladyn entry but only once for symbol.

grimar added inline comments.Oct 28 2015, 9:18 AM
ELF/Writer.cpp
203–209

So I think my code is correct:
if this is copy relocation that if it is the first one, it will pass through and create the reladyn entry.
If this is copy relocation and it is not the first one - nothing shall be done here, it continues;
If this is not copy relocation it will change nothing and continue checking other relocation types.

ruiu added inline comments.Oct 28 2015, 9:22 AM
ELF/Writer.cpp
203–209

Then how about this?

if (auto *E = dyn_cast<SharedSymbol<ELFT>>(Body)) {
  if (E->NeedsCopy)
    continue;
  E->NeedsCopy = Target->relocNeedsCopy(Type, *Body);
}
grimar added inline comments.Oct 28 2015, 9:32 AM
ELF/Writer.cpp
203–209

That should work. I think I was need bool NeedsCopy variable few iterations ago and just forgot to remove it. Thats why such strange construction happened.

ruiu accepted this revision.Oct 28 2015, 9:35 AM
ruiu edited edge metadata.

Then LGTM with that change. Thank you for doing this!

This revision is now accepted and ready to land.Oct 28 2015, 9:35 AM
grimar closed this revision.Oct 28 2015, 9:54 AM

Revision: 251526