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

[Hal Finkel via llvm-commits]

----- 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.

> 
> >>
> >> 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