-
Notifications
You must be signed in to change notification settings - Fork 451
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
Handle interpreter stack overflow, and test runaway recursion. #103
Conversation
It appears that while this gets a Stack_overflow exception on Linux, it segfaults on the Travis Mac build. |
712e6c8
to
1cfe99c
Compare
I had added the inner try blocks to provide a more precise source location, but they aren't necessary. I removed them now. This test is deliberately testing that implementations do not do opportunistic TCO, because this optimization is semantically observable (program traps reliably vs program executes reliably in constant space), so it could cause portability problems if some implementations do it and others don't, or don't in the same places. I've now added a comment about this. I don't yet know what to do when OCaml segfaults instead of raising a Stack_overflow exception though. Is this an OCaml bug? |
Hm, some common optimisations amount ot (limited forms of) TCO, e.g., inlining. Not sure what the spec could say exactly to forbid only the general thing. Stack size is going to be implementation-dependent anyway, so I'm not even sure how valuable such an attempt would be. Travis builds the native code version, which doesn't produce SO exceptions: http://caml.inria.fr/pub/docs/manual-ocaml/native.html#s%3Acompat-native-bytecode |
Agreed that it would be hard for the spec to forbid TCO in the MVP. However, if we add stack-walking (something already we've discussed) and specify it deterministically, that may effectively rule out implicit TCO (so that if we want TCO, we'd specify an explicit tail-call op that would have a well-defined effect on the observable stack). |
Makes sense. @sunfishcode: btw, I assume that the Travis failure is gone once you rebase on my Makefile change. |
All the spec needs to say is that implementations have some finite maximum limit and that every call monotonically |
Oops. All the spec needs to say is that implementations have some finite maximum limit and that every call monotonically reduces the distance to that limit. |
@sunfishcode Well, the point @rossberg-chromium was making is that not every call will monotonically reduce the distance. Of course we don't care about every call, just the ones that form call cycles, but specifying that is harder. I agree it's good to simply have the test for now. |
@lukewagner An implementation can't do infinite inlining though, so it can still be thought of as having some finite theoretical limit to the call stack depth. |
1cfe99c
to
4b25268
Compare
Good point. I guess we could model that in ml-proto by having an int counter inc'd by calls and then a host-supplied (in |
I don't know, that would postulate that each implementation has an exact,
deterministic limit, which still doesn't account for inlining. Inlining can
increase the effective limit by arbitrary factors.
Does it really make sense to spec upper bounds on computational resources?
Because that is what this amounts to.
|
It does account for inlining. Putting cycles aside (which is the point), there's only a finite amount of inlining an implementation can do in finite-sized code before there are no callsites left. The practical case here is that we don't want opportunistic TCO to create a situation where apps run in some implementations and not others. Given how much some people want TCO, such a situation may even be fairly likely. I'm not aiming to just withhold TCO from people here -- I do expect that we'll put it in the spec in the future -- but we should do it together so that we can think through how it interacts with stack walking and other features. |
Also, there'd be no practical upper bound on computational resources. The limit itself would be purely theoretical in real-world implementations. There is a limitation here, and it's that one can't write programs that depend on TCO until we add guaranteed TCO to the spec. I expect we'll do that in the future, but it's not part of the plan for the MVP. |
I was having some of the same concerns @rossberg-chromium stated that specifying a single precise (though host-defined) limit would overly constrains impls. In particular, the concern is that user code could in theory first do a probe to find the host limit, and then, knowing that host limit, depend on its precise value for all future executions (which of course would break given the way we want to impl overflow checking). The way I rationalized away this concern is that, as long as we:
then an impl would always be free to fault whenever it wanted. The formal limit would exist only to state ∃ some finite limit. |
This is a resource exhaustion question akin to memory exhaustion (i.e. heap I don't think we can be precise about when stack overflow occurs, but For stack introspection, we'll need to ban implicit TCO, because I can see a couple of uses of stack introspection, such as creation of So maybe it's enough to ban implicit TCO and state that programs that Making the checking deterministic--especially across VMs--will likely be a On Mon, Oct 5, 2015 at 8:43 PM, Luke Wagner notifications@github.com
|
This corrsponds with WebAssembly/spec#103
@titzer I agree, and that's exactly what my PR here does :-). It currently just reports stack overflow whenever the underlying OCaml process hits stack overflow, which leaves room for improvement, but it's a good first step. @rossberg-chromium I rebased on your Makefile change and the Travis failure is now fixed. Thanks! Also, I've now created WebAssembly/design#387 which adds a paragraph about this issue to the design. |
On Mon, Oct 5, 2015 at 10:48 PM, Dan Gohman notifications@github.com
|
This corresponds with WebAssembly/spec#103
I disagree with the spec stating that 'Implementations are not permitted to do implicit tail-call optimizations'. It should just be implementation dependant, and this is enough of a protection against code being written to depend on TCO. I also disagree with the spec stating that 'every call must take up some resources toward exhausting that size'. Some compilers might naturally use TCO and might not consume stack. Some archs pass the return address in a register and might have adequate registers for a particular function so that they do not not need to use the stack on some functions, and they should not be hobbled. By defining this area it creates the same problem it was proposing to solve. Programs might now be written to depend on stack exhaustion! But it's not a big flaw in the spec, and implementations can just ignore it, and code will need to be written to not depend on stack exhaustion, but why not just note this in the spec. |
I agree with what JF suggested. We don't want to ban a particular On Tue, Oct 6, 2015 at 1:26 AM, JSStats notifications@github.com wrote:
|
@titzer Stack walking is not defined yet, and may well be implementation dependant too wrt call optimizations. Perhaps it would be better if stack exhaustion was not even observable to the wasm code, that the code would terminate in a manner not different to the user terminating the program, then there is no 'observability issue'. |
On Tue, Oct 6, 2015 at 1:50 AM, JSStats notifications@github.com wrote:
|
@titzer I don't think it will be possible to spec stack walking so that call optimizations are not observable anyway, not without performance and accounting burden. Has this even been done? |
On Tue, Oct 6, 2015 at 2:14 AM, JSStats notifications@github.com wrote:
|
@titzer That is interesting, but I would like to understand how, to understand any tradeoffs. Could you point me to a JSVM that does this? That is a JSVM that implements functions with no stack usage, keeping the return address in a register, while still allowing recognition of the functions frame when stack walking? Show me a JSVM that implements TCO while still allowing the frame to be recognized? |
@titzer Sorry, I think I misunderstood the discussion. There seems to be agreement that 'call optimizations (e.g. inlining) are not observable' in stack walking, which is what I was (trying) to communicate support for (that they might not be observable), so this seems fine. But then how is that consistent with the patch being discussed here? |
The current quibble is just about tail call optimization being observable when you walk the stack. With stack inspection you'll see a single instance of the function that was tail called, but not its recursion depth. You could spec that such cases show you the function at least once (and maybe more if not optimized) but then there ends up being interesting corner cases with more than just one recursive function that require extra bookkeeping, or just make it hard to do the optimization. It's not impossible, but then you stack inspection becomes not-so-trivial. You could also spec that stack walks are mostly precise, but then we don't have full insight into what developers want it for so the loss of precision may be undesirable (or not!). |
On 5 October 2015 at 22:25, titzer notifications@github.com wrote:
Sure. I think the main discussion here is if and how that could be done at Even for that we need to ban implicit TCO Same problem: how do you specify that without overspecifying? Sure it must I remain skeptical, despite Luke's argument. IMO, it makes more sense at |
@rossberg-chromium Agreed that we don't need to put a requirement in the spec for now; just tests like what's in this PR should hold down the fort until eventually we get to stack inspection and then we'll have to answer the hard questions. |
4b25268
to
9b051cd
Compare
I updated the patch to only handle Stack_overflow on the Eval.invoke path; if there's a stack overflow on the Eval.host_eval path, that's something different. And, I reworded the comment in the testcase to reflect the changes in the wording made in WebAssembly/design#387 . |
9b051cd
to
45673f5
Compare
This needn't be the last word on this topic, but it's useful to have this test in place as guidance for implementation developers. |
Handle interpreter stack overflow, and test runaway recursion.
In WebAssembly#90 it was decided to move the event section between memory section and global section. This change is reflected in the next paragraph, but not in the introduction.
Update the encodings for ref.as_non_null, br_on_null, (ref ht), and (ref null ht) for consistency with the final encodings chosen in WebAssembly/gc#372. Fixes WebAssembly#103.
No description provided.