[PATCH] D42095: [WebAssembly] Symbol changes #3: Cosmetic table, LLVM. NFC.

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lib/MC/WasmObjectWriter.cpp:220
   DenseMap<const MCSymbolWasm *, uint32_t> SymbolIndices;
+  // Maps function symbol indices to the table element index space. Used
+  // for TABLE_INDEX relocation types (i.e. address taken functions).
----------------
sbc100 wrote:
> Maps function index to table element index, no?  Only functions can exist in this map right?
Also, this isn't really map is it?  It more a list of function indexes to be written to the indirect function table.

With that in mind, why not call it TableElems, perhaps?


================
Comment at: lib/MC/WasmObjectWriter.cpp:515
+    uint32_t SymbolIndex = SymbolIndices[Sym];
+    int32_t TableIndex = TableIndices[SymbolIndex];
+    assert(TableIndex >= 0);
----------------
This doesn't look like it should work to me.

TableIndices is not a map.. but a list of function indexes.. so it seems like this will look up the N'th element in the table, not the table entry for function N.


================
Comment at: test/MC/WebAssembly/weak-alias.ll:197
+; CHECK-NEXT:           Value:           16
+; CHECK-NEXT:         Content:         '01000000'
+; CHECK-NEXT:   - Type:            CUSTOM
----------------
How as this change generated a new data segment?  I guess maybe it was zero before so now it was elided?  

It looks to me like we really want to two different function addresses here for the address of the function itself and the address of the weak alias.  Its not until link time that these two things become the same thing.  Maybe it doesn't really matter as long as the linker can distinguish them, but it makes sense to be that there would be two different function addresses in this object file.


Repository:
  rL LLVM

https://reviews.llvm.org/D42095