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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: lld/test/wasm/debuginfo-relocs.s:20
-# since its not address taken in the code.  In this case any relocations in the
-# debug sections see a address of zero.
-
----------------
sbc100 wrote:
> Why delete this file?  Can we just update the comment instead?
This test no longer tests the code it was intended to cover (see https://reviews.llvm.org/D66435) becuase that change is about handling R_WASM_TABLE_INDEX and now this test case generates R_WASM_FUNCTION_OFFSET instead. In fact, I'm not even sure it's possible anymore to get the situation that D66435 is supposed to handle (I at least couldn't think of another way to write a test for it).


================
Comment at: lld/test/wasm/map-file.s:33
-    .int32 123
-.size somedata, 4
 
----------------
sbc100 wrote:
> Why change this test?
The .debug_info section on line 40 has the same pattern that's being changed. Previously it generated R_WASM_TABLE_INDEX reloc, and therefore a table section in the linked binary (now there would be no table section). Since the point of this test is to show the link map with all the sections, I decided to go ahead and change the test so that the result would still include a table section.


================
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.
+
----------------
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.


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:630
   case wasm::R_WASM_SECTION_OFFSET_I32: {
+    if (!RelEntry.Symbol->isInSection())
+      return 0;
----------------
sbc100 wrote:
> Can we use `RelEntry.Symbol->isDefined` like below?  (or at least make them the same?)
It seems to pass the tests, although I'm not sure I understand what the difference is. `isInSection` is implemented as `return isDefined() && !isAbsolute();` but I'm not sure whether `isAbsolute` is relevant here. `isInSection` seems more intuitive given that the condition it guards goes to actually get the section?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103557