[PATCH] D103557: [WebAssembly] Generate R_WASM_FUNCTION_OFFSET relocs in debuginfo sections

[Sam Clegg via Phabricator via llvm-commits]

sbc100 accepted this revision.
sbc100 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:514
+    // later gets changed again to a func symbol?] or it can be a real
+    // function symbol, in which case it can be left as-is.
+
----------------
dschuff wrote:
> sbc100 wrote:
> > dschuff wrote:
> > > sbc100 wrote:
> > > > Its seem odd that does doesn't break stuff since R_WASM_SECTION_OFFSET_I32 relocations (where SymA is not a function) no longer get SymA converted to a section symbol?
> > > I think the R_WASM_SECTION_OFFSET_I32 relocations are never associated with function symbols, so they always fall into this case. (Whereas the R_WASM_FUNCTION_OFFSET relocs can be either way). This condition could also be written as 
> > > ```
> > >   if (((Type == wasm::R_WASM_FUNCTION_OFFSET_I32 ||
> > >        Type == wasm::R_WASM_FUNCTION_OFFSET_I64) &&
> > >       !SymA->isFunction()) ||
> > >       Type == wasm::R_WASM_SECTION_OFFSET_I32) {
> > > ```
> > > and it also seems to work.
> > I still don't understand.  Surely we want to use the section symbol for all relocations of this type not just the  `!SymA->isFunction()` ones?     
> > 
> > i.e. Why do we leave SymA alone one when `isFunction()` returns true?  Why not always adjust SymA to be a section symbol?
> We can't use the section symbol for the cases in PR50408 because the function is undefined, and therefore not in any section.
> It seems that for every other case the symbol is not actually a function symbol but a data symbol.
> In other words, I don't know of any cases where `IsFunction()` and `IsDefined()` are both true.
> If there were, I think we would have the correct behavior either way, we could just get it via 2 different codepaths. I still don't fulle understand how the section-symbol case ends up using the defined function symbol by the time the object file is written.
> Another way this could be written could maybe be
> ```
>   if ((Type == wasm::R_WASM_FUNCTION_OFFSET_I32 ||
>        Type == wasm::R_WASM_FUNCTION_OFFSET_I64 ||
>        Type == wasm::R_WASM_SECTION_OFFSET_I32) &&
>       SymA->isDefined()) {
> ```
> That would mean that if SymA were a defined function it would go through the section-symbol codepath instead. That also passes all the tests we have (but as I said, I'm pretty sure our tests don't cover that case).
I think I would prefer this since it obvious why undefined symbols can't be replaced with the corresponding section symbol.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103557