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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 3 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/ELF/InputSection.cpp:935
+                              isDebugLocOrRanges ? UINT64_MAX - 1 : UINT64_MAX);
+        continue;
+      }
----------------
grimar wrote:
> 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.
`isDebugLocOrRanges ? UINT64_MAX - 1 : UINT64_MAX` can easily hoisted by the compiler if it likes. Manually doing this is unnecessary.

> Though it is not the piece to really worry about.

Agree.

> because auto *ds = dyn_cast<Defined>(&sym) is always called, though it is not always needed.

The suggested transformation looks a bit unnecessary to me. Most relocations from non-SHF_ALLOC sections are from `.debug_*`. The remaining ones are uncommon and/or tiny sections such as __patchable_function_entries, .stack_sizes, .llvm_addrsig, etc. There is very little we can save from the restricting the scope of `dyn_cast`.

I want to keep the control flows as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81784