This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add asm.js-style setjmp/longjmp handling for wasm (reland r280302)
ClosedPublic

Authored by aheejin on Sep 1 2016, 12:21 AM.

Details

Summary

This patch adds asm.js-style setjmp/longjmp handling support for WebAssembly. It also uses JavaScript's try and catch mechanism.

Diff Detail

Event Timeline

aheejin updated this revision to Diff 69953.Sep 1 2016, 12:21 AM
aheejin retitled this revision from to [WebAssembly] Add asm.js-style setjmp/longjmp handling for wasm (reland r280302).
aheejin updated this object.
aheejin added reviewers: dschuff, jpp.
aheejin added a comment.EditedSep 1 2016, 12:27 AM

The reason for the test failure in the previous commit was this part:
In lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

if (!EnableEmException) {
  addPass(createLowerInvokePass());
  addPass(createCFGSimplificationPass());
}

Actually, if EH is not supported, LowerInvoke pass runs anyway at later point because TargetPassConfig::addPassesToHandleExceptions adds createLowerInvokePass() to the list of passes. But this runs after all our custom IR passes run, and SjLj handling expects all the invoke instructions to be lowered before processing. So that's the reason I added these two passes when EH is not enabled.(LowerInvoke for lowering invokes to calls, and CFGSimplify for removing orphan landingpad blocks.)

But the problem is even if neither EH nor SjLj is enabled, this runs CFGSimplify all the time, causing several WebAssembly tests to break. I changed this part as follows:

if (!EnableEmException && EnableEmSjLj) {
  addPass(createLowerInvokePass());
  addPass(createCFGSimplificationPass());
}

This way, CFGSimplify only runs when SjLj is enabled.
Confirmed that all tests run.

dschuff edited edge metadata.Sep 1 2016, 9:00 AM

Oh, I had forgotten there's a dedicated target hook for inserting EH handling passes. So probably what we should do is override TargetPassConfig::addPassesToHandleExceptions() and put this pass there instead of in addIRPasses(). Also the default EH-handling pass seems to run only createUnreachableBlockEliminationPass() rather than full CFG simplification. Maybe we should do that instead too? Then it wouldn't interfere with the other tests, and be more consistent with other targets.

aheejin added a comment.EditedSep 1 2016, 9:42 AM

Then should I also add SjLj to that addPassesToHandleException? Currently SjLj does not expect any invokes to exist in the module.
I guess we have to, because anyway EH and SjLj are in the same pass.

Yeah, I think it would be OK to keep them in the same pass, and just move it. We'll want to put some comments to make it clear that it also handles setjmp/longjmp using JS exceptions. That part is somewhat unique.

So then I guess the logic in addPassesToHandleExceptions would be something like

if (!EnableEmException) {
  addPass(createLowerInvokePass());
  addPass(createUnreachableBlockEliminationPass());
}

// Handle exceptions and setjmp/longjmp if enabled.
if (EnableEmException || EnableEmSjLj)
  addPass(createWebAssemblyLowerEmscriptenEHSjLj(EnableEmException,
                                                 EnableEmSjLj))
aheejin added a comment.EditedSep 1 2016, 11:02 AM

In that case, because addPassesToHandleExceptions always runs, createUnreachableBlockEliminationPass() will always run even if neither EH nor SjLj is enabled. And I think this may affect other tests in case there are unreachable blocks in other tests. Shouldn't it be

if (!EnableEmException && EnableEmSjLj) {
  addPass(createLowerInvokePass());
  addPass(createCFGSimplificationPass());
}

As I did in this patch?

aheejin added a comment.EditedSep 1 2016, 11:09 AM

Oh, TargetPassConfig::addPassesToHandleExceptions() is not virtual, so it can't be overriden anyway.

Before this patch, lowerInvoke and UnreachableBlockElimination are always run anyway as the the standard EH passes. So having UBE run unconditionally shouldn't break anything existing.

Hm, that's unfortunate about addPassesToHandleExceptions not being virtual. Maybe we could change that. For now maybe we can go with what you had in the original CL, except with UnreachableBlockElimination instead of CFGSimplify (and add a comment that we need to run it before setjmp/longjmp handing). Then it would run again in addPassesToHandleExceptions which would be redundant when this pass is turned on, but not harmful.

aheejin updated this revision to Diff 70063.Sep 1 2016, 1:30 PM
aheejin edited edge metadata.

Address comments

dschuff accepted this revision.Sep 1 2016, 1:39 PM
dschuff edited edge metadata.
This revision is now accepted and ready to land.Sep 1 2016, 1:39 PM
aheejin closed this revision.Sep 1 2016, 2:13 PM

(tip: you don't have to manually close the code reviews. They will automatically close when a commit lands that references the review.)

I didn't manually close this one. This one was automatically closed when I committed the revision.