[ELF] - Better diagnostic for relative relocation to an absolute value error.

[grimar]

Summary

Patch adds a filename to that error message.

I am facing this now when debugging one of FreeBSD port:
error: relocation R_X86_64_PLT32 cannot refer to absolute symbol __tls_get_addr

Body->File field is empty for next place:

if (AbsVal && RelE) {
  if (Body.isUndefined() && !Body.isLocal() && Body.symbol()->isWeak())
    return true;
  error("relocation " + getRelName(Type) +
        " cannot refer to absolute symbol " + Body.getName());
  return true;
}

It took some time to find the file and since global direction is to make linker more user friendly,
I suggest to add filename for that message.

[pcc]

Test case?

ELF/SymbolTable.h

63 ↗

(On Diff #77495)

Why is it necessary to add this parameter, can't you access the file via the section?

[ruiu]

What is the previous output, and how does it change with this patch? An example would be helpful to understand.

[emaste]

Also out of curiosity, which port produced this?

[evgeny777]

ELF/InputFiles.cpp

471 ↗

(On Diff #77495)

Why do you need to pass 'this' pointer if you can use Sec->getFile()

ELF/Relocations.cpp

345 ↗

(On Diff #77495)

I suggest printing getLocation(Rel.Section, Rel.Offset) as it seems to be more useful than location of absolute symbol. Also please add ": " after location.

[grimar]

In D26508#592221, @emaste wrote:

Also out of curiosity, which port produced this?

It is cad/iverilog (iverilog-10.1.1)

[grimar]

• Addressed review comments.

ELF/InputFiles.cpp

471 ↗

(On Diff #77495)

Because Sec can be nullptr, in case of absolute symbol.

ELF/Relocations.cpp

345 ↗

(On Diff #77495)

Done, printing both, thanks for hint.

ELF/SymbolTable.h

63 ↗

(On Diff #77495)

Section is null for absolute symbols.

[grimar]

In D26508#592123, @ruiu wrote:

What is the previous output, and how does it change with this patch? An example would be helpful to understand.

So previous output would be just something like "relocation R_X86_64_PLT32 cannot refer to absolute symbol answer",
Body->File was empty so it was not possible to print symbol location definition.

I added missing argument to replaceBody (that was an issue of first diff) and also implemented Eugene's
suggestion, now messages seems to be much more helpfull (please see testcase).

[grimar]

In D26508#592688, @grimar wrote:

In D26508#592221, @emaste wrote:

Also out of curiosity, which port produced this?

It is cad/iverilog (iverilog-10.1.1)

Just in case - opened an issue: https://llvm.org/bugs/show_bug.cgi?id=30984

[grimar]

• Rebased.
• Emit single error instead of 2 for a one place.

[ruiu]

ELF/InputFiles.cpp

807–814 ↗

(On Diff #77617)

Why don't you pass this instead of nullptr?

ELF/Relocations.cpp

309–310 ↗

(On Diff #77617)

I don't think you need to add a new parameter because DefinedRegular has Section member.

349 ↗

(On Diff #77617)

Please change the message "cannot refer to absolute symbol '" + Body.getName() + "' defined in "

637 ↗

(On Diff #77617)

Revert this change because this is not related.

ELF/SymbolTable.h

63 ↗

(On Diff #77495)

Actually, I hope we represent an absolute symbol in a different way than a null pointer so that we can eliminate the parameter. You don't need to do that in this patch though.

[grimar]

• Addressed review comments.

ELF/InputFiles.cpp

807–814 ↗

(On Diff #77617)

addRegular here calls the next code:

template <class ELFT>
std::pair<Symbol *, bool>
SymbolTable<ELFT>::insert(StringRef &Name, uint8_t Type, uint8_t Visibility,
                          bool CanOmitFromDynSym, InputFile *File) {
  bool IsUsedInRegularObj = !File || File->kind() == InputFile::ObjectKind;

If I would pass this, IsUsedInRegularObj then changes from true to false what breaks the logic and test.
I think the condition should be extended to check kind() for BinaryFile, but I also
believe it is out of area of this patch and should be done separatelly.

ELF/Relocations.cpp

309–310 ↗

(On Diff #77617)

Additional parameter helps not to assume that Body belongs to DefinedRegular.
Otherwise if I understood idea correctly we can end up with something like:

if (AbsVal && RelE) {
  if (Body.isUndefined() && !Body.isLocal() && Body.symbol()->isWeak())
    return true;

  auto *D = dyn_cast<DefinedRegular<ELFT>>(&Body);
  if (!D)
    fatal(getRelName(Type) + " cannot refer to absolute symbol '" +
          Body.getName() + "' defined in " + getFilename(Body.File));
  else
    error(getLocation(*D->Section, RelOff) + ": relocation " +
          getRelName(Type) + " cannot refer to absolute symbol '" +
          Body.getName() + "' defined in " + getFilename(Body.File));
  return true;
}

I think it is better not to assume that body is DefinedRegular here,
as we are trying to catch and print any possible error un a user-friendly way and also keep code simpler.
What do you think ?

349 ↗

(On Diff #77617)

Changed the part of message as suggested.
I think you did not mean you want to remove "getLocation()..... + : relocatiion" part right ?

637 ↗

(On Diff #77617)

Done.

[ruiu]

LGTM.

I still think that there's a way to reduce the number of parameters, but this patch is good enough to land.

lld/trunk/ELF/InputFiles.cpp

SymbolBody *elf::ObjectFile<ELFT>::createSymbolBody(const Elf_Sym *Sym) {
   case STB_WEAK:
   case STB_GNU_UNIQUE:
     if (Sec == &InputSection<ELFT>::Discarded)
       return elf::Symtab<ELFT>::X->addUndefined(Name, Binding, Sym->st_other,
                                                 Sym->getType(),
                                                 /*CanOmitFromDynSym*/ false,
                                                 this)
           ->body();
-    return elf::Symtab<ELFT>::X->addRegular(Name, *Sym, Sec)->body();
+    return elf::Symtab<ELFT>::X->addRegular(Name, *Sym, Sec, this)->body();
   }
 }
 
 template <class ELFT> void ArchiveFile::parse() {
   File = check(Archive::create(MB), "failed to parse archive");
 
   // Read the symbol table to construct Lazy objects.
   for (const Archive::Symbol &Sym : File->symbols())
template <class ELFT> void BinaryFile::parse() {
   StringRef EndName = Saver.save(Twine(Filename) + "_end");
   StringRef SizeName = Saver.save(Twine(Filename) + "_size");
 
   auto *Section =
       make<InputSection<ELFT>>(SHF_ALLOC, SHT_PROGBITS, 8, Data, ".data");
   Sections.push_back(Section);
 
   elf::Symtab<ELFT>::X->addRegular(StartName, STV_DEFAULT, STT_OBJECT, 0, 0,
-                                   STB_GLOBAL, Section);
+                                   STB_GLOBAL, Section, nullptr);
   elf::Symtab<ELFT>::X->addRegular(EndName, STV_DEFAULT, STT_OBJECT,
-                                   Data.size(), 0, STB_GLOBAL, Section);
+                                   Data.size(), 0, STB_GLOBAL, Section,
+                                   nullptr);
   elf::Symtab<ELFT>::X->addRegular(SizeName, STV_DEFAULT, STT_OBJECT,
-                                   Data.size(), 0, STB_GLOBAL, nullptr);
+                                   Data.size(), 0, STB_GLOBAL, nullptr,
+                                   nullptr);
 }
 
 static bool isBitcode(MemoryBufferRef MB) {
   using namespace sys::fs;
   return identify_magic(MB.getBuffer()) == file_magic::bitcode;
 }
 
 InputFile *elf::createObjectFile(MemoryBufferRef MB, StringRef ArchiveName,

lld/trunk/ELF/LinkerScript.cpp

 using namespace lld::elf;
 
 LinkerScriptBase *elf::ScriptBase;
 ScriptConfiguration *elf::ScriptConfig;
 
 template <class ELFT> static void addRegular(SymbolAssignment *Cmd) {
   uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
   Symbol *Sym = Symtab<ELFT>::X->addRegular(Cmd->Name, Visibility, STT_NOTYPE,
-                                            0, 0, STB_GLOBAL, nullptr);
+                                            0, 0, STB_GLOBAL, nullptr, nullptr);
   Cmd->Sym = Sym->body();
 
   // If we have no SECTIONS then we don't have '.' and don't call
   // assignAddresses(). We calculate symbol value immediately in this case.
   if (!ScriptConfig->HasSections)
     cast<DefinedRegular<ELFT>>(Cmd->Sym)->Value = Cmd->Expression(0);
 }
 

lld/trunk/ELF/Relocations.cpp

 static bool isRelExpr(RelExpr Expr) {
   return Expr == R_PC || Expr == R_GOTREL || Expr == R_GOTREL_FROM_END ||
          Expr == R_PAGE_PC || Expr == R_RELAX_GOT_PC || Expr == R_THUNK_PC ||
          Expr == R_THUNK_PLT_PC;
 }
 
 template <class ELFT>
 static bool isStaticLinkTimeConstant(RelExpr E, uint32_t Type,
-                                     const SymbolBody &Body) {
+                                     const SymbolBody &Body,
+                                     InputSectionBase<ELFT> &S,
+                                     typename ELFT::uint RelOff) {
   // These expressions always compute a constant
   if (E == R_SIZE || E == R_GOT_FROM_END || E == R_GOT_OFF ||
       E == R_MIPS_GOT_LOCAL_PAGE || E == R_MIPS_GOT_OFF ||
       E == R_MIPS_GOT_OFF32 || E == R_MIPS_TLSGD || E == R_GOT_PAGE_PC ||
       E == R_GOT_PC || E == R_PLT_PC || E == R_TLSGD_PC || E == R_TLSGD ||
       E == R_PPC_PLT_OPD || E == R_TLSDESC_CALL || E == R_TLSDESC_PAGE ||
       E == R_HINT || E == R_THUNK_PC || E == R_THUNK_PLT_PC)
     return true;
static bool isStaticLinkTimeConstant(RelExpr E, uint32_t Type,
   // Relative relocation to an absolute value. This is normally unrepresentable,
   // but if the relocation refers to a weak undefined symbol, we allow it to
   // resolve to the image base. This is a little strange, but it allows us to
   // link function calls to such symbols. Normally such a call will be guarded
   // with a comparison, which will load a zero from the GOT.
   if (AbsVal && RelE) {
     if (Body.isUndefined() && !Body.isLocal() && Body.symbol()->isWeak())
       return true;
-    error("relocation " + getRelName(Type) +
-          " cannot refer to absolute symbol " + Body.getName());
+    error(getLocation(S, RelOff) + ": relocation " + getRelName(Type) +
+          " cannot refer to absolute symbol '" + Body.getName() +
+          "' defined in " + getFilename(Body.File));
     return true;
   }
 
   return Target->usesOnlyLowPageBits(Type);
 }
 
 static RelExpr toPlt(RelExpr Expr) {
   if (Expr == R_PPC_OPD)
template <class ELFT> static void addCopyRelSymbol(SharedSymbol<ELFT> *SS) {
   }
   Out<ELFT>::RelaDyn->addReloc(
       {Target->CopyRel, Out<ELFT>::Bss, SS->OffsetInBss, false, SS, 0});
 }
 
 template <class ELFT>
 static RelExpr adjustExpr(const elf::ObjectFile<ELFT> &File, SymbolBody &Body,
                           bool IsWrite, RelExpr Expr, uint32_t Type,
-                          const uint8_t *Data) {
+                          const uint8_t *Data, InputSectionBase<ELFT> &S,
+                          typename ELFT::uint RelOff) {
   bool Preemptible = isPreemptible(Body, Type);
   if (Body.isGnuIFunc()) {
     Expr = toPlt(Expr);
   } else if (!Preemptible) {
     if (needsPlt(Expr))
       Expr = fromPlt(Expr);
     if (Expr == R_GOT_PC && !isAbsoluteValue<ELFT>(Body))
       Expr = Target->adjustRelaxExpr(Type, Data, Expr);
   }
   Expr = Target->getThunkExpr(Expr, Type, File, Body);
 
-  if (IsWrite || isStaticLinkTimeConstant<ELFT>(Expr, Type, Body))
+  if (IsWrite || isStaticLinkTimeConstant<ELFT>(Expr, Type, Body, S, RelOff))
     return Expr;
 
   // This relocation would require the dynamic linker to write a value to read
   // only memory. We can hack around it if we are producing an executable and
   // the refered symbol can be preemepted to refer to the executable.
   if (Config->Shared || (Config->Pic && !isRelExpr(Expr))) {
     StringRef Name = getSymbolName(File.getStringTable(), Body);
     error("can't create dynamic relocation " + getRelName(Type) + " against " +
for (auto I = Rels.begin(), E = Rels.end(); I != E; ++I) {
 
     // We only report undefined symbols if they are referenced somewhere in the
     // code.
     if (!Body.isLocal() && Body.isUndefined() && !Body.symbol()->isWeak())
       reportUndefined(Body, C, RI.r_offset);
 
     RelExpr Expr = Target->getRelExpr(Type, Body);
     bool Preemptible = isPreemptible(Body, Type);
-    Expr = adjustExpr(*File, Body, IsWrite, Expr, Type, Buf + RI.r_offset);
+    Expr = adjustExpr(*File, Body, IsWrite, Expr, Type, Buf + RI.r_offset, C,
+                      RI.r_offset);
     if (HasError)
       continue;
 
     // Skip a relocation that points to a dead piece
     // in a eh_frame section.
     while (PieceI != PieceE &&
            (PieceI->InputOff + PieceI->size() <= RI.r_offset))
       ++PieceI;
for (auto I = Rels.begin(), E = Rels.end(); I != E; ++I) {
     // only markers for relaxation.
     if (Expr == R_HINT || Expr == R_TLSDESC_CALL)
       continue;
 
     if (needsPlt(Expr) || Expr == R_THUNK_ABS || Expr == R_THUNK_PC ||
         Expr == R_THUNK_PLT_PC || refersToGotEntry(Expr) ||
         !isPreemptible(Body, Type)) {
       // If the relocation points to something in the file, we can process it.
-      bool Constant = isStaticLinkTimeConstant<ELFT>(Expr, Type, Body);
+      bool Constant =
+          isStaticLinkTimeConstant<ELFT>(Expr, Type, Body, C, RI.r_offset);
 
       // If the output being produced is position independent, the final value
       // is still not known. In that case we still need some help from the
       // dynamic linker. We can however do better than just copying the incoming
       // relocation. We can process some of it and and just ask the dynamic
       // linker to add the load address.
       if (!Constant)
         AddDyn({Target->RelativeRel, &C, Offset, true, &Body, Addend});

lld/trunk/ELF/SymbolTable.h

DefinedRegular<ELFT> *addIgnored(StringRef Name,
                                    uint8_t Visibility = llvm::ELF::STV_HIDDEN);
 
   Symbol *addUndefined(StringRef Name);
   Symbol *addUndefined(StringRef Name, uint8_t Binding, uint8_t StOther,
                        uint8_t Type, bool CanOmitFromDynSym, InputFile *File);
 
   Symbol *addRegular(StringRef Name, uint8_t StOther, uint8_t Type,
                      uintX_t Value, uintX_t Size, uint8_t Binding,
-                     InputSectionBase<ELFT> *Section);
+                     InputSectionBase<ELFT> *Section, InputFile *File);
   Symbol *addRegular(StringRef Name, const Elf_Sym &Sym,
-                     InputSectionBase<ELFT> *Section);
+                     InputSectionBase<ELFT> *Section, InputFile *File);
 
   Symbol *addSynthetic(StringRef N, OutputSectionBase *Section, uintX_t Value,
                        uint8_t StOther);
 
   void addShared(SharedFile<ELFT> *F, StringRef Name, const Elf_Sym &Sym,
                  const typename ELFT::Verdef *Verdef);
 
   void addLazyArchive(ArchiveFile *F, const llvm::object::Archive::Symbol S);

lld/trunk/ELF/SymbolTable.cpp

for (InputFile *File : Lto->compile()) {
     Obj->parse(DummyGroups);
     ObjectFiles.push_back(Obj);
   }
 }
 
 template <class ELFT>
 DefinedRegular<ELFT> *SymbolTable<ELFT>::addAbsolute(StringRef Name,
                                                      uint8_t Visibility) {
-  Symbol *Sym =
-      addRegular(Name, Visibility, STT_NOTYPE, 0, 0, STB_GLOBAL, nullptr);
+  Symbol *Sym = addRegular(Name, Visibility, STT_NOTYPE, 0, 0, STB_GLOBAL,
+                           nullptr, nullptr);
   return cast<DefinedRegular<ELFT>>(Sym->body());
 }
 
 // Add Name as an "ignored" symbol. An ignored symbol is a regular
 // linker-synthesized defined symbol, but is only defined if needed.
 template <class ELFT>
 DefinedRegular<ELFT> *SymbolTable<ELFT>::addIgnored(StringRef Name,
                                                     uint8_t Visibility) {
static void reportDuplicate(SymbolBody *Existing,
 
   print(NewLoc + ": duplicate symbol '" + maybeDemangle(Existing->getName()) +
         "'");
   print(OldLoc + ": previous definition was here");
 }
 
 template <typename ELFT>
 Symbol *SymbolTable<ELFT>::addRegular(StringRef Name, const Elf_Sym &Sym,
-                                      InputSectionBase<ELFT> *Section) {
+                                      InputSectionBase<ELFT> *Section,
+                                      InputFile *File) {
   return addRegular(Name, Sym.st_other, Sym.getType(), Sym.st_value,
-                    Sym.st_size, Sym.getBinding(), Section);
+                    Sym.st_size, Sym.getBinding(), Section, File);
 }
 
 template <typename ELFT>
 Symbol *SymbolTable<ELFT>::addRegular(StringRef Name, uint8_t StOther,
                                       uint8_t Type, uintX_t Value, uintX_t Size,
                                       uint8_t Binding,
-                                      InputSectionBase<ELFT> *Section) {
+                                      InputSectionBase<ELFT> *Section,
+                                      InputFile *File) {
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name, Type, StOther & 3,
-                                    /*CanOmitFromDynSym*/ false,
-                                    Section ? Section->getFile() : nullptr);
+                                    /*CanOmitFromDynSym*/ false, File);
   int Cmp = compareDefinedNonCommon(S, WasInserted, Binding);
   if (Cmp > 0)
     replaceBody<DefinedRegular<ELFT>>(S, Name, StOther, Type, Value, Size,
-                                      Section);
+                                      Section, File);
   else if (Cmp == 0)
     reportDuplicate(S->body(), Section, Value);
   return S;
 }
 
 template <typename ELFT>
 Symbol *SymbolTable<ELFT>::addSynthetic(StringRef N, OutputSectionBase *Section,
                                         uintX_t Value, uint8_t StOther) {

lld/trunk/ELF/Symbols.h

public:
   DefinedRegular(StringRef Name, uint8_t StOther, uint8_t Type, uintX_t Value,
                  uintX_t Size, InputSectionBase<ELFT> *Section, InputFile *File)
       : Defined(SymbolBody::DefinedRegularKind, Name, StOther, Type),
         Value(Value), Size(Size),
         Section(Section ? Section->Repl : NullInputSection) {
     this->File = File;
   }
 
-  DefinedRegular(StringRef Name, uint8_t StOther, uint8_t Type, uintX_t Value,
-                 uintX_t Size, InputSectionBase<ELFT> *Section)
-      : DefinedRegular(Name, StOther, Type, Value, Size, Section,
-                       Section ? Section->getFile() : nullptr) {}
-
   DefinedRegular(StringRef Name, uint8_t StOther, uint8_t Type, BitcodeFile *F)
       : DefinedRegular(Name, StOther, Type, 0, 0, NullInputSection, F) {}
 
   DefinedRegular(const Elf_Sym &Sym, InputSectionBase<ELFT> *Section)
       : Defined(SymbolBody::DefinedRegularKind, Sym.st_name, Sym.st_other,
                 Sym.getType()),
         Value(Sym.st_value), Size(Sym.st_size),
         Section(Section ? Section->Repl : NullInputSection) {

lld/trunk/ELF/Writer.cpp

 }
 
 template <class ELFT>
 static Symbol *addRegular(StringRef Name, InputSectionBase<ELFT> *IS,
                           typename ELFT::uint Value) {
   typename ELFT::Sym LocalHidden = {};
   LocalHidden.setBindingAndType(STB_LOCAL, STT_NOTYPE);
   LocalHidden.setVisibility(STV_HIDDEN);
-  Symbol *S = Symtab<ELFT>::X->addRegular(Name, LocalHidden, IS);
+  Symbol *S = Symtab<ELFT>::X->addRegular(Name, LocalHidden, IS, nullptr);
   cast<DefinedRegular<ELFT>>(S->body())->Value = Value;
   return S;
 }
 
 template <class ELFT>
 static Symbol *addOptionalRegular(StringRef Name, InputSectionBase<ELFT> *IS,
                                   typename ELFT::uint Value) {
   SymbolBody *S = Symtab<ELFT>::X->find(Name);

lld/trunk/test/ELF/Inputs/relocation-relative-absolute.s

+.globl answer
+answer = 42

lld/trunk/test/ELF/relocation-relative-absolute.s

 # REQUIRES: x86
-# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
-# RUN: not ld.lld %t.o -o %t -pie 2>&1 | FileCheck %s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %tinput1.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux \
+# RUN:   %S/Inputs/relocation-relative-absolute.s -o %tinput2.o
+# RUN: not ld.lld %tinput1.o %tinput2.o -o %t -pie 2>&1 | FileCheck %s
 
 .globl _start
 _start:
 
-# CHECK: relocation R_X86_64_PLT32 cannot refer to absolute symbol answer
-call answer@PLT
+# CHECK: {{.*}}input1.o (.text+0x1): relocation R_X86_64_PLT32 cannot refer to absolute symbol 'answer' defined in {{.*}}input2.o
 
-.globl answer
-answer = 42
+call answer@PLT