-
Notifications
You must be signed in to change notification settings - Fork 726
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 partial support for -fwasm-exceptions in Asyncify (#5343) #5475
base: main
Are you sure you want to change the base?
Conversation
0b27b2e
to
6f1e8b5
Compare
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.
Interesting idea! I think this makes sense to do.
6447962
to
5dbbd2c
Compare
Refactored, now I change only top-level catch blocks. Also added lit tests, for me seems correct. Dosbox-x works fine. // br_if case with condition and value
Index newLocal = builder->addVar(func, br->value->type);
auto setLocal = builder->makeLocalSet(newLocal, br->value);
auto getLocal = builder->makeLocalGet(newLocal, br->value->type);
auto condition = br->condition;
br->condition = nullptr;
br->value = getLocal;
auto decIf =
builder->makeIf(condition,
builder->makeSequence(builder->makeDec(), br),
getLocal);
replaceCurrent(builder->makeSequence(setLocal, decIf)); Not sure how to deal if sideeffect will be in |
5dbbd2c
to
3282bfe
Compare
@kripken is there anything preventing this PR from being merged? |
I'm sorry, I want to finish this and add the changes suggested by @kripken, I'll try to find some time to finish it (not sure I can do it anytime soon). Btw I've used it many times and it works great in the case of js-dos. |
Excellent work @caiiiycuk! I look forward to seeing this merged! |
@caiiiycuk I am currently testing your branch and have updated the binaries in
I think the warning is fine. But I am confused as to why |
I just it like this
Did you check, maybe you set flag |
50dc3a6
to
0092287
Compare
7efcd66
to
15bfc21
Compare
@kripken I apologize for the delay. I have made all the changes you requested. Please take a look and let me know if there's anything else to do. |
@kripken friendly ping |
I have been using this for several weeks now and I haven't found any issues yet! Excellent work @caiiiycuk! |
@allsey87 thanks, great to know |
@caiiiycuk Sorry, I've been meaning to get to this but have been busy. Thanks for updating this! There has recently been a suggestion to make some changes in how wasm exceptions works in the spec, see WebAssembly/exception-handling#280 - I have no idea what will happen there, but it might mean that if we land this we'd need to then refactor it significantly. On the positive side, they do mention it is a goal to make things like Asyncify easier, so perhaps this will become simpler actually. Either way, though, I think we should wait on this PR to see what the spec changes are first, to avoid wasted work for all of us. |
@kripken No worries, all good. At least it works for my case. :) |
15bfc21
to
22a248b
Compare
Is there any chance that we can merge this? JSPI still need to take some time... |
The new wasm EH proposal I mentioned above is in the process of being implemented now. The main component is this: Lines 1473 to 1474 in 3049fb8
If we want to land this PR then targeting that would make sense, as the old EH will be removed. However, the new wasm EH is not done yet (e.g. parts of the optimizer don't work on it) so it's too early for that, I'm afraid. |
22a248b
to
6600723
Compare
6600723
to
4e0a009
Compare
Hi. This PR does not do lowering wasm exceptions to MVP as suggested in #5343, so I need to explain situation:
I am trying to port a dosbox/dosbox-x core as is to WebAssembly. The dosbox core is syncrhnous, it runs emulation in forever while loop, events are processed inside this loop. To make this loop asynchrous I use Asyncify, however the unmodified core uses exceptions in certain situations.
Asyncify does not support exceptions, this PR should change the situation in some cases including dosbox/x case.
The main idea of emulation loop is follow:
The catch blocks is used to recover core, or to jump out from some of deep cpudecode calls. There is no situation when emulation loop is called when catch block somewhere in stack:
This also means that catch block can't change the state, and so only try block can. But on other hand try block it self should works fine with current asyncify code. When we are doing rewind we are only visiting try blocks and never catch blocks, so it work out of the box.
For such type of synchronous code we can add exceptions support, we need only verify that unwind didn't start from a catch block (when catch is not in stack). We can validate this requirment and throw exception if it breaks or ignore the unwind call.
This is very small overhead and simple solution to bring exceptions support in Asyncify, more over for some codebases it should be simple to rewrite the synchronous loop to not call unwind from a catch blocks.
This PR is tested on dosbox-x, it works fine.
emscripten-discuss initial context
There is one thing in impelementation that probably is not correct:
I have a global variable that I need to decrease on some amount just before
br_if
instruction,but only when condition is true. I tried lot of other variants but no luck:
Result:
Result:
Just wrap the br_if but save condition result into local var like in 1. Results is sames as in 2.
Completely replace br_if with if and br:
Results:
OR
Block
,Loop
,If
,Drop
,Try
, etc. I don't have straightforward algorithm in my mind.Thanks!