-
Notifications
You must be signed in to change notification settings - Fork 699
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
wasm2c EH: Move away from alloca #2480
Conversation
src/c-writer.cc
Outdated
// if it does not currently do so. also, it is possible to compile different | ||
// modules with different exception sizes and still link them together. | ||
// (whether you'll run into issues will heavily depend on the use-case.) | ||
Write("if (", tlabel, "_size > WASM_EXN_MAX_SIZE) { TRAP(EXHAUSTION); }"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we perhaps assert this statically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only reason we don't is that we'd prefer the ability to use different WASM_EXN_MAX_SIZE
per module, until we get around to implementing GC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But WASM_EXN_MAX_SIZE
is not variable, its fixed in the header file in source control. I don't think we need to worry about variable WASM_EXN_MAX_SIZE
(at least not until we have to).
How about replacing this with assert( tlabel, "_size <= WASM_EXN_MAX_SIZE)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're gonna do that, we may as well go a little extra. so, how about this?
Can you explain a little more the motivation. How does this "fix the ability to catch exceptions in a loop without the runtime trapping"? |
it's a bit unintuitive, but basically:
(ofc, that's assuming you're using the stack exhaustion handler... if you're using depth count instead, well... it'll just blow the stack.) |
Is this because we are longjmp'ing back to before the alloca? Shouldn't longjump reset the stack pointer in that case to the value it had before the alloca? i.e. doesn't longjump undo alloca just fine? |
... honestly, we don't know what longjmp'ing to an alloca function does. as far as we can tell, the manpage only talks about longjmping over such a function. |
longjump basically resets all the registers to the state they were in when setjmp was called. So if the alloca happened after the setjmp the longjmp will essentially "undo" it by simply resetting the stack pointer. |
if alloca changes the stack pointer then how does a function know where its locals are stored...? (also, y'know, alloca is not standard C, so there's that. also, we can't use alloca with EHv4's exnrefs, which is ultimately the real reason we're doing this.) |
I believe the compiler uses an offset from a frame pointer in this case.
Still I'd quite like to know how exactly the alloca'd region in somehow leaked.. does it leak each up a try/catch or rethrow happens? |
we were under the assumption that longjmp wouldn't reset alloca (because that would be a massive footgun that isn't described in the man page). we suppose we can test it... |
Detailed info on this: https://stackoverflow.com/a/69413437/2770641
|
It seems to me that it would much more of a problem if longjmp treated dynamic and static stack allocation differently. I don't really see how it could possibly reset static stack segments but not dynamic ones since they are all interleaved in the same stack. |
this non-standard |
... y'know what, we think we may be able to come up with a way to implement EHv4 without doing this change. we can rip it out when we implement GC. |
Move away from
alloca
.EHv4
exnrefs
can be stored in locals and globals and passed around,alloca
isn't particularly compatible with this. While this change doesn't introduceexnref
, it paves the way for doing so.Split from #2470