This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Make WasmSymbol's signature usable for events (NFC)
ClosedPublic

Authored by aheejin on Dec 3 2018, 8:04 PM.

Details

Summary

WasmSignature used to use its WasmSignature member variable only for
function types, but now it also can be used for events as well.

Companion to D54875.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Dec 3 2018, 8:04 PM
sbc100 accepted this revision.Dec 3 2018, 9:18 PM

Should the first word of the description be WasmSymbol

include/llvm/BinaryFormat/Wasm.h
335 ↗(On Diff #176541)

Did you mean to remove this?

EventTypes are not equal unless the signature is equal too right?

This revision is now accepted and ready to land.Dec 3 2018, 9:18 PM
aheejin marked an inline comment as done.Dec 4 2018, 9:08 AM
aheejin added inline comments.
include/llvm/BinaryFormat/Wasm.h
335 ↗(On Diff #176541)

This was supposed to be used in checkEventType() function in lld patch (D54875). When parsing multiple object files and adding events from the object files, the function is used to check the pre-existing event type with the same name has the same attribute and signature with the new event type. At that point, it is adding events from several objects and the same SigIndex does not mean anything, because you can't compare sigindex of different objects. This was one of the reasons that I wanted to take SigIndex out of WasmEventType in D54873.

Anyway, I just checked the current state of D54873 and it is not currently using this operator but comparing Attribute field directly, like this:

const WasmEventType *OldType = cast<EventSymbol>(Existing)->getEventType();
const WasmSignature *OldSig = ExistingEvent->Signature;
if (NewType->Attribute != OldType->Attribute)
  error("Event type mismatch: " + Existing->getName() + "\n>>> defined as " +
        toString(*OldType) + " in " + toString(Existing->getFile()) +
        "\n>>> defined as " + toString(*NewType) + " in " + toString(File));
if (*NewSig != *OldSig)
  warn("Event signature mismatch: " + Existing->getName() +
       "\n>>> defined as " + toString(*OldSig) + " in " +
       toString(Existing->getFile()) + "\n>>> defined as " +
       toString(*NewSig) + " in " + toString(File));

So I think I can actually delete this operator.

sbc100 added inline comments.Dec 4 2018, 9:45 AM
include/llvm/BinaryFormat/Wasm.h
335 ↗(On Diff #176541)

Thats sounds like the right direction yes.

aheejin updated this revision to Diff 176761.Dec 4 2018, 9:14 PM
  • Delete unused operators
sbc100 accepted this revision.Dec 4 2018, 10:19 PM

IIUC this change is NFC so you can mark it as such in the title?

aheejin retitled this revision from [WebAssembly] Make WasmSymbol's signature usable for events to [WebAssembly] Make WasmSymbol's signature usable for events (NFC).Dec 4 2018, 10:24 PM
This revision was automatically updated to reflect the committed changes.