[ELF] - R_386_GOTOFF relocation implemented.

[grimar]

Summary

R_386_GOTOFF is calculated as S + A - GOT, where:
S - Represents the value of the symbol whose index resides in the relocation entry.
A - Represents the addend used to compute the value of the relocatable field.
GOT - Represents the address of the global offset table.

This relocation needs got address.
Problem here was that got table was not created if had no entries. Had to change how GOT gets to the output, test covers that case.

[ruiu]

ELF/OutputSections.h

140 ↗

(On Diff #42311)

Maybe we should have a separate global variable, HasGotoffRel, instead of this field.

ELF/Target.cpp

328–334 ↗

(On Diff #42311)

I don't think this is the right place to write this code. This function is doing more than its name implies.

[grimar]

Review comments addressed.

ELF/OutputSections.h

140 ↗

(On Diff #42311)

I think HasGotOffRel is too specific, at least it can be used for mips as shown in this patch:

Out<ELFT>::Got->Demanded |=
    (isOutputDynamic() && Config->EMachine == EM_MIPS);

I can make it separate static but what the point for it ? It only should be used to keep got section now, but when separate static it looks like it can be used for something else, what is not true.

ELF/Target.cpp

328–334 ↗

(On Diff #42311)

Yes, I didn`t like that place too, but then only suitable place for me would be adding Target->relocUsesGotAddr() and call it during iterations over relocations at upper level.
That adds one more method to Target thats only useful only for x86 now. Made that change.

[ruiu]

ELF/OutputSections.h

140 ↗

(On Diff #42397)

Well, I think that is rather too specific. You set Demanded field in Writer.cpp, but that boolean value is consumed immediately in the following line in an obscure way. (You explicitly wrote to the boolean variable but read from it through leaveInOutput() function.)

Can we write like this in Writer.cpp?

bool needsGot = (isOutputDynamic() && Config->EMachine == EM_MIPS);

// If we have a relocation that is relative to GOT (such as GOTOFFREL),
// we need to emit a GOT even if it's empty.
if (HasGotoffRel)
  needsGot = true;

if (needsGotMips || !Out<ELFT>::Got->empty()) {
  ....
}

ELF/Target.h

62 ↗

(On Diff #42397)

I'd name relocNeedsGot.

[grimar]

Review comments addressed.

ELF/OutputSections.h

140 ↗

(On Diff #42397)

Done.
(I actually don`t like idea of global variable at all. I would prefer static member of GotSection then. But if you insist..)

ELF/Target.h

62 ↗

(On Diff #42397)

I`d too, but unfortunately we already have one bool relocNeedsGot(...) above :)
Which is used to determine if relocation needs got entry to be allocated. So I dont think you want to move functionality from relocUsesGotAddr there.
May be relocAddressesGot() ?

[grimar]

• Rebased.

[ruiu]

LGTM with a few nits.

ELF/OutputSections.cpp

24–28 ↗

(On Diff #43111)

bool lld::elf2::HasGotOffRel = false;

ELF/OutputSections.h

39 ↗

(On Diff #43111)

got -> GOT

ELF/Target.cpp

364 ↗

(On Diff #43111)

I'd name this isGotRelative.

ELF/Writer.cpp

771–779 ↗

(On Diff #43111)

I think this flow is slightly better in readability (because special cases are now in ifs.)

bool needsGot = !Out<ELFT>::Got->empty();
// We add the .got section to the result for dynamic MIPS target because
// its address and properties are mentioned in the .dynamic section.
if (Config->EMachine == EM_MIPS)
  needsGot = needsGot || isOutputDynamic();
// If we have a relocation that is relative to GOT (such as GOTOFFREL),
// we need to emit a GOT even if it's empty.
if (HasGotOffRel)
  needsGot = true;

[grimar]

ELF/Writer.cpp

771–779 ↗

(On Diff #43111)

Ok.
The only distinction - I used |= :

if (Config->EMachine == EM_MIPS)
  needsGot |= isOutputDynamic();

[grimar]

lld/trunk/ELF/Writer.cpp

214

btw, what about making this more generic ?

1. Change HasGotOffRel to be not a global variable but a public member of target.
2. remove isGotRelative(Type), create visitRelocation(Type) or something alike instead. Which purpose would be to review relocation and set flags like HasGotOffRel inside.

lld/trunk/ELF/OutputSections.h

 template <class ELFT> class InputSectionBase;
 template <class ELFT> class MergeInputSection;
 template <class ELFT> class MipsReginfoInputSection;
 template <class ELFT> class OutputSection;
 template <class ELFT> class ObjectFile;
 template <class ELFT> class DefinedRegular;
 template <class ELFT> class ELFSymbolBody;
 
+// Flag to force GOT to be in output if we have relocations
+// that relies on its address.
+extern bool HasGotOffRel;
+
 template <class ELFT>
 static inline typename llvm::object::ELFFile<ELFT>::uintX_t
 getAddend(const typename llvm::object::ELFFile<ELFT>::Elf_Rel &Rel) {
   return 0;
 }
 
 template <class ELFT>
 static inline typename llvm::object::ELFFile<ELFT>::uintX_t

lld/trunk/ELF/OutputSections.cpp

 using namespace llvm;
 using namespace llvm::object;
 using namespace llvm::support::endian;
 using namespace llvm::ELF;
 
 using namespace lld;
 using namespace lld::elf2;
 
+bool lld::elf2::HasGotOffRel = false;
+
 template <class ELFT>
 OutputSectionBase<ELFT>::OutputSectionBase(StringRef Name, uint32_t sh_type,
                                            uintX_t sh_flags)
     : Name(Name) {
   memset(&Header, 0, sizeof(Elf_Shdr));
   Header.sh_type = sh_type;
   Header.sh_flags = sh_flags;
 }

lld/trunk/ELF/Target.h

public:
   virtual bool isRelRelative(uint32_t Type) const;
   virtual bool isSizeDynReloc(uint32_t Type, const SymbolBody &S) const;
   virtual bool relocNeedsDynRelative(unsigned Type) const { return false; }
   virtual bool relocNeedsGot(uint32_t Type, const SymbolBody &S) const = 0;
   virtual bool relocNeedsPlt(uint32_t Type, const SymbolBody &S) const = 0;
   virtual void relocateOne(uint8_t *Loc, uint8_t *BufEnd, uint32_t Type,
                            uint64_t P, uint64_t SA, uint64_t ZA = 0,
                            uint8_t *PairedLoc = nullptr) const = 0;
+  virtual bool isGotRelative(uint32_t Type) const;
   virtual bool isTlsOptimized(unsigned Type, const SymbolBody *S) const;
   virtual bool needsCopyRel(uint32_t Type, const SymbolBody &S) const;
   virtual unsigned relocateTlsOptimize(uint8_t *Loc, uint8_t *BufEnd,
                                        uint32_t Type, uint64_t P, uint64_t SA,
                                        const SymbolBody &S) const;
   virtual ~TargetInfo();
 
 protected:

lld/trunk/ELF/Target.cpp

public:
   bool relocNeedsPlt(uint32_t Type, const SymbolBody &S) const override;
   void relocateOne(uint8_t *Loc, uint8_t *BufEnd, uint32_t Type, uint64_t P,
                    uint64_t SA, uint64_t ZA = 0,
                    uint8_t *PairedLoc = nullptr) const override;
   bool isTlsOptimized(unsigned Type, const SymbolBody *S) const override;
   unsigned relocateTlsOptimize(uint8_t *Loc, uint8_t *BufEnd, uint32_t Type,
                                uint64_t P, uint64_t SA,
                                const SymbolBody &S) const override;
+  bool isGotRelative(uint32_t Type) const override;
 
 private:
   void relocateTlsLdToLe(uint8_t *Loc, uint8_t *BufEnd, uint64_t P,
                          uint64_t SA) const;
   void relocateTlsGdToIe(uint8_t *Loc, uint8_t *BufEnd, uint64_t P,
                          uint64_t SA) const;
   void relocateTlsGdToLe(uint8_t *Loc, uint8_t *BufEnd, uint64_t P,
                          uint64_t SA) const;

 }
 
 uint64_t TargetInfo::getVAStart() const { return Config->Shared ? 0 : VAStart; }
 
 bool TargetInfo::needsCopyRel(uint32_t Type, const SymbolBody &S) const {
   return false;
 }
 
+bool TargetInfo::isGotRelative(uint32_t Type) const { return false; }
+
 unsigned TargetInfo::getPltRefReloc(unsigned Type) const { return PCRelReloc; }
 
 bool TargetInfo::isRelRelative(uint32_t Type) const { return true; }
 
 bool TargetInfo::isSizeDynReloc(uint32_t Type, const SymbolBody &S) const {
   return false;
 }
 
bool X86TargetInfo::relocNeedsGot(uint32_t Type, const SymbolBody &S) const {
   return Type == R_386_GOT32 || relocNeedsPlt(Type, S);
 }
 
 bool X86TargetInfo::relocNeedsPlt(uint32_t Type, const SymbolBody &S) const {
   return (Type == R_386_PLT32 && canBePreempted(&S, true)) ||
          (Type == R_386_PC32 && S.isShared());
 }
 
+bool X86TargetInfo::isGotRelative(uint32_t Type) const {
+  // This relocation does not require got entry,
+  // but it is relative to got and needs it to be created.
+  // Here we request for that.
+  return Type == R_386_GOTOFF;
+}
+
 void X86TargetInfo::relocateOne(uint8_t *Loc, uint8_t *BufEnd, uint32_t Type,
                                 uint64_t P, uint64_t SA, uint64_t ZA,
                                 uint8_t *PairedLoc) const {
   switch (Type) {
   case R_386_32:
     add32le(Loc, SA);
     break;
   case R_386_GOT32:
+  case R_386_GOTOFF:
     add32le(Loc, SA - Out<ELF32LE>::Got->getVA());
     break;
   case R_386_GOTPC:
     add32le(Loc, SA + Out<ELF32LE>::Got->getVA() - P);
     break;
   case R_386_PC32:
   case R_386_PLT32:
     add32le(Loc, SA - P);

lld/trunk/ELF/Writer.cpp

void Writer<ELFT>::scanRelocs(
     iterator_range<const Elf_Rel_Impl<ELFT, isRela> *> Rels) {
   typedef Elf_Rel_Impl<ELFT, isRela> RelType;
   const ObjectFile<ELFT> &File = *C.getFile();
   for (const RelType &RI : Rels) {
     uint32_t SymIndex = RI.getSymbol(Config->Mips64EL);
     SymbolBody *Body = File.getSymbolBody(SymIndex);
     uint32_t Type = RI.getType(Config->Mips64EL);
 
+    if (Target->isGotRelative(Type))
+      HasGotOffRel = true;
+
     if (Target->isTlsLocalDynamicReloc(Type)) {
       if (Target->isTlsOptimized(Type, nullptr))
         continue;
       if (Out<ELFT>::Got->addCurrentModuleTlsIndex())
         Out<ELFT>::RelaDyn->addReloc({&C, &RI});
       continue;
     }
 
if (Config->EMachine == EM_MIPS && !Config->Shared) {
       Out<ELFT>::MipsRldMap = new (SecAlloc.Allocate())
           OutputSection<ELFT>(".rld_map", SHT_PROGBITS, SHF_ALLOC | SHF_WRITE);
       Out<ELFT>::MipsRldMap->setSize(ELFT::Is64Bits ? 8 : 4);
       Out<ELFT>::MipsRldMap->updateAlign(ELFT::Is64Bits ? 8 : 4);
       OutputSections.push_back(Out<ELFT>::MipsRldMap);
     }
   }
 
+  bool needsGot = !Out<ELFT>::Got->empty();
   // We add the .got section to the result for dynamic MIPS target because
   // its address and properties are mentioned in the .dynamic section.
-  if (!Out<ELFT>::Got->empty() ||
-      (isOutputDynamic() && Config->EMachine == EM_MIPS))
+  if (Config->EMachine == EM_MIPS)
+    needsGot |= isOutputDynamic();
+  // If we have a relocation that is relative to GOT (such as GOTOFFREL),
+  // we need to emit a GOT even if it's empty.
+  if (HasGotOffRel)
+    needsGot = true;
+
+  if (needsGot)
     OutputSections.push_back(Out<ELFT>::Got);
   if (Out<ELFT>::GotPlt && !Out<ELFT>::GotPlt->empty())
     OutputSections.push_back(Out<ELFT>::GotPlt);
   if (!Out<ELFT>::Plt->empty())
     OutputSections.push_back(Out<ELFT>::Plt);
 
   std::stable_sort(OutputSections.begin(), OutputSections.end(),
                    compareSections<ELFT>);

lld/trunk/test/ELF/got-i386.s

+// RUN: llvm-mc -filetype=obj -triple=i686-unknown-linux %s -o %t.o
+// RUN: ld.lld %t.o -o %t
+// RUN: llvm-readobj -s -r -t %t | FileCheck %s
+// RUN: llvm-objdump -d %t | FileCheck --check-prefix=DISASM %s
+// REQUIRES: x86
+
+// CHECK:      Name: .got
+// CHECK-NEXT: Type: SHT_PROGBITS
+// CHECK-NEXT: Flags [
+// CHECK-NEXT:   SHF_ALLOC
+// CHECK-NEXT:   SHF_WRITE
+// CHECK-NEXT: ]
+// CHECK-NEXT: Address: 0x12000
+// CHECK-NEXT: Offset:
+// CHECK-NEXT: Size: 0
+// CHECK-NEXT: Link:
+// CHECK-NEXT: Info:
+// CHECK-NEXT: AddressAlignment:
+
+// CHECK:      Symbol {
+// CHECK:       Name: bar
+// CHECK-NEXT:  Value: 0x12000
+// CHECK-NEXT:  Size: 10
+// CHECK-NEXT:  Binding: Global
+// CHECK-NEXT:  Type: Object
+// CHECK-NEXT:  Other: 0
+// CHECK-NEXT:  Section: .bss
+// CHECK-NEXT: }
+// CHECK-NEXT: Symbol {
+// CHECK-NEXT:  Name: obj
+// CHECK-NEXT:  Value: 0x1200A
+// CHECK-NEXT:  Size: 10
+// CHECK-NEXT:  Binding: Global
+// CHECK-NEXT:  Type: Object
+// CHECK-NEXT:  Other: 0
+// CHECK-NEXT:  Section: .bss
+// CHECK-NEXT: }
+
+// 0x12000 - 0 = addr(.got) = 0x12000
+// 0x1200A - 10 = addr(.got) = 0x12000
+// 0x1200A + 5 - 15 = addr(.got) = 0x12000
+// DISASM:      Disassembly of section .text:
+// DISASM-NEXT: _start:
+// DISASM-NEXT: 11000: c7 81 00 00 00 00 01 00 00 00 movl $1, (%ecx)
+// DISASM-NEXT: 1100a: c7 81 0a 00 00 00 02 00 00 00 movl $2, 10(%ecx)
+// DISASM-NEXT: 11014: c7 81 0f 00 00 00 03 00 00 00 movl $3, 15(%ecx)
+
+.global _start
+_start:
+  movl $1, bar@GOTOFF(%ecx)
+  movl $2, obj@GOTOFF(%ecx)
+  movl $3, obj+5@GOTOFF(%ecx)
+  .type bar, @object
+  .comm bar, 10
+  .type obj, @object
+  .comm obj, 10