[PATCH] D81784: [ELF] Resolve relocations in .debug_* referencing (discarded symbols or ICF folded section symbols) to tombstone values

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/InputSection.cpp:935
+                              isDebugLocOrRanges ? UINT64_MAX - 1 : UINT64_MAX);
+        continue;
+      }
----------------
grimar wrote:
> It looks a bit unideal to me, because `auto *ds = dyn_cast<Defined>(&sym)` is always called,
> though it is not always needed.
> The same happens with `isDebugLocOrRanges ? UINT64_MAX - 1 : UINT64_MAX` condition, which 
> can be caluclated once.
> Also `isDebugSection()` just do an excessive job it seems, because it checks the ".debug_" prefix.
> And `sym.getOutputSection()` checks that `sym` is Defined internally, so this check is done twice too.
> 
> What about doing the following?
> 
> 
> ```
> template <class ELFT, class RelTy>
> void InputSection::relocateNonAlloc(uint8_t *buf, ArrayRef<RelTy> rels) {
>   const unsigned bits = sizeof(typename ELFT::uint) * 8;
>   const bool isDebug = isDebugSection(*this);
>   const uint64_t Tombstone = (name == ".debug_loc" || name == ".debug_ranges")
>                                  ? (UINT64_MAX - 1)
>                                  : UINT64_MAX;
>   ....
> 
>     if (sym.isTls() && !Out::tlsPhdr) {
>       target->relocateNoSym(bufLoc, type, 0);
>       continue;
>     }
> 
>     if (!isDebug || type != target->symbolicRel) {
>       target->relocateNoSym(bufLoc, type,
>                             SignExtend64<bits>(sym.getVA(addend)));
>       continue;
>     }
> 
>     auto *ds = dyn_cast<Defined>(&sym);
>     if (!ds || !ds->section->repl || !ds->section->repl->getOutputSection() ||
>         ds->section->repl != ds->section)
>       target->relocateNoSym(bufLoc, type, Tombstone);
>     else
>       target->relocateNoSym(bufLoc, type,
>                             SignExtend64<bits>(sym.getVA(addend)));
>   }
> }
> ```
> Also isDebugSection() just do an excessive job it seems, because it checks the ".debug_" prefix.

By this I meant that probably no reason to have `isDebug &&` part in the condition:

```
isDebug && (name == ".debug_loc" || name == ".debug_ranges")
```

Though it is not the piece to really worry about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81784