[PATCH] D91803: [lld] Use -1 as tombstone value for discarded code ranges

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/wasm/InputChunks.cpp:298
     compressedFuncSize += rel.Offset - lastRelocEnd;
-    compressedFuncSize += getRelocWidth(rel, file->calcNewValue(rel));
+    compressedFuncSize += getRelocWidth(rel, file->calcNewValue(rel, getTombstone()));
     lastRelocEnd = rel.Offset + getRelocWidthPadded(rel);
----------------
For all these `file->calcNewValue(rel, getTombstone())` calls, shouldn't `getTombstone()` cached to avoid performance overhead?


================
Comment at: lld/wasm/InputFiles.cpp:196
+    if ((isa<FunctionSymbol>(sym) || isa<DataSymbol>(sym)) && !sym->isLive()) {
+      return tombstone ? tombstone : reloc.Addend;
+    }
----------------
dblaikie wrote:
> Eric wrote:
> > dblaikie wrote:
> > > sbc100 wrote:
> > > > So setting the `tombstone` to zero means we should include the `Addend` ?
> > > Yeah, I guess that's beyond my debug info purview - but I'd suggest checking what bfd ld does here & maybe going with that, rather than what gold does/lld used to do. Which is probably absolute 0 rather than addend. For broader compatibility (& probably make this code a bit less subtle - avoiding the tombstone of zero not really being zero)
> > Yes, this was based on MaskRay@'s comment on my initial commit. If there is not agreement on this perhaps bring discussion back to the email thread?
> Ah, [[ https://reviews.llvm.org/D91803?vs=on&id=306711#2405969 | this ]] one?
> 
> > The change does not seem right. For non-debug sections (when --allow-undefined is set), addend; .debug_ranges/.debug_loc => -2; other .debug_* => -1
> 
> @MaskRay is that what ld.bfd does? ("addend" for non-debug sections)
Yes for "addend" for non-debug sections. This makes subtraction work.


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

https://reviews.llvm.org/D91803