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

Only validate memops when there is a memory section #107

Merged
merged 2 commits into from Oct 6, 2015

Conversation

Projects
None yet
4 participants
@lukewagner
Member

lukewagner commented Oct 2, 2015

This PR makes it a validation error to use linear memory operations (load, store, page_size, resize_memory, memory_size) when there is no memory section declared in the module. Since it is possible to have a (memory 0) section (which allows memory ops but doesn't require allocation), there is no loss of expressiveness for producers. Rather, this is a simple a priori way to capture the wasm producer's intent to never use linear memory that allows the impl to take advantage of this (to reduce footprint, avoid corner cases in codegen) without having to worry about a linear memory later appearing (via resize_memory or dynamic linking). This restriction could later be backwards-compatibly relaxed if necessary. The main expected use is apps using GC for all memory.

@titzer

This comment has been minimized.

Show comment
Hide comment
@titzer

titzer Oct 2, 2015

Contributor

We should make this the default, as memory just causes so many problems.

Haha, jk. I like this idea.

On Fri, Oct 2, 2015 at 5:41 PM, Luke Wagner notifications@github.com
wrote:

This PR makes it a validation error to use linear memory operations (load,
store, page_size, resize_memory, memory_size) when there is no memory
section declared in the module. Since it is possible to have a (memory 0)
section (which allows memory ops but doesn't require allocation), there is
no loss of expressiveness for consumers. Rather, this is a simple a priori
way to capture the wasm producer's intent to never use linear memory that
allows the impl to take advantage of this (to reduce footprint, avoid
corner cases in codegen) without having to worry about a linear memory
later appearing (via resize_memory or dynamic linking). This restriction
could later be backwards-compatibly relaxed if necessary. The main expected
use is apps using GC

https://github.com/WebAssembly/design/blob/master/GC.md for all memory.

You can view, comment on, or merge this pull request online at:

#107
Commit Summary

  • Only validate memops when there is a memory section

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#107.

Contributor

titzer commented Oct 2, 2015

We should make this the default, as memory just causes so many problems.

Haha, jk. I like this idea.

On Fri, Oct 2, 2015 at 5:41 PM, Luke Wagner notifications@github.com
wrote:

This PR makes it a validation error to use linear memory operations (load,
store, page_size, resize_memory, memory_size) when there is no memory
section declared in the module. Since it is possible to have a (memory 0)
section (which allows memory ops but doesn't require allocation), there is
no loss of expressiveness for consumers. Rather, this is a simple a priori
way to capture the wasm producer's intent to never use linear memory that
allows the impl to take advantage of this (to reduce footprint, avoid
corner cases in codegen) without having to worry about a linear memory
later appearing (via resize_memory or dynamic linking). This restriction
could later be backwards-compatibly relaxed if necessary. The main expected
use is apps using GC

https://github.com/WebAssembly/design/blob/master/GC.md for all memory.

You can view, comment on, or merge this pull request online at:

#107
Commit Summary

  • Only validate memops when there is a memory section

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#107.

@sunfishcode

This comment has been minimized.

Show comment
Hide comment
@sunfishcode

sunfishcode Oct 2, 2015

Member

lgtm

Member

sunfishcode commented Oct 2, 2015

lgtm

Show outdated Hide outdated ml-proto/spec/eval.ml
@@ -88,6 +88,9 @@ let numerics_error at = function
error at "runtime: invalid conversion to integer"
| exn -> raise exn
let some_memory c =
match c.module_.memory with Some m -> m | _ -> assert false

This comment has been minimized.

@rossberg

rossberg Oct 5, 2015

Member

Can we make this a proper runtime error instead of an assertion failure? As a rule of thumb, always assume that the evaluator can be fed unchecked code. In fact, we even do so for invoke arguments.

@rossberg

rossberg Oct 5, 2015

Member

Can we make this a proper runtime error instead of an assertion failure? As a rule of thumb, always assume that the evaluator can be fed unchecked code. In fact, we even do so for invoke arguments.

This comment has been minimized.

@lukewagner

lukewagner Oct 5, 2015

Member

As brought up in #109, I wonder if we can stop feeding the evaluator unchecked code. In that case, I think it'd be nice if our use of assert in eval specifically meant "checked earlier" and so we had a clear documented distinction between actual runtime errors and things that should never happen, assuming soundness. That is, if someone did a soundness proof, they'd prove we never hit an assert.

@lukewagner

lukewagner Oct 5, 2015

Member

As brought up in #109, I wonder if we can stop feeding the evaluator unchecked code. In that case, I think it'd be nice if our use of assert in eval specifically meant "checked earlier" and so we had a clear documented distinction between actual runtime errors and things that should never happen, assuming soundness. That is, if someone did a soundness proof, they'd prove we never hit an assert.

This comment has been minimized.

@lukewagner

lukewagner Oct 5, 2015

Member

... but I should probably leave that for a separate issue so it can be considered orthogonally; I'll put in the runtime error here for now.

@lukewagner

lukewagner Oct 5, 2015

Member

... but I should probably leave that for a separate issue so it can be considered orthogonally; I'll put in the runtime error here for now.

This comment has been minimized.

@rossberg

rossberg Oct 5, 2015

Member
@rossberg

rossberg via email Oct 5, 2015

Member

This comment has been minimized.

@lukewagner

lukewagner Oct 5, 2015

Member

That sounds good. So maybe a new Soundness exn type? Since this shouldn't ever happen, seems like it could be made more terse to raise and leave out position/string.

@lukewagner

lukewagner Oct 5, 2015

Member

That sounds good. So maybe a new Soundness exn type? Since this shouldn't ever happen, seems like it could be made more terse to raise and leave out position/string.

This comment has been minimized.

@rossberg

rossberg Oct 5, 2015

Member
@rossberg

rossberg via email Oct 5, 2015

Member

This comment has been minimized.

@lukewagner

lukewagner Oct 5, 2015

Member

Not strong reason: just because the source info clutters a bit and while it's generally worth it because it helps us when developing, the Crashes should never happen, even when developing. That's why I liked assert. Since impl=spec (sortof), I do wonder if we need to discriminate bugs in impl and bugs in semantics.

@lukewagner

lukewagner Oct 5, 2015

Member

Not strong reason: just because the source info clutters a bit and while it's generally worth it because it helps us when developing, the Crashes should never happen, even when developing. That's why I liked assert. Since impl=spec (sortof), I do wonder if we need to discriminate bugs in impl and bugs in semantics.

This comment has been minimized.

@rossberg

rossberg Oct 6, 2015

Member
@rossberg

rossberg via email Oct 6, 2015

Member

This comment has been minimized.

@lukewagner

lukewagner Oct 6, 2015

Member

I can see your point and I don't have a strong opinion on this.

@lukewagner

lukewagner Oct 6, 2015

Member

I can see your point and I don't have a strong opinion on this.

@lukewagner

This comment has been minimized.

Show comment
Hide comment
@lukewagner

lukewagner Oct 6, 2015

Member

Merging with comments addressed.

Member

lukewagner commented Oct 6, 2015

Merging with comments addressed.

lukewagner added a commit that referenced this pull request Oct 6, 2015

Merge pull request #107 from WebAssembly/only-allow-memops-with-mem-sec
Only validate memops when there is a memory section

@lukewagner lukewagner merged commit 36ad001 into master Oct 6, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@lukewagner lukewagner deleted the only-allow-memops-with-mem-sec branch Oct 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment