Page MenuHomePhabricator

[WebAssembly] Remove unneeded MCSymbolRefExpr variants
ClosedPublic

Authored by sbc100 on Feb 20 2019, 12:45 PM.

Details

Summary

Also simplify WebAssemblyWasmObjectWriter::getRelocType.

We record the type of the symbol (event/function/data/global) in the
MCWasmSymbol and so it should always be clear how to handle a relocation
based on the symbol itself.

The exception is a function which still needs the special @TYPEINDEX
then the relocation contains the signature rather than the address
of the functions.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Feb 20 2019, 12:45 PM
sbc100 updated this revision to Diff 187659.Feb 20 2019, 12:53 PM
sbc100 retitled this revision from [WebAssembly] Remove unneeded MCSymbolRefExpr variants Also simplify WebAssemblyWasmObjectWriter::getRelocType. to [WebAssembly] Remove unneeded MCSymbolRefExpr variants.
sbc100 edited the summary of this revision. (Show Details)
  • revert part
sbc100 edited the summary of this revision. (Show Details)Feb 20 2019, 12:54 PM
sbc100 added a reviewer: sunfish.

@sunfish, what do you think? I'm not totally sure about how this part of the code is supposed to work, but this seems better to me.

sbc100 edited the summary of this revision. (Show Details)
aheejin accepted this revision.Feb 20 2019, 5:49 PM

Looks OK to me, we have all the info in MCSymbolWasm after all.

llvm/lib/MC/WasmObjectWriter.cpp
1559 ↗(On Diff #187659)

I guess this should be below if (!SymRef) test?

This revision is now accepted and ready to land.Feb 20 2019, 5:49 PM

IIRC this code goes back to when we were piggybacking on ELF, and is more-or-less there to match how those variants are used on ELF. I don't see any asm parser changes in this CL, but I could imagine that they would exist so that the assembler could know what kind of reloc to use even without any context or special knowledge of symbol types; does this change affect how asm is parsed? Maybe we've already added enough extra intelligence to the assembler that we don't need them.

sbc100 marked an inline comment as done.Feb 22 2019, 1:50 PM

IIRC this code goes back to when we were piggybacking on ELF, and is more-or-less there to match how those variants are used on ELF. I don't see any asm parser changes in this CL, but I could imagine that they would exist so that the assembler could know what kind of reloc to use even without any context or special knowledge of symbol types; does this change affect how asm is parsed? Maybe we've already added enough extra intelligence to the assembler that we don't need them.

You are correct. The @XXX is for the reloc type. When a symbol is referenced normally its just means that address of the symbol, but for things other than address (or normal refs) you need the @. That is why we still have the @TYPEINDEX for when you want the functions type, not its address. For any given symbol we already know the type and we can defined a default meaning when we the symbol. We don't need to decorate the default references IIUC.

dschuff accepted this revision.Feb 22 2019, 2:06 PM

Makes sense. I think the thing that has changed compared to when we started is that symbol types like function, global, and event are more first-class in the wasm object format. Anyway, this change seems fine. I assume the asm parser already isn't using these annotations and we still round-trip?

Makes sense. I think the thing that has changed compared to when we started is that symbol types like function, global, and event are more first-class in the wasm object format. Anyway, this change seems fine. I assume the asm parser already isn't using these annotations and we still round-trip?

Right, the symbols types from the special ".functype" and ".eventtype" declarations I believe, not from the references.

This revision was automatically updated to reflect the committed changes.