[PATCH] D91426: [PowerPC] Fix issue where binary uses a .got but is missing a .TOC.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Apologies for my late response.

In D91426#2651428 <https://reviews.llvm.org/D91426#2651428>, @stefanp wrote:

>> I'll need to go over this again to refresh my memory. As a quick recap though:
>>
>> - On PPC, if we create a .got, then the first entry must be the tocbase.
>> - If `.TOC.` is not defined by one of the input objects, then we do not increment the number of .got entries here <https://github.com/llvm/llvm-project/blob/3fd7d0d281a9b1dc7a8352cbd29178cbfacc73f1/lld/ELF/SyntheticSections.cpp#L653>
>> - If we synthetically define `.TOC.` in the linker, then we end up with a .got section in some cases where we would otherwise not need one.
>
> Yes, you got it!

Thanks:) I need to refresh my memory as well.

>> Is it possible to change the .got section to always add `gotHeaderEntriesNum` in its constructor for PPC64, and remove the section late if all it contains is the single entry and `.TOC.` is not defined?
>
> I see what you mean. An option might be to modify this function:
>
>   bool GotSection::isNeeded() const {
>     // We need to emit a GOT even if it's empty if there's a relocation that is
>     // relative to GOT(such as GOTOFFREL).
>     return numEntries || hasGotOffRel;
>   }
>
> On power PC we can say that we don't need the GOT if the number of entries is one and if the `.TOC.` is not defined. We now always add the GOT header but if we only have a GOT header at the end then we can say that we don't need the GOT. Does does that sound ok?
>
> I can try to rework the patch to have it do this.

Sounds good.

Eventually I think we may need to do something like if we want to have better compatibility with GNU ld. A rudimentary patch like the following needs test updates for ppc32/ppc64/riscv.
(I haven't thought enough about the GOT symbol.)

I can check whether your ppc64 specific patch will make it closer to the below ... and if so, make our code looks like that once your patch is committed:)

  diff --git i/lld/ELF/SyntheticSections.cpp w/lld/ELF/SyntheticSections.cpp
  index 9a875bd7ec3e..1f9a741b206a 100644
  --- i/lld/ELF/SyntheticSections.cpp
  +++ w/lld/ELF/SyntheticSections.cpp
  @@ -648,7 +648,3 @@ GotSection::GotSection()
                          ".got") {
  -  // If ElfSym::globalOffsetTable is relative to .got and is referenced,
  -  // increase numEntries by the number of entries used to emit
  -  // ElfSym::globalOffsetTable.
  -  if (ElfSym::globalOffsetTable && !target->gotBaseSymInGotPlt)
  -    numEntries += target->gotHeaderEntriesNum;
  +  numEntries += target->gotHeaderEntriesNum;
   }
  @@ -694,3 +690,3 @@ bool GotSection::isNeeded() const {
     // relative to GOT(such as GOTOFFREL).
  -  return numEntries || hasGotOffRel;
  +  return numEntries > target->gotHeaderEntriesNum || hasGotOffRel;
   }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91426/new/

https://reviews.llvm.org/D91426