[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:
> 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?
actually no, the content of the debug_info section is now 2 (because it's the offset of bar from the start of the code section, rather than a table index).  And the whole point of the test originally was to test lld's behavior on table_index relocations, which it doesn't anymore. So this test doesn't test anything not covered by other tests.


================
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:
> 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).


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:630
   case wasm::R_WASM_SECTION_OFFSET_I32: {
+    if (!RelEntry.Symbol->isInSection())
+      return 0;
----------------
sbc100 wrote:
> 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).
OK yeah, that makes sense given the invariant.


================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyWasmObjectWriter.cpp:47
 
-static const MCSection *getFixupSection(const MCExpr *Expr) {
+static const MCSection *getTargetSection(const MCExpr *Expr) {
   if (auto SyExp = dyn_cast<MCSymbolRefExpr>(Expr)) {
----------------
sbc100 wrote:
> I'm still not sure about the rename of this function.    This function seems to return the section in which the symbol being relocated against is defined.   Is that the "target" section?  Or is the target section the section in which the relocation itself lives?
yes, I guess it's a matter of perspective. To me the "fixup section", is the section being fixed up, which this is not.  I'm open to other ideas for names though.


================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyWasmObjectWriter.cpp:136
+    if (SymA.isFunction()) {
+      if (FixupSection.getKind().isMetadata())
+        return wasm::R_WASM_FUNCTION_OFFSET_I64;
----------------
sbc100 wrote:
> I guess this is fine for now, but would break if we ever have metadata sections that want to refer to the table index of a function? 
Yes, it would. I couldn't think of any cases where we'd need that (nor a convenient way to tell the difference in that case, short of having another fixup type?)


================
Comment at: llvm/test/MC/WebAssembly/debuginfo-relocs.s:1
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
+# RUN: obj2yaml %t.o | FileCheck %s
----------------
sbc100 wrote:
> Maybe comment here saying what we are testing?  i.e. that we always generate R_WASM_FUNCTION_OFFSET_I32 in debug sections.
> 
> I wonder if it would make sense to use a relocation type in the assembly here to make this more explicit: i.e. `.int32 bar at OFF`?  Might make this change more invasive but would be less magical perhaps?  I think it would involve a new  MCSymbolRefExpr::VariantKind such as VS_BYTE_OFFSET? 
I added a comment.
That's an interesting idea about the SymbolRefExpr. I guess that would allow assembly code to explicitly select either byte offset or table index relocs. Would it be possible to use it in the compiler though?
One of the weird things I discovered working on this CL is that for the existing case (where it starts with a temp data symbol, and we convert it to use the section symbols) I couldn't think of any way to test that with assembly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103557