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

Sam Clegg via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 16 14:55:30 PDT 2021


sbc100 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.
-
----------------
dschuff wrote:
> 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).
But why not just update the comment?  This .s file is still valid right?  And it should have the same content (zero) for the debug_info 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.
+
----------------
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?


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:630
   case wasm::R_WASM_SECTION_OFFSET_I32: {
+    if (!RelEntry.Symbol->isInSection())
+      return 0;
----------------
dschuff wrote:
> 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?
Given that we have the following code below I still think we should be consistent:

```
    // For undefined symbols, use zero
    if (!RelEntry.Symbol->isDefined())
      return 0;
```

All defined functions must have a section .. I think that is just as intuitive (maybe more intuitive).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103557



More information about the llvm-commits mailing list