[PATCH] D42095: [WebAssembly] Symbol changes #3: Make Clang object file tables match LLD output. NFC.

[Nicholas Wilson via Phabricator via llvm-commits]

ncw 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:
> ncw wrote:
> > sbc100 wrote:
> > > 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?
> > Functions and function imports too. It's a map in the sense that it's an int->int lookup table; it's just a dense map with bounded elements so an array made sense.
> > 
> > `TableIndices[i]` is the table index of the ith function, so the name makes sense to me.
> When we talk about "function index space" that always includes imported functions.   
> 
> If that is the case then I think I prefer a map, since most functions are not address taken.  I also don't see why you wouldn't want it to continue to a map that is indexed by  symbol.
I see the code comment now, you're right the comment is anticipating a future change (should say "function index" not "function symbol index" since that doesn't exist yet).

I just did it because I thought the array would be "denser" in time/space, and take a bit less code than instantiating a map template. I suppose it doesn't matter, maybe using a DenseMap is clearer. I can make that change tomorrow.


================
Comment at: test/MC/WebAssembly/weak-alias.ll:197
+; CHECK-NEXT:           Value:           16
+; CHECK-NEXT:         Content:         '01000000'
+; CHECK-NEXT:   - Type:            CUSTOM
----------------
sbc100 wrote:
> sbc100 wrote:
> > ncw wrote:
> > > sbc100 wrote:
> > > > 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.
> > > Ah, the test data before was skipping the section - see the "CHECK" in the original file, rather than "CHECK-NEXT". I thought it was useful to expand the test to assert on the section that was being skipped; it was odd that previously it was deliberately not examining the final segment.
> > > 
> > > "It's not until link time that they become the same thing" -> but the table in the object file is linked. The "provisional" values that are used for relocations in the object file are a preview of the values that would get written out. I think it makes sense to use the same rules for writing out provisional values as we do for writing out the actual values in LLD. Hence we don't put the exact same function twice in the table - there's really no point. Seeing a function appear multiple times in the table doesn't make the output any easier to read.
> > But for intermediate output such as this (or the lld -r) we are still waiting for the a potential strong version of the symbol, so the weak alias and the function in points too are still district (both here and in lld -r).   
> > 
> Hmm.. maybe i'll expand this test expectation now as a separate change to make this change more explicit.
Yes - LLD might well resolve the indexes separately. But it doesn't seem to me that there's any added clarity in making the object file different to the linked output here (ie we're just outputting the same table that you'd get if you run LLD to link the single object file).

The output you're requesting is going to look like this. It doesn't seem to me to assist in examining the object file to put the same function twice in the table:

```
 (table 3 anyfunc)
 (elem (i32.const 1) $func $func)
 (memory $0 1)
 (export "memory" (memory $0))
 (export "func" (func $func))
 (export "funcAlias" (func $func)) // Weak alias for func
 (func $func  (type blah)
 )
 (func $anotherFunc (type blah)
  // A call to the alias and a call to the non-alias...
  // But there's no way to tell which is which! The alias is weakly-
  // defined, so it's not an import
  (call_indirect 1)
  (call_indirect 2)
 )
```

Of course the other alternative is just to output a totally empty table in object files!


Repository:
  rL LLVM

https://reviews.llvm.org/D42095