[WebAssembly][MC] Fix computation of relative symbol offset

[ddcc]

Summary

For relative symbols, add its offset when computing relocation value.

[ddcc]

llvm/test/MC/WebAssembly/alias-relative.ll

31 ↗

(On Diff #290790)

I haven't been able to get obj2yaml or llvm-readobj to print out the full relocation information. Is there a more appropriate tool to use?

[sbc100]

Thanks for fixing this! Sorry it took me so long to get back to you. This lgtm % some comments.

Is this essentially a fix for the work that was started in https://reviews.llvm.org/D7946 ?

If we I wonder if the existing test for aliases at offset could be extended to include a relocation: llvm/test/MC/WebAssembly/offset.s? Rather than adding a new test?

I'd be happy to rename the existing test to alias-relative.ll or alias-offset.ll which seem more meaningful.

We are currently trying to move away from .ll tests in the MC directory to .s where possible so even if we do need to add a new test .s would probably be prefered.

llvm/lib/MC/WasmObjectWriter.cpp

1233

Is this a separate issue? It looks like a possible crash. Did you hit this case during testing? I'm curious what conditions would cause this.

llvm/test/MC/WebAssembly/alias-relative.ll

31 ↗

(On Diff #290790)

The relocation is shown above.. what more information are you looking for?

I would recommend objdump -d -r but I'm not sure its works well enough with WebAssembly at this point.

[ddcc]

Merge tests

[ddcc]

Is this essentially a fix for the work that was started in https://reviews.llvm.org/D79462 ?

Yeah, and enabling CFI will enable the LowerTypeTests pass which can generate relative addressing.

If we I wonder if the existing test for aliases at offset could be extended to include a relocation: llvm/test/MC/WebAssembly/offset.s? Rather than adding a new test?
I'd be happy to rename the existing test to alias-relative.ll or alias-offset.ll which seem more meaningful.
We are currently trying to move away from .ll tests in the MC directory to .s where possible so even if we do need to add a new test .s would probably be prefered.

Ok, this is pretty small so I can just merge it into the existing test.

llvm/lib/MC/WasmObjectWriter.cpp

1233

Currently, if you enable CFI for WebAssembly (-fsanitize=cfi -fvisibility=hidden), the LowerTypeTests pass will generate a null base pointer as an absolute address, which eventually triggers here. It was a workaround from back when I originally implemented CFI support; D87258 reworks the WAsm portion of that pass to avoid it.

llvm/test/MC/WebAssembly/alias-relative.ll

31 ↗

(On Diff #290790)

Never mind, I misunderstood the relocation behavior.

[sbc100]

Great!

llvm/test/MC/WebAssembly/alias-offset.s

2

If you add -r here will it also show the relocation information in the objdump output? (I can't remember if that works with wasm or not).

[ddcc]

Check relocation output

[ddcc]

llvm/test/MC/WebAssembly/alias-offset.s

2

Yup, added.

[sbc100]

Do you have commit access or should I land this for you?

[ddcc]

Landed, mind looking at https://reviews.llvm.org/D87258#2267267 when you get the time?

llvm/lib/MC/WasmObjectWriter.cpp

 //===- lib/MC/WasmObjectWriter.cpp - Wasm File Writer ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
WasmObjectWriter::getProvisionalValue(const WasmRelocationEntry &RelEntry,
   case wasm::R_WASM_MEMORY_ADDR_LEB:
   case wasm::R_WASM_MEMORY_ADDR_LEB64:
   case wasm::R_WASM_MEMORY_ADDR_SLEB:
   case wasm::R_WASM_MEMORY_ADDR_SLEB64:
   case wasm::R_WASM_MEMORY_ADDR_REL_SLEB:
   case wasm::R_WASM_MEMORY_ADDR_REL_SLEB64:
   case wasm::R_WASM_MEMORY_ADDR_I32:
   case wasm::R_WASM_MEMORY_ADDR_I64: {
-    // Provisional value is address of the global
+    // Provisional value is address of the global plus the offset
     const MCSymbolWasm *Base =
         cast<MCSymbolWasm>(Layout.getBaseSymbol(*RelEntry.Symbol));
     // For undefined symbols, use zero
     if (!Base->isDefined())
       return 0;
-    const wasm::WasmDataReference &Ref = DataLocations[Base];
-    const WasmDataSegment &Segment = DataSegments[Ref.Segment];
+    const wasm::WasmDataReference &BaseRef = DataLocations[Base],
+                                  &SymRef = DataLocations[RelEntry.Symbol];
+    const WasmDataSegment &Segment = DataSegments[BaseRef.Segment];
     // Ignore overflow. LLVM allows address arithmetic to silently wrap.
-    return Segment.Offset + Ref.Offset + RelEntry.Addend;
+    return Segment.Offset + BaseRef.Offset + SymRef.Offset + RelEntry.Addend;
   }
   default:
     llvm_unreachable("invalid relocation type");
   }
 }
 
 static void addData(SmallVectorImpl<char> &DataBytes,
                     MCSectionWasm &DataSection) {
uint64_t WasmObjectWriter::writeObject(MCAssembler &Asm,
   // symbols.  This must be done before populating WasmIndices for defined
   // symbols.
   for (const MCSymbol &S : Asm.symbols()) {
     const auto &WS = static_cast<const MCSymbolWasm &>(S);
 
     // Register types for all functions, including those with private linkage
     // (because wasm always needs a type signature).
     if (WS.isFunction()) {
-      const MCSymbolWasm *Base = cast<MCSymbolWasm>(Layout.getBaseSymbol(S));
-      registerFunctionType(*Base);
+      const auto *BS = Layout.getBaseSymbol(S);
+      if (!BS)
+        report_fatal_error(Twine(S.getName()) +
+                           ": absolute addressing not supported!");
+      registerFunctionType(*cast<MCSymbolWasm>(BS));
     }
 
     if (WS.isEvent())
       registerEventType(WS);
 
     if (WS.isTemporary())
       continue;
 
uint64_t WasmObjectWriter::writeObject(MCAssembler &Asm,
   // target of the alias before the alias itself and the symbols are not
   // necessarily ordered in this way.
   for (const MCSymbol &S : Asm.symbols()) {
     if (!S.isVariable())
       continue;
 
     assert(S.isDefined());
 
-    const MCSymbolWasm *Base = cast<MCSymbolWasm>(Layout.getBaseSymbol(S));
+    const auto *BS = Layout.getBaseSymbol(S);
+    if (!BS)
+      report_fatal_error(Twine(S.getName()) +
+                         ": absolute addressing not supported!");
+    const MCSymbolWasm *Base = cast<MCSymbolWasm>(BS);
 
     // Find the target symbol of this weak alias and export that index
     const auto &WS = static_cast<const MCSymbolWasm &>(S);
     LLVM_DEBUG(dbgs() << WS.getName() << ": weak alias of '" << *Base << "'\n");
 
     if (Base->isFunction()) {
       assert(WasmIndices.count(Base) > 0);
       uint32_t WasmIndex = WasmIndices.find(Base)->second;

llvm/test/MC/WebAssembly/alias-offset.s

+# RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj %s | llvm-objdump --triple=wasm32-unknown-unknown -d -t -r - | FileCheck %s
+
+  .section    .data,"",@
+foo:
+  .int32 0
+  .size foo, 4
+sym_a:
+  .int32 1
+  .int32 2
+  .size sym_a, 8
+
+.set sym_b, sym_a + 4
+
+# CHECK-LABEL: SYMBOL TABLE:
+# CHECK-NEXT: 00000000 l     O DATA foo
+# CHECK-NEXT: 00000004 l     O DATA sym_a
+# CHECK-NEXT: 00000008 l     O DATA sym_b
+# CHECK-NEXT: 00000001 l     F CODE main
+
+  .text
+  .section    .text,"",@
+main:
+  .functype   main () -> ()
+  i32.const sym_a
+  i32.store sym_b
+  end_function
+
+# CHECK-LABEL: <main>:
+# CHECK-EMPTY:
+# CHECK-NEXT:       3: 41 88 80 80 80 00     i32.const       8
+# CHECK-NEXT:                        00000004:  R_WASM_MEMORY_ADDR_SLEB      sym_a+0
+# CHECK-NEXT:       9: 36 02 8c 80 80 80 00  i32.store       12
+# CHECK-NEXT:                        0000000b:  R_WASM_MEMORY_ADDR_LEB      sym_b+0
+# CHECK-NEXT:      10: 0b            end

llvm/test/MC/WebAssembly/offset.s

-# RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj %s | llvm-objdump -t - | FileCheck %s
-
-.section    .data,"",@
-foo:
-  .int32 0
-  .size foo, 4
-sym_a:
-  .int32 1
-  .int32 2
-  .size sym_a, 8
-
-.set sym_b, sym_a + 4
-
-# CHECK: 00000000 l     O DATA foo
-# CHECK: 00000004 l     O DATA sym_a
-# CHECK: 00000008 l     O DATA sym_b