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

Wed Oct 14 06:40:15 PDT 2015

Awesome, thanks agin!

On 14 October 2015 at 08:30, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- 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