Add tests for if/then/else with indices + unknown label; #284

bnjbvr · 2016-05-10T16:24:00Z

Hi! I'd like to add test cases we ran into in our implementation and which could be useful for others:

a test for if/then/else where the depths are not labels, but raw integer indices.
a test for if/then/else where the label target of a br in the else is the label of the then block.

Unfortunately, the second test (at the end of the file) doesn't work: the parser complains that the label $l is unknown, which is exactly what we are testing here. The correct behavior would be to silently swallow the error and consider the test as passed. Am I missing something here?

rossberg · 2016-05-10T16:32:47Z

The current behaviour is correct: the body of each if-branch is (implicitly) a block, and the labels name these respective blocks. Not sure why you expect a label to be in scope in a sibling block? It's no different from

(if (...) (then (block $l ...)) (else (block (br $l))))

which likewise is an error.

The other test look good.

kripken · 2016-05-10T16:37:45Z

@rossberg-chromium I think it's not meant to be in scope, it should be an error - it's an assert_invalid test.

rossberg · 2016-05-10T16:44:02Z

Oh, sorry, totally missed that. Yeah, that's definitely a bug, I'll look into it.

rossberg · 2016-05-12T17:34:37Z

Okay, I now see what's going on. The problem is that labels (and identifiers in general) are resolved in the parser, because the AST doesn't have them. An unbound name hence is a syntax error, rejecting the script before you ever get to run it.

So it's not really a bug but a limitation of scripts -- a script can't check for this kind of error, just like it can't check other syntax errors. You'll need to check externally against expected-output instead.

binji · 2016-05-12T17:53:33Z

@rossberg-chromium I actually ran an issue in sexpr-wasm because of this; prior to having binary modules, I could assume that assert_invalid would only be given modules that were syntactically valid. So I moved the checking pass out of the parser, and the parser always returns a module as an AST, even inside assert_invalid. But now that binary modules can fail to parse, I need a second path for passing back binary data that is only later parsed and checked for errors.

bnjbvr · 2016-05-13T06:58:50Z

So it's not really a bug but a limitation of scripts -- a script can't check for this kind of error, just like it can't check other syntax errors. You'll need to check externally against expected-output instead.

Thank you for the explanation, I can do this, however I wonder how https://github.com/WebAssembly/spec/blob/master/ml-proto/test/switch.wast#L155 doesn't trigger the exact same issue?

rossberg · 2016-05-13T07:35:19Z

@bnjbvr, that test uses a raw numeric index. What's resolved in the parser is the use of symbolic labels, and their translation into numeric ones (because they are purely syntactic sugar, not a part of the Wasm AST).

I certainly won't argue that this difference is nice. :)

bnjbvr · 2016-05-13T09:08:11Z

I've updated the PR. The code wasn't accepting non-zero exit codes, so I had to refactor this, by adding a way to specify that a test is expected to fail. Also, there were issues with redirecting stderr to stdout with a > in the command argument, so I've refactored the way output is written so that stdout and stderr goes to the output file, instead. How does this look?

rossberg · 2016-05-13T09:37:15Z

ml-proto/runtests.py

+
+      if headers[0] == ';;;':
+        for h in headers[1:]:
+          if h.strip() == 'expected-fail':


Instead of instrumenting comments in the file, what do you think about simply using a file name convention? Say, <test>.fail.wast for tests that ought to fail? That's easier and cheaper to process (including for other test harnesses).

Yeah, good idea.

rossberg · 2016-05-13T09:38:15Z

Thanks! Looks good modulo a few comments.

bnjbvr · 2016-05-13T12:19:52Z

Updated the latest patch, thank you for the review!

rossberg · 2016-05-13T12:23:35Z

ml-proto/runtests.py


  def _runTestFile(self, shortName, fileName, interpreterPath):
+
+    expectedExitCode = 1 if "fail" in fileName else 0


Can we make this a bit narrower? ".fail." should be good.

rossberg · 2016-05-13T19:49:37Z

Hm, Travis still barfs at the .fail test, although I can't immediately say why.

rossberg · 2016-05-13T19:51:55Z

ml-proto/runtests.py

+    self.assertEqual(expectedExitCode, exitCode, "test runner failed with exit code %i, expected %i" % (exitCode, expectedExitCode))
+
+    if logPath is not None:
+        out.close()


One further style nit I just noted: inconsistent indentation, rest of the file uses 2

…ects output to the right places in all cases;

bnjbvr · 2016-05-17T15:12:06Z

Right, the test was failing because we are now very tied to line numbers, which is unfortunate. Addressed the two styles nits and updated the expected output to match line numbers.

rossberg · 2016-05-19T12:47:46Z

Cool, thanks! LGTM and landing.

* Reintroduce exnref * Fix type_of * Simplify parser * Implement 1b * Change opcodes for catch/catch_all to avoid conflict * Put catch clauses first * Remove obsolete Delegating cases * Change exn type opcode to -0x17 * Switch to B' variant * [interpreter] Add boilerplate for ref.exn result patterns * [ci] Deactivate node run, since it can't handle try_table yet * Try -> TryTable in AST * [spec] Update spec for option B' (#283) * Deactivate Bikeshed * [spec/tests] Specification of legacy exceptions + tests (#284) * [legacy] Create specification doc for legacy exception handling * [test] Create infra for legacy tests

Add tests for if/then/else with indices + unknown label;

8a256ac

bnjbvr force-pushed the add-test-label-not-found branch from a0c3403 to 080aa4e Compare May 13, 2016 09:09

rossberg reviewed May 13, 2016
View reviewed changes

bnjbvr force-pushed the add-test-label-not-found branch from 080aa4e to e5e2123 Compare May 13, 2016 12:18

rossberg reviewed May 13, 2016
View reviewed changes

bnjbvr force-pushed the add-test-label-not-found branch from e5e2123 to 561169f Compare May 13, 2016 12:25

rossberg reviewed May 13, 2016
View reviewed changes

Refactor runtests.py so that it accepts non zero exit codes and redir…

9223278

…ects output to the right places in all cases;

bnjbvr force-pushed the add-test-label-not-found branch from 561169f to 9223278 Compare May 17, 2016 15:11

rossberg merged commit 040e28e into WebAssembly:master May 19, 2016

bnjbvr deleted the add-test-label-not-found branch January 27, 2017 19:45

dhil pushed a commit to dhil/webassembly-spec that referenced this pull request Oct 3, 2023

Remove the immediate from array.len in binary format (WebAssembly#284)

515282e


		def _runTestFile(self, shortName, fileName, interpreterPath):

		expectedExitCode = 1 if "fail" in fileName else 0

Add tests for if/then/else with indices + unknown label; #284

Add tests for if/then/else with indices + unknown label; #284

Uh oh!

Conversation

bnjbvr commented May 10, 2016

Uh oh!

rossberg commented May 10, 2016

Uh oh!

kripken commented May 10, 2016

Uh oh!

rossberg commented May 10, 2016

Uh oh!

rossberg commented May 12, 2016

Uh oh!

binji commented May 12, 2016

Uh oh!

bnjbvr commented May 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rossberg commented May 13, 2016

Uh oh!

bnjbvr commented May 13, 2016

Uh oh!

rossberg May 13, 2016

Choose a reason for hiding this comment

Uh oh!

bnjbvr May 13, 2016

Choose a reason for hiding this comment

Uh oh!

rossberg commented May 13, 2016

Uh oh!

bnjbvr commented May 13, 2016

Uh oh!

rossberg May 13, 2016

Choose a reason for hiding this comment

Uh oh!

rossberg commented May 13, 2016

Uh oh!

rossberg May 13, 2016

Choose a reason for hiding this comment

Uh oh!

bnjbvr commented May 17, 2016

Uh oh!

rossberg commented May 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bnjbvr commented May 13, 2016 •

edited

Loading