Page MenuHomePhabricator

[WebAssembly] Add support for the event section
ClosedPublic

Authored by aheejin on Nov 25 2018, 12:44 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Nov 25 2018, 12:44 AM
aheejin marked an inline comment as done.Nov 25 2018, 12:48 AM
aheejin added inline comments.
wasm/InputEntity.h
31 ↗(On Diff #175172)

This is supposed to be the parent class for both InputGlobal and InputEvent, and I'm not sure if this is a good name. I also thought about InputNonChunk, but that didn't sound very intuitive either. Suggestions are welcome.

aheejin updated this revision to Diff 175173.Nov 25 2018, 1:33 AM
  • Fix a typo
sbc100 added a subscriber: ruiu.Nov 27 2018, 3:11 PM

lgtm % nits and the question of whether we leave SigIndex as part of the EventType in wasm.h.

wasm/InputFiles.cpp
61 ↗(On Diff #175173)

Align on :

280 ↗(On Diff #175173)

It would make sense to me if the type was part of the Event struct.

wasm/SymbolTable.cpp
146 ↗(On Diff #175173)

TYPE_EVENT?

160 ↗(On Diff #175173)

I would expect this to looks basically the same as checkFunctionType.

What are we storing the Attribute exactly? (should it be Attributes plural?)

195 ↗(On Diff #175173)

I would drop all references to syntheticevents until/unless you actually need them.

wasm/Symbols.h
318 ↗(On Diff #175173)

IIRC we don't have undefined events yet do we? Is it worth leaving that out until we have a use case for them?

wasm/Writer.cpp
196 ↗(On Diff #175173)

If you leave SigIndex as part of the EventType could you write this as:

Import.Event.Attributes = EventSym->getEventType().Attributes;
Import.Event.SigIndex = lookupType(*EventSym->Signature);
797 ↗(On Diff #175173)

A tip from @ruiu for this kind of case is to use a cast after the last else rather than a dyn_cast. This will fail in the same way without needing the additional assert.

sbc100 added inline comments.Nov 27 2018, 3:14 PM
wasm/WriterUtils.cpp
214 ↗(On Diff #175173)

if sig was part of this we could print it here too.

ruiu added a comment.Nov 28 2018, 1:22 PM

Overall, this patch needs more comments. lld should be readable to those who knows what the linker is but don't know too much about wasm, so you should explain what event symbol/section/etc. are.

wasm/InputEntity.h
25–26 ↗(On Diff #175173)

We don't usually add using to headers. Please consider adding these identifiers to lld/include/lld/Common/LLVM.h.

31 ↗(On Diff #175173)

We should keep the class hierarchy as shallow as they can be, and we should avoid using inheritance if we can live without it. If something can be written just as a non-member function, we should write that way. I'm trying to not use too many object-oriented features in lld.

It looks like this abstract class is not really needed to me; you could just define InputGlobal and InputEvent as two separate classes. Can you make that change?

wasm/Symbols.h
318 ↗(On Diff #175173)

Do you really have to distinguish this type of object from other symbols by class? It feels like it is too much use of class inheritance.

aheejin updated this revision to Diff 176539.Dec 3 2018, 7:57 PM
aheejin marked 15 inline comments as done.
aheejin added inline comments.Dec 3 2018, 7:58 PM
wasm/InputEntity.h
31 ↗(On Diff #175173)

Done. Do you think it's ok to keep the two classes InputGlobal and InputEvent in this same header InputEntity.h? If so, do you have any other opinions on the file name?

wasm/SymbolTable.cpp
160 ↗(On Diff #175173)

I would expect this to looks basically the same as checkFunctionType.

That's not possible, because we have to check both the Attribute and the signature. WasmEventType's (in)equality operator just checks whether Attribute matches, because comparing SigIndex of WasmEventTypes from different objects does not make sense.

What are we storing the Attribute exactly?

The possible list of attributes is here, which is, only WASM_EVENT_ATTRIBUTE_EXCEPTION for now. The Attribute field is supposed to store the kind of event, for later when we get to support other kinds of events that modifies control flow in a various way. Possible candidates have been discussed here, but we don't have a concrete plan to implement any of these yet now.

(should it be Attributes plural?)

Why do you think so?

wasm/Symbols.h
318 ↗(On Diff #175173)

@sbc100 Deleted it.

@ruiu This does not necessarily increase the depth of inheritance; we need at least EventSymbol and DefinedEvent (which is child of EventSymbol). EventSymbol has a different characteristics from all other existing symbols such as FunctionSymbol or GlobalSymbol, so we can't merge them. We don't currently have UndefinedEvent so I can delete it.

wasm/WriterUtils.cpp
214 ↗(On Diff #175173)

How? WasmEventType just contains an int sigindex, not a WasmSignature object.

@ruiu I add some comments here and there, but it's hard to explain the concept of wasm event fully in the comments only. Wasm globals seem to not have comments for the same reason.

aheejin updated this revision to Diff 176540.Dec 3 2018, 8:02 PM
  • clang-format
sbc100 added a comment.Dec 6 2018, 4:31 PM

This change is looking nice!

The only thing I'm not sure about is the header file named "InputEntity.h". This names doesn't mean much. Perhaps we can avoid the problem by just have InputGlobal.h and InputEvent.h as separate headers?

Aside form that an couple of nits this lgtm.

include/lld/Common/LLVM.h
50 ↗(On Diff #176540)

nit: alphabetize

test/wasm/Inputs/event-section1.ll
10 ↗(On Diff #176540)

nit: remove trailing newlines, or is that just phabricator?

wasm/SymbolTable.cpp
153 ↗(On Diff #176540)

This reminds me that Attribute is a really bad name, but we don't need to block landing this CL on that name which is LLVM side.

aheejin updated this revision to Diff 177100.Dec 6 2018, 7:45 PM
aheejin marked 3 inline comments as done.
  • Address comments
wasm/SymbolTable.cpp
153 ↗(On Diff #176540)

As I said in another CL, we can change it in the spec (and LLVM) if we want. Do you have any suggestions?

aheejin updated this revision to Diff 177102.Dec 6 2018, 7:55 PM

Rebase onto D55406

sbc100 accepted this revision.Dec 7 2018, 10:58 AM
This revision is now accepted and ready to land.Dec 7 2018, 10:58 AM
ruiu accepted this revision.Dec 7 2018, 11:06 AM

LGTM

wasm/InputFiles.cpp
62 ↗(On Diff #177102)

I think the first column should be aligned to the right, instead of inserting two spaces after "Event Imports", because other rows are aligned that way.

wasm/WriterUtils.cpp
213–214 ↗(On Diff #177102)

nit: you can write this with less indentation.

if (Type.Attribute == WASM_EVENT_ATTRIBUTE_EXCEPTION)
  return "exception";
return "unknown";
aheejin updated this revision to Diff 177291.Dec 7 2018, 1:12 PM
aheejin marked 2 inline comments as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.