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

[George Rimar via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/InputFiles.cpp:807-814
   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,
----------------
ruiu wrote:
> Why don't you pass `this` instead of `nullptr`?
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.


================
Comment at: ELF/Relocations.cpp:309-310
 static bool isStaticLinkTimeConstant(RelExpr E, uint32_t Type,
-                                     const SymbolBody &Body) {
+                                     const SymbolBody &Body,
+                                     InputSectionBase<ELFT> &S,
+                                     typename ELFT::uint RelOff) {
----------------
ruiu wrote:
> I don't think you need to add a new parameter because DefinedRegular has Section member.
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 ?


================
Comment at: ELF/Relocations.cpp:349
+          " cannot refer to absolute symbol '" + Body.getName() +
+          "', symbol defined here " + getFilename(Body.File));
     return true;
----------------
ruiu wrote:
> Please change the message "cannot refer to absolute symbol '" + Body.getName() + "' defined in "
Changed the part of message as suggested. 
I think you did not mean you want to remove "getLocation()..... + : relocatiion" part right ?


================
Comment at: ELF/Relocations.cpp:637
     uint32_t Type = RI.getType(Config->Mips64EL);
+    uintX_t Off = RI.r_offset;
 
----------------
ruiu wrote:
> Revert this change because this is not related.
Done.


https://reviews.llvm.org/D26508