This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Make assembler check for proper nesting of control flow.
ClosedPublic

Authored by aardappel on Dec 17 2018, 5:00 PM.

Details

Summary

It does so using a simple nesting stack, and gives clear errors upon
violation. This is unique to wasm, since most CPUs do not have
any nested constructs.

Had to add an end of file check to the general assembler for this.

Note: if/else/end instructions are not currently supported in our
tablegen defs, so these tests will be enabled in a follow-up.
They already pass the nesting check.

Diff Detail

Repository
rL LLVM

Event Timeline

aardappel created this revision.Dec 17 2018, 5:00 PM
dschuff accepted this revision.Dec 17 2018, 5:09 PM
This revision is now accepted and ready to land.Dec 17 2018, 5:09 PM
aheejin added a comment.EditedDec 17 2018, 5:12 PM
  • Function names should begin with a lowercase letter
  • Is there a test case that actually generates an error so we can test the error is correctly generated? It looks like we don't have a test here (Test changes here don't look relevant to this CL)
aheejin added inline comments.Dec 17 2018, 5:18 PM
test/MC/WebAssembly/basic-assembly.s
66 ↗(On Diff #178564)

I guess this is not relevant to this CL?

141 ↗(On Diff #178564)

Here too

test/MC/WebAssembly/simd-encodings.s
4 ↗(On Diff #178564)

I guess this is not relevant to this CL?

Nevermind these two comments, but still think a error-generating test case would be great

test/MC/WebAssembly/basic-assembly.s
66 ↗(On Diff #178564)

Oh nevermind, it is.

141 ↗(On Diff #178564)

Nevermind again

FYI you can check error message of an invalid test case by appending not in the RUN line and redirect the stderr to stdout, like this test.

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
183 ↗(On Diff #178564)

We have Try now too

230 ↗(On Diff #178564)

+ try handling

241 ↗(On Diff #178564)

Nit: Maybe add " or " between to strings to clarify we require only one of them?

246 ↗(On Diff #178564)

Nit: Maybe add " or " between to strings to clarify we require only one of them?

403 ↗(On Diff #178564)

We have end_try too

405 ↗(On Diff #178564)

Did you mean to use &&? Not sure if || makes sense here

559 ↗(On Diff #178564)

Nit: Given that this is not a function end but rather a function start, how about renaming the function to something like ensureEmptyControlFlowStack or something and change the error message accordingly?

aardappel marked 16 inline comments as done.Dec 20 2018, 3:44 PM

Added a test that tests the errors too.

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
403 ↗(On Diff #178564)

Ok, added support for try/catch/catch_all/end_try

405 ↗(On Diff #178564)

in the parser, functions return true on failure.. which is a bit odd, yes.

559 ↗(On Diff #178564)

It is also possibly the end of the previous function (if end_function was forgotten. But sure, I'll change the name.

test/MC/WebAssembly/simd-encodings.s
4 ↗(On Diff #178564)

It is.. it had an end_function without a start of a function

aardappel updated this revision to Diff 179184.Dec 20 2018, 3:45 PM
aardappel marked 4 inline comments as done.

Addressed code review.

aheejin added inline comments.Dec 20 2018, 4:18 PM
lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
405 ↗(On Diff #178564)

Ah I see.

test/MC/WebAssembly/basic-assembly-errors.s
16 ↗(On Diff #179184)

Indent the same way with other lines

22 ↗(On Diff #179184)

These error messages are a little confusing, because we have end_loop in the code but the error message says "we got loop". The same for all other messages. I guess we should change the message to reflect the actual instructions in the test?

aardappel marked 2 inline comments as done.Dec 20 2018, 4:44 PM
aardappel updated this revision to Diff 179207.Dec 20 2018, 4:44 PM

Addressed code review.

  • It looks like even if an error occurs, the parsing continues. Is this an expected behavior?
  • If it is, is a mismatched instruction supposed to pop the stack? For example,
try
loop
block
end_loop    // stray incorrect instruction
end_block
end_loop
end_try

In this case, there is a stray end_loop instruction in the middle of an otherwise correct sequence of instructions. When we parse end_loop, we pop block and compare that with end_loop and emit an error, which is fine. But next we parse end_block, but block was incorrectly popped when we parse loop, it is going to fail. The next instruction as well, and this ends up failing all the instructions. But if we don't pop the stack when an incorrect match occurs, there is gonna be only one error, other than four. I'm not sure what the right behavior is, so I'm asking.

test/MC/WebAssembly/basic-assembly-errors.s
1 ↗(On Diff #179207)

This is ok as is, but it looks like usually people append not in front of the command (in which way you don't need || true)

13 ↗(On Diff #179207)

By the way, can we parse if? You commented these out in the other test case saying they are not supported in tablegen.

16 ↗(On Diff #179207)

Looks like this line is indented with a tab, while others with spaces.

22 ↗(On Diff #179207)

How about interleaving these lines within the test case, so we can easily see which lines match which instruction? In that way we may not need inline comments for those instructions as well

aardappel marked 5 inline comments as done.Dec 26 2018, 9:18 AM

@aheejin
Yes, the parser normally continues after an error, that is out of my control.. unless I put in a hack like consuming all tokens until EOF.

I can make it not pop if there is an error, which is indeed slightly better behavior. Do note that not popping equally can cause unwanted errors:

try
loop
block
end_if  // I meant to write end_block, error.
end_loop // now also error
end_try // now also error

But that should be slightly less likely than your example. Does require a different structure in basic-assembly-errors.s

test/MC/WebAssembly/basic-assembly-errors.s
13 ↗(On Diff #179207)

Yes, they will be added in the next CL. dschuff suggested it was nicer to keep them seperate.

aardappel updated this revision to Diff 179514.Dec 26 2018, 9:27 AM
aardappel marked an inline comment as done.

More code review fixes.

aheejin accepted this revision.Dec 26 2018, 2:08 PM
aheejin added inline comments.
lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
258 ↗(On Diff #179514)

This looks nice!

This revision was automatically updated to reflect the committed changes.

Aahhhhh don't meke me mad I will found you