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

[Sam Clegg via Phabricator via llvm-commits]

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.
-
----------------
Why delete this file?  Can we just update the comment instead?


================
Comment at: lld/test/wasm/map-file.s:33
-    .int32 123
-.size somedata, 4
 
----------------
Why change this test?


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


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:630
   case wasm::R_WASM_SECTION_OFFSET_I32: {
+    if (!RelEntry.Symbol->isInSection())
+      return 0;
----------------
Can we use `RelEntry.Symbol->isDefined` like below?  (or at least make them the same?)


================
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)) {
----------------
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?


================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyWasmObjectWriter.cpp:136
+    if (SymA.isFunction()) {
+      if (FixupSection.getKind().isMetadata())
+        return wasm::R_WASM_FUNCTION_OFFSET_I64;
----------------
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? 


================
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
----------------
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? 


================
Comment at: llvm/test/MC/WebAssembly/debuginfo-relocs.s:39
+# CHECK-NEXT:        Offset:          0x8
+# CHECK-NEXT:         Name:            .debug_info
+
----------------
Indentation looks off here


================
Comment at: llvm/test/MC/WebAssembly/debuginfo-relocs.s:40
+# CHECK-NEXT:         Name:            .debug_info
+
----------------
Remove blank line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103557