[PATCH] D87258: [WebAssembly, LowerTypeTests] Fix control-flow integrity support

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

So is the idea that the table indexes would be reserved also by the linker?

Right now wasm-ld completely ignores the table elements in the object files and generates contiguous table entries when if finds TABLE_INDEX relocations.



================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:548
+    // subsequent conflict if an index has already been assigned
+    if (!Rec.Symbol->hasTableIndex())
+      CodeRelocations.push_back(Rec);
----------------
Would the read better if you flipped the if/else and remove the negation?


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:622
     // 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;
   }
----------------
ddcc wrote:
> Not sure if this is entirely correct, please take a look. My understanding is that D79462 introduced a bug where it fails to add the offset from a relative symbol. Without these changes, I get the following error from the lld side:
> 
> `wasm-ld: warning: unexpected existing value for R_WASM_MEMORY_ADDR_SLEB: existing=8420 expected=8452`
> 
> After some debugging, it appears that this is a relative symbol, where the +32 offset gets missed since only the base is examined.
> 
> ```
> RelEntry.Symbol->getVariableValue(false): .L__unnamed_1+32
> symbol: Name=.L_ZTV3Foo.0, Kind=WASM_SYMBOL_TYPE_DATA, Flags=2, Segment=62, Offset=32, Size=20
> WasmObjectWriter::writeObject: Ref = {Segment = 62, Offset = 32, Size = 20}
> WasmObjectWriter::getProvisionalValue: Ref = {Segment = 62, Offset = 0, Size = 52}
> ```
This looks correct to me.   Is the reason we have not been seeing because we have `-fdata-sections` on by default so all symbols are in their own section?

I would hope we could land this separately with its own test(s).  Do you have a way to trigger this bug?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87258