[lld] r250101 - [ELF2] Add a base set of PPC64 relocations

[Hal Finkel via llvm-commits]

----- Original Message -----
> From: "Hal Finkel via llvm-commits" <llvm-commits at lists.llvm.org>
> To: "Rafael Espíndola" <rafael.espindola at gmail.com>
> Cc: "llvm-commits" <llvm-commits at lists.llvm.org>
> Sent: Tuesday, October 13, 2015 5:38:51 PM
> Subject: Re: [lld] r250101 - [ELF2] Add a base set of PPC64 relocations
> 
> ----- Original Message -----
> > From: "Rafael Espíndola" <rafael.espindola at gmail.com>
> > To: "Hal Finkel" <hfinkel at anl.gov>
> > Cc: "llvm-commits" <llvm-commits at lists.llvm.org>
> > Sent: Tuesday, October 13, 2015 2:02:44 PM
> > Subject: Re: [lld] r250101 - [ELF2] Add a base set of PPC64
> > relocations
> > 
> > > I imagine this is true currently because we create both sections
> > > together. Does anything prevent a .got from existing in an input
> > > file? When we support the lazy-binding .bss-style .plt sections,
> > > we might not have a .got (that's my current understanding).
> > 
> > We would have a .got.plt section. Not sure how that interact with
> > the
> > ppc64 abi (is it part of the TOC?).
> 
> Lazy binding on PPC64 does not work the same way as it does on x86.
> On PPC64, lazy binding works by declaring a NOBITS .plt section, and
> ld.so actually generates the entire stub. You don't necessarily need
> any .got entries at all.
> 
> The full logic should be that the TOC base is computed from the
> starting address of .got, .toc, .tocbss, .plt (in that order,
> whichever is first in that list and exists). At least, that's what
> it says in RuntimeDyldELF.cpp.
> 
> > 
> > >> Is this here for the same reason as X86_64 and i386? If so, you
> > >> only
> > >> need the S.isShared(), no?
> > >
> > > No, I added this specifically because it came up, even in my
> > > hello-world test case (because there was some symbol,
> > > __gmon_start__ as I recall, that has weak linkage and needs a
> > > .plt
> > > stub). Maybe I'm just not understanding what is going on, but
> > > what
> > > would locally resolving such an undefined symbol mean?
> > 
> > But how is a weak undefined making a difference in here? It should
> > resolve to 0 anyway.
> > 
> > It is also present on x86_64:
> > 
> > $ nm
> > /usr/lib/gcc/x86_64-redhat-linux/4.9.2/../../../../lib64/crti.o
> > | grep gmon
> >                  w __gmon_start__
> > 
> > 
> > In any case, even if it is needed for some reason, it is missing a
> > testcase for that reason. All tests pass if I convert the line to
> > just
> > "return S.isShared();".
> 
> It is missing a test case, but this brings up a question: Why does
> the x86 logic do the right thing? It seems that lld resolves the
> weak symbol reference to 0 during linking, but is that right? Can't
> it still be provided by an LD_PRELOAD? When I link a program with
> ld.bfd on PPC64, it leaves a dynamic binding and stub to
> __gmon_start__ in the final executable.

FWIW, the x86_64 behavior here seems similar:

$ cat /tmp/hello.cpp 
#include <iostream>

int main() {
  std::cout << "hello\n";
}

$ clang++ -fuse-ld=lld2 -o /tmp/h -O3 /tmp/hello.cpp
$ clang++ -o /tmp/h-ld -O3 /tmp/hello.cpp

$ objdump -R /tmp/h | grep gmon
  (empty)
$ objdump -R /tmp/h-ld | grep gmon
0000000000600ae0 R_X86_64_GLOB_DAT  __gmon_start__

Presumably a separate problem, the resulting /tmp/h does not run (I'll file a bug report).

Thanks again,
Hal 

> 
> > 
> > >>
> > >> git-clang-format.
> > >
> > > No, thank you.
> > 
> > Please do. It is just not worth it having to worry about which
> > parts
> > are handled by clang-format and which ones are not.
> > 
> > If you particularly dislike the output of clang-format for a
> > particular code, please open a bug about it.
> 
> No, this is not reasonable. clang-format is a tool, but nowhere in
> LLVM is it a requirement, and it should not be here. Regardless of
> clang-format's abilities, this code:
> 
>   switch (Type) {
>   case R_PPC64_TOC16:       Type = R_PPC64_ADDR16;       A -= TB;
>   break;
>   case R_PPC64_TOC16_DS:    Type = R_PPC64_ADDR16_DS;    A -= TB;
>   break;
>   case R_PPC64_TOC16_LO:    Type = R_PPC64_ADDR16_LO;    A -= TB;
>   break;
>   case R_PPC64_TOC16_LO_DS: Type = R_PPC64_ADDR16_LO_DS; A -= TB;
>   break;
>   case R_PPC64_TOC16_HI:    Type = R_PPC64_ADDR16_HI;    A -= TB;
>   break;
>   case R_PPC64_TOC16_HA:    Type = R_PPC64_ADDR16_HA;    A -= TB;
>   break;
>   default: break;
>   }
> 
> should remain as-is. The special alignment here is the only thing
> that makes repeating the 'A -= TB' reasonable. Is these are not all
> aligned like that, it would be too easy to miss one that was not
> like the others.
> 
> To be clear, in the original patch, I had the code like this:
> 
> if (Type == T1 ||
>     Type == T2 ||
>     ...) {
>   switch (Type) {
>   case T1: Type = Q1; break;
>   case T2: Type = Q2; break;
>   ...
>   default: llvm_unreachable(...);
>   }
> 
>   A -= TB;
> }
> 
> which is how the corresponding code in
> lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp is written.
> 
> Rui said that looked weird to him, and requested that I remove the
> enclosing if and repeat the addend adjustment. After making the
> variable names short enough, this could be reasonably done. The
> result is what we have now. There are many ways we can write this
> logic, but without the current formatting, this current way is not
> acceptable.
> 
> Thanks again,
> Hal
> 
> > 
> > Cheers,
> > Rafael
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory