Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing finalize() call #1184

Merged
merged 3 commits into from
Sep 14, 2017
Merged

Conversation

pepyakin
Copy link
Contributor

Looks like finalize call is missing in BinaryenCallIndirect.

This may cause validation fail in the case when target and/or operands have the unreachable type.

[wasm-validator error in function $test] 1 != 2: function body type must match, if function returns, on
[i64] (call_indirect $return_i64
 [unreachable] (unreachable)
)
(module
 (type $0 (func (result i32)))
 (type $return_i64 (func (result i64)))
 (memory $0 0)
 (func $test (type $0) (result i32)
  (call_indirect $return_i64
   (unreachable)
  )
 )
)

@kripken
Copy link
Member

kripken commented Sep 13, 2017

Thanks, this looks right. Let's also add a test. You can add to one of the files in test/example, like maybe the kitchen sink test (can search for where it creates a node of this type). Then run ./auto_update_tests.py and it will update the test outputs for you.

@pepyakin
Copy link
Contributor Author

pepyakin commented Sep 14, 2017

Done!

As side question: I'm on macOS, and it looks like neither ./check.py nor ./auto_update_tests.py are working, because macOS waterfall builds.

Expand `./check.py` output
$ ./check.py                                                                                                                                                                             
(downloading waterfall 13645: https://storage.googleapis.com/wasm-llvm/builds/mac/13645/wasm-binaries-13645.tbz2)
Traceback (most recent call last):
  File "./check.py", line 24, in <module>
    from scripts.test.shared import (
  File "/Users/pepyakin/dev/etc/binaryen/scripts/test/shared.py", line 249, in <module>
    fetch_waterfall()
  File "/Users/pepyakin/dev/etc/binaryen/scripts/test/shared.py", line 217, in fetch_waterfall
    downloaded = urllib2.urlopen(url).read().strip()
  File "/usr/local/Cellar/python/2.7.9/Frameworks/Python.framework/Versions/2.7/lib/python2.7/urllib2.py", line 154, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/local/Cellar/python/2.7.9/Frameworks/Python.framework/Versions/2.7/lib/python2.7/urllib2.py", line 437, in open
    response = meth(req, response)
  File "/usr/local/Cellar/python/2.7.9/Frameworks/Python.framework/Versions/2.7/lib/python2.7/urllib2.py", line 550, in http_response
    'http', request, response, code, msg, hdrs)
  File "/usr/local/Cellar/python/2.7.9/Frameworks/Python.framework/Versions/2.7/lib/python2.7/urllib2.py", line 475, in error
    return self._call_chain(*args)
  File "/usr/local/Cellar/python/2.7.9/Frameworks/Python.framework/Versions/2.7/lib/python2.7/urllib2.py", line 409, in _call_chain
    result = func(*args)
  File "/usr/local/Cellar/python/2.7.9/Frameworks/Python.framework/Versions/2.7/lib/python2.7/urllib2.py", line 558, in http_error_default
    raise HTTPError(req.get_full_url(), code, msg, hdrs, fp)
urllib2.HTTPError: HTTP Error 404: Not Found

Because of this I had to write my own Dockerfile. I'm wondering if it is worth to check-in that Dockerfile for those like me?

@kripken
Copy link
Member

kripken commented Sep 14, 2017

Hmm, you can run check and auto-update with --no-test-waterfall to work around that. But it's odd it fails. @dschuff, what's the status of the waterfall - is it expected to work on non-linux platforms?

Dockerfile looks like it might be useful to check in, yeah. Is there a convention for such things, like what directory they go in, etc?

@kripken kripken merged commit 9f4e328 into WebAssembly:master Sep 14, 2017
@pepyakin
Copy link
Contributor Author

Hmm, you can run check and auto-update with --no-test-waterfall to work around that.

Indeed, --no-test-waterfall helped, thanks! But there is another issue popped up.

Dockerfile looks like it might be useful to check in, yeah. Is there a convention for such things, like what directory they go in, etc?

I'm not an expert in such kind of things. Example that I know of is rust, but they use docker along with Travis CI.

@pepyakin pepyakin deleted the fix-call-indirect branch September 14, 2017 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants