Page MenuHomePhabricator

[WebAssembly] Restore __stack_pointer after catch instructions
ClosedPublic

Authored by aheejin on Aug 20 2018, 10:41 AM.

Details

Summary

After the stack is unwound due to a thrown exception, the
__stack_pointer global can point to an invalid address. This inserts
instructions that restore __stack_pointer global.

Diff Detail

Event Timeline

aheejin created this revision.Aug 20 2018, 10:41 AM
lib/Target/WebAssembly/WebAssemblyEHRestoreStackPointer.cpp
59

This is kinda awkwardly indented, was this clang-formatted?
Might be worth extracting a temporary variable here for the getExceptionHandlingType() call, i.e. auto EHType = MF.getTarget().getMCAsmInfo()->getExceptionHandlingType();, which should make the line-breaks more consistent.

... Reading further down this patch, can we just call needsPrologForEH here?

dschuff added inline comments.Aug 20 2018, 3:36 PM
lib/Target/WebAssembly/WebAssemblyEHRestoreStackPointer.cpp
74

This comment is a little confusing. Looking at the code, I guess it means we just always insert the fixup at the top of the BB, (but after the catch if it hasn't been moved)?
What prevents the SP restoration from being moved after a call in the catch block?

lib/Target/WebAssembly/WebAssemblyFrameLowering.h
49

Since SP is now a global and not a memory address anymore, we should change this function name. I realize that's pre-existing for this change, so it can be in a different CL (either before or after this one, whatever is convenient).

aheejin updated this revision to Diff 161753.Aug 21 2018, 10:18 AM
aheejin marked 2 inline comments as done.

Address comments

lib/Target/WebAssembly/WebAssemblyEHRestoreStackPointer.cpp
59

This is kinda awkwardly indented, was this clang-formatted?

Yes it was.

... Reading further down this patch, can we just call needsPrologForEH here?

Good idea. Done.

Might be worth extracting a temporary variable here for the getExceptionHandlingType() call, i.e. auto EHType = MF.getTarget().getMCAsmInfo()->getExceptionHandlingType();, which should make the line-breaks more consistent.

Done in WebAssemblyFrameLowering::needsPrologForEH.

74

This comment is a little confusing. Looking at the code, I guess it means we just always insert the fixup at the top of the BB, (but after the catch if it hasn't been moved)?

Yes that's what it tries to do. Do you have any suggestions on making it less confusing? What I tried to say is, at this point, we have catch by this point but not catch_all (it will be generated later in LateEHPrepare). And even if we have catch, it may not be staying at the top. But here we don't want to go out of the way to find the catch instruction and generate these instructions after it because it will be done in LateEHPrepare anyway.

What prevents the SP restoration from being moved after a call in the catch block?

I'm not exactly sure about this. What prevents normal epilogues generated by WebAssemblyFrameLowering::emitEpilogue being moved after a call within the same BB?

lib/Target/WebAssembly/WebAssemblyFrameLowering.h
49

Done in D51046, which should land before this one.

dschuff added inline comments.Aug 21 2018, 11:21 AM
lib/Target/WebAssembly/WebAssemblyEHRestoreStackPointer.cpp
74

Stepping back, why can catch instructions get reordered? It seems odd that they should be if we model them as having side effects or control flow or whatever.

Stepping back, if we move catches later, and insert catch_all later, is there a reason we even need to put the catch in the BB early? Why not just add them when we add catch_all? Is it just because we ISel it from catchpad? I guess if we could avoid reordering, then it wouldn't be so confusing.

I'm not exactly sure about this. What prevents normal epilogues generated by WebAssemblyFrameLowering::emitEpilogue being moved after a call within the same BB?

I think actually what happens is that calls are modeled as modifying SP, so that is ordered wrt anything else that modifies SP. So maybe that should just work for us too

aheejin added inline comments.Aug 21 2018, 1:22 PM
lib/Target/WebAssembly/WebAssemblyEHRestoreStackPointer.cpp
74

Stepping back, why can catch instructions get reordered? It seems odd that they should be if we model them as having side effects or control flow or whatever.

It is currently marked as hasCtrlDep, but I still have noticed cases that catches are reordered (mostly something like i32.const is hoisted before catch). I added hasSideEffects in D50919 just to be safer. But even with that I don't think we can be sure instructions like i32.const will not be reordered before catch, or something like this won't happen:

ehpad:
  br bb2

bb3:
  catch

So LateEHPrepare even considers cases like the latter and make sure catch will be the first instruction of an EH pad.

Stepping back, if we move catches later, and insert catch_all later, is there a reason we even need to put the catch in the BB early? Why not just add them when we add catch_all? Is it just because we ISel it from catchpad? I guess if we could avoid reordering, then it wouldn't be so confusing.

The reason we select catch in the isel stage is because unlike catch_all, it produces a value. Instructions below catch use the value produced by the catch.

dschuff accepted this revision.Aug 21 2018, 1:49 PM
dschuff added inline comments.
lib/Target/WebAssembly/WebAssemblyEHRestoreStackPointer.cpp
74

So LateEHPrepare even considers cases like the latter and make sure catch will be the first instruction of an EH pad.

Hm, interesting.
In the latter case, I guess we'd end up with control flow inside the catch block, which is OK even when there are instructions in the first BB.

So getting back to this code, I guess it's not actually necessary to move the insert point below catch, and it makes the explanation funny, but OTOH it probably results in more readable IR in the common case.
I guess the comment could be something like:
"Insert __stack_pointer restoring instructions at the beginning of each EH pad, after the catch instruction. (Catch instructions may have been reordered, and catch_all instructions have not been inserted yet, but those cases are handled in LateEHPrepare)."
That's not much better than what you had, but it does state the simple case first, followed by the extra explanation, which IMO is a little simpler to read.

This revision is now accepted and ready to land.Aug 21 2018, 1:49 PM
aheejin updated this revision to Diff 161817.Aug 21 2018, 2:19 PM

Fix comments

lib/Target/WebAssembly/WebAssemblyEHRestoreStackPointer.cpp
74

Yes, it's not strictly necessary to move the insert point below catch. I did it because as you said it's gonna make code more readable in common cases. Fixed comments; I agree this is simpler and easier to understand. Thanks.

This revision was automatically updated to reflect the committed changes.