Skip to content
This repository was archived by the owner on Apr 25, 2025. It is now read-only.

Proposed spec changes + rethrow question #125

Closed
aheejin opened this issue Sep 11, 2020 · 49 comments
Closed

Proposed spec changes + rethrow question #125

aheejin opened this issue Sep 11, 2020 · 49 comments

Comments

@aheejin
Copy link
Member

aheejin commented Sep 11, 2020

I did a presentation on why we need some spec changes to be extensible to future two-phase unwinding on 8/18 CG meeting: slides

I pre-uploaded slides on the spec changes I am planning to propose in the next week's 9/15 CG meeting: slides

This is basically the same as what I discussed in #123. The changes consist of:

  • Restore the first version of the proposal
  • Add catch_br instruction
  • Add try-unwind instruction

If you have any feedback or comments, I'd appreciate them.

Also, I'm not sure if we still want the immediate argument of rethrow in the first version of the proposal. rethrow used to have an immediate argument that specifies which exception in the current EH pad stack to rethrow. I don't have a use case for this in C++, but other languages might need it, so if people have any opinions on keep this or not, I'd appreciate that.

@dschuff
Copy link
Member

dschuff commented Sep 11, 2020

I agree that we should do this. The presentation and previous discussion show why, but here's my tl;dr version if it's helpful:

The 2 reasons we originally had for moving to use exnref were 1) Allowing the reference to escape the catch block was the way we solved the unwind mismatch problem, and 2) we thought exnref/event design would extend simply to resumable exceptions. However, as @rossberg developed the concept for resumable exceptions, they ended up being a separate mechanism rather than a simple extension. And, as @aheejin described in her presentation, the way we used exnref to solve unwind mismatches breaks 2-phase unwinding. So that invalidates both of the reasons we had for exnref, and furthermore I think a method without exnref is more likely to be easily compatible/composable with the various options that are currently on the table for stack-switching and other related functionality.

I think getting consensus on switching away from exnref is the critical thing (that's item 1 in @aheejin's list above). The proposed way to solve the unwind mismatch problem is catch_br but the exact name and semantics of that mechanism can be tweaked. And this proposal is also agnostic to the exact way we end up implementing the first phase (it can be straightforwardly extended to include a first phase, but it would also work to use a stack-switching mechanism). So we should be clear that we aren't committing to a mechanism yet.

@dschuff
Copy link
Member

dschuff commented Sep 11, 2020

(also direct ping of some folks who've expressed opinions or participated in discussion previously: @rossberg @RossTate @ioannad @lukewagner @takikawa @fgmccabe @tlively @conrad-watt @KronicDeth @taralx)

@taralx
Copy link

taralx commented Sep 12, 2020

I find the example in the slides a bit confusing. Why would you not move callB out of the catchA block?

@KronicDeth
Copy link

I also am confused the same as @taralx.

I assumed the example was just an oversimplification to fit it all in a slide and there is some pattern where the try can’t surround just the one call.

@aheejin
Copy link
Member Author

aheejin commented Sep 13, 2020

@taralx

I find the example in the slides a bit confusing. Why would you not move callB out of the catchA block?

Yes, that example is an oversimplification for the presentation purpose. There are cases that callB cannot be moved out of the inner catch.

The below is a portion from a real CFG. It looks a little complicated, but other than BB names are long and cumbersome (because it is from a real application), it is not that bad. This is just to show this kind of CFG exists, so people who aren't interested can skip it.

In this example,

  • BBs encleanup54 and ehcleanup59 are EH pads, and catch instruction will be placed in the beginning of them.

  • If any instruction from BB for.body.i.i.i.i.preheader.i.218 throws, we should unwind to ehcleanup59.

  • If any instruction from BB for.body.i.i.i.i.preheader.i.247, for.body.i.i.i.i.preheader.i.283, or _ZN10PixelArrayIhED2Ev.exit163 throws, we should unwind to ehcleanup54.

  • For ehcleanup54's catch instruction, its corresponding try should be placed in _ZN10PixelArrayIhE6ResizeERK16PixelArrayFormathi.exit (the top BB in the picture), because it is the nearest common dominator of all ehcleanup54's predecessors (for.body.i.i.i.i.preheader.i.247, for.body.i.i.i.i.preheader.i.283, and _ZN10PixelArrayIhED2Ev.exit163).

  • ehcleanup59's catch's corresponding try should be placed in _ZN10PixelArrayIhE6ResizeERK16PixelArrayFormathi.exit too, but above ehcleanup54's matching try.

  • The only possible sorting order is for.body.i.i.i.i.preheader.i.218 -> for.body.i.i.i.i.preheader.i.247 -> ehcleanup54 -> ehcleanup59, because of dependence edges.

  • Now the instruction order is

try      ;; _ZN10PixelArrayIhE6ResizeERK16PixelArrayFormathi.exit / matching 'catch' at ehcleanup59
  try    ;; _ZN10PixelArrayIhE6ResizeERK16PixelArrayFormathi.exit / matching 'catch' at ehcleanup54
    ...
    code for for.body.i.i.i.i.preheader.i.218  ;; unwind destination mismatch!
    ...
    code for for.body.i.i.i.i.preheader.i.247
    ...
  catch  ;; ehcleanup54
  end
catch    ;; ehcleanup59
end
  • Now, even though for.body.i.i.i.i.preheader.i.218 should unwind to ehcleanup59, it ends up being caught by ehcleanup54.

This is an example of "unwind mismatch problem".
image

@taralx
Copy link

taralx commented Sep 13, 2020

For ehcleanup54's catch instruction, its corresponding try should be placed in _ZN10PixelArrayIhE6ResizeERK16PixelArrayFormathi.exit (the top BB in the picture), because it is the nearest common dominator of all ehcleanup54's predecessors (for.body.i.i.i.i.preheader.i.247, for.body.i.i.i.i.preheader.i.283, and _ZN10PixelArrayIhED2Ev.exit163).

I don't understand here. While it is true that this is the nearest common dominator, this CFG is irreducible even without the exception paths. So it's going to undergo rewriting, and any decision about where to put the try would have to be made after that rewriting.

Is it possible to distill this into a case with a reducible CFG and exception mismatch?

@aheejin
Copy link
Member Author

aheejin commented Sep 13, 2020

@taralx Can I ask why this is irreducible? Currently our backend, which has our own pass for fixing irreducible CFGs, does not seem to rewrite this. I also tried LLVM's target-independent pass for fixing irreducible CFG and it doesn't seem to change the CFG either.

@taralx
Copy link

taralx commented Sep 13, 2020

You're right, not irreducible. But still seems ok after lowering:

try      ;; _ZN10PixelArrayIhE6ResizeERK16PixelArrayFormathi.exit / matching 'catch' at ehcleanup59
  ... if
    code for for.body.i.i.i.i.preheader.i.218
  end
  try    ;; _ZN10PixelArrayIhE6ResizeERK16PixelArrayFormathi.exit / matching 'catch' at ehcleanup54
    ...
    code for for.body.i.i.i.i.preheader.i.247
    ...
  catch  ;; ehcleanup54
  end
catch    ;; ehcleanup59
end

@aheejin
Copy link
Member Author

aheejin commented Sep 14, 2020

@taralx Not sure what you mean by 'lowering'. encleanup54's corresponding try cannot be placed after for.body.i.i.i.i.preheader.i.218, because the nearest common dominator of all ehcleanup54's predecessors is _ZN10PixelArrayIhE6ResizeERK16PixelArrayFormathi.exit, which has to precede for.body.i.i.i.i.preheader.i.218. Also I'm not sure how the if you placed before for.body.i.i.i.i.preheader.i.218 is supposed to work.

The reason try should be placed in the nearest common dominator BB of all predecessor of an EH pad is, otherwise there will be an incoming edge into the middle of a try. (This is the same for block placement.) In this CFG, suppose we place ehcleanup54's corresponding try right after for.body.i.i.i.i.preheader.i.218, as you suggested. But all three of ehcleanup54's predecessors can be reached without first reaching for.body.i.i.i.i.preheader.i.218, i.e, for.body.i.i.i.i.preheader.i.218 does not dominate ehcleanup54 and its predecessors. Where try is placed should dominate the corresponding EH pad; otherwise there should be an incoming edge that jumps into the middle of a try.

@aheejin
Copy link
Member Author

aheejin commented Sep 14, 2020

cc @backes too

@taralx
Copy link

taralx commented Sep 14, 2020

Here's my problem. If I try to convert your CFG into wasm, I get to this point:

call _ZN10PixelArrayIhE6ResizeERK16PixelArrayFormathi.exit
if
  call invoke.cont
  if
    call for.body.i.i.i.i.preheader.i.i218
    call invoke.cont16.thread
    if
      call if.end.i242.thread

And now what? I said "irreducible" earlier, but that was the wrong word -- it's not in a structured flow form.

@RossTate
Copy link
Contributor

@taralx You can use block and br/br_if to encode this CFG (or any DAG) in wasm (ignoring exceptions).

@aheejin
Copy link
Member Author

aheejin commented Sep 14, 2020

@taralx
As @RossTate said, we currently only generate br/br_if in in LLVM backend and does not use if. This does not necessarily mean the CFG cannot be lowered to wasm using if; I'm just explaining the status quo in our LLVM backend. Other tools, like Binaryen, creates if when necessary for optimization later.

And those long names in the CFG are BB names, and not function names, and each BB can have any number of instructions, including calls. The technique of placing try or block in the nearest common dominator works well when we use br/br_if. One slide in my first presentation gives a taste of what it looks like:
image

The algorithm is called CFG stackification, and the full code is here just in case you are interested, but reading this is not necessary for this discussion at all.

Maybe there exists a complicated general algorithm that can convert all CFGs that causes this mismatch problem so we don't need catch_br. But I don't know what that algorithm is, don't even know if that exists, or it will be compatible with our LLVM backend algorithm. Adding catch_br solves this in an easy way.

@taralx
Copy link

taralx commented Sep 14, 2020

Ah, I see, this is an artifact of how LLVM converts its CFG, not an inherent limitation of the current model in wasm. FWIW, the Dream decompiler has an algorithm that reconstructs structured control flow from an arbitrary CFG. Importantly, it does so by constructing boolean predicates that can be used to linearize complex flows. That's where that if statement came from in my earlier comment.

@aheejin
Copy link
Member Author

aheejin commented Sep 15, 2020

@taralx While our LLVM backend's CFG stackification isn't the only way to make structural control flow, I'm not sure other more powerful algorithms like you pointed out can solve our problem without any spec changes (if we remove exnref). (By the way, Binaryen's Relooper is also an algorithm that takes arbitrary CFG and transforms it into structured code.)

The restriction here is, try-catch and call are much more limited in their descriptive abilities than normal control flow: br, br_if, and if-then-else. Those powerful algorithms make use of direct branches (br and br_if) and ifs, which try-catch don't have. try is a big code block within which all call instructions that throw have to unwind to a specific catch. And unlike branches and ifs, calls don't have destinations specified in them. Where we should unwind to when a call throws is implied by the surrounding try-catch and not directly encoded as in the case of branches.

The currently proposed solution is one way to fix this, and we also think it can be a small addition to the first version of the proposal. But maybe there can be other alternative solutions, such as

  • Create invoke instruction, which takes an unwind destination.
    • Cons: More code size. Also we should make invoke for every single call instruction, such as call, call_indirect, return_call, return_call_indirect, ..
  • Split try and catch, and allow multiple try blocks can target a single catch block in arbitrary location.
    • Cons: More pervasive changes to the current model.
  • ... and possibly more

What I'd like to say is, the unwind mismatch problem itself will very likely to be present even outside of out LLVM backend, as long as someone tries to convert a CFG, basically BB soup, to a linearized wasm. The specific solution we proposed might be more suited to our LLVM backend, but I think other compilers can easily make use of it too.

@fgmccabe
Copy link

@aheejin

More code size. Also we should make invoke for every single call instruction, such as call, call_indirect, return_call, return_call_indirect, ..

To be clear, we are talking about an extra byte for each call instruction.

However, I think there is a stronger argument against this (unfortunately): it would require either a set of completely new call instructions, or it would require invalidating existing code. Neither option seems palatable.

@RossTate
Copy link
Contributor

As a meta point, my understanding is that the most pressing item is to determine if we want to go in the overall direction of these changes. Once that's decided, we can have more focused discussions on specifics (like the various options @aheejin mentions), but first we need to establish the high-level direction.

@aheejin
Copy link
Member Author

aheejin commented Sep 15, 2020

@fgmccabe

However, I think there is a stronger argument against this (unfortunately): it would require either a set of completely new call instructions, or it would require invalidating existing code. Neither option seems palatable.

Yes, that's also one of the concerns I posted.

@RossTate

As a meta point, my understanding is that the most pressing item is to determine if we want to go in the overall direction of these changes. Once that's decided, we can have more focused discussions on specifics (like the various options @aheejin mentions), but first we need to establish the high-level direction.

I listed other options just to illustrate that our proposal wouldn't be the only solution in the world, and while it is possible that other changes can fix this too, they have downsides, and the unwind mismatch problem itself will likely to remain in other toolchain if we don't do anything.

We can still find-tune the changes, but I'm not sure if we should spend more time discussing all those different paths in detail (whether we should introduce invoke, etc) that require more pervasive changes; I hope to settle down to something after today's CG meeting.

@aheejin
Copy link
Member Author

aheejin commented Sep 15, 2020

We decided to make the change (without the immediate argument to rethrow) in 9/15/2020 CG meeting. We can add the immediate argument to rethrow later as an extension if we decide we need it. Thank you very much for everyone for spending long time for discussions and comments. This also closes #123.

@rossberg
Copy link
Member

We decided to make the change (without the immediate argument to rethrow)

Sorry that I couldn't make it to the meeting yesterday. But I strongly suggest to reconsider dropping the immediate, since it creates a language that does no longer compose. Consider a pseudo-code source snippet like

try {
  ...
} catch (e) {
  let result = some_fallback();
  if (is_failure(result)) {
    rethrow e;
  }
}

This is something you would want to write, and it works. But now imagine the failure is itself signalled by an exception. Then you need to be able to write:

try {
  ...
} catch (e) {
  try {
    some_fallback();
  } catch (e2) {
    rethrow e;
  }
}

This would no longer translate.

Such failures of composability of nestable constructs due to implicit naming are a common mistake in programming languages. Please let's not repeat that old mistake!

@RossTate
Copy link
Contributor

It's still unclear what the use cases for rethrow are. For example, C++ cannot use it for compiling throw; because that can occur within a function call. Different languages have different semantics for rethrowing, and so it's unclear how a single rethrow instruction can capture all these semantics. So we should investigate the use cases further before discussing whether or not it needs an immediate.

@rossberg
Copy link
Member

We should make that an orthogonal discussion. As long as the instruction exists, the reference to the catch should be explicit.

@RossTate
Copy link
Contributor

Makes sense.

@aheejin
Copy link
Member Author

aheejin commented Sep 16, 2020

We didn't include it in yesterday's poll, but we can certainly add it if we need it. It was more like we didn't include that in the poll than we decided to drop it. C++ does not need that, but as @rossberg pointed out, other languages might. We can discuss this in a separate issue.

@RossTate
Copy link
Contributor

RossTate commented Oct 6, 2020

I'm noticing that catch_br is adding a lot of complexity (and, with it, questions). I understand it's being added as one way to address the unwind-mismatch problem. @aheejin gives a good example of this above. But something occurred to me. What if this problem is the exception-analog to irreducible CFGs? Both are rare, and both are addressed by duplicating code in these rare cases. Now I don't have a strong opinion about irreducible CFGs, but I do know how to add them to wasm, and a similar solution would work for addressing the unwind-mismatch problem without the complexities of catch_br. But at the moment, I wonder if we are adding an unnecessarily complicated (and concerningly unexplored, at least in the literature) semantic construct.

@aheejin
Copy link
Member Author

aheejin commented Oct 6, 2020

I don't think we can apply an algorithm similar to fixing irreducible CFG to this problem, and I haven't been able to come up with a general transformation algorithm that solves this problem. Unless we have an algorithm at hand, I'm not sure what you're suggesting. Also, even if we come up with some complicated algorithm someday later, I don't think we necessarily need to remove it. As I said in the discussions on rethrow, I don't think wasm should be a minimal set of instructions that are absolutely necessary to comprise a program; if an instruction makes either speed faster, or code size smaller, or code generation easier (this also means faster speed and smaller code size), I think that's worth it.

And also, I don't think it is "adding a lot of complexity and questions" as you suggested; the confusion was whether it targets unwind or not, and it was answered.

@RossTate
Copy link
Contributor

RossTate commented Oct 6, 2020

As you point out above, the issue comes up when the nearest common dominator of the blocks that a catch/unwind applies to also covers a block that the catch/unwind must not apply to. When that happens, duplicate the catch/unwind code for each successor of that dominator that has applicable blocks. Repeat as needed. I believe this algorithm turns any CFG with unwinding mismatches into a CFG without any unwinding mismatch.

@aheejin
Copy link
Member Author

aheejin commented Oct 6, 2020

@RossTate I don't understand how that would work. Suppose there's a big try(dominator) and in the middle of that there's a call that has mismatched unwind destination. It is not clear how to extract a combination of all conditions to reach that call from that dominator. I don't think it can be done as a mechanical and simple way as we fix irreducible CFGs.

And even if it can be done in some way, unwind mismatches are rare but not that rare. I've seen dozens of it within a single big file in a real application. One try can involve literally hundreds or even more blocks, and it can have any number of mismatched calls. If you are suggesting to duplicate the whole hundreds of blocks by the number of mismatched calls, I'm not sure if it is a good strategy.

I'm not saying there cannot ever exist an algorithm that can do this transformation. There may be. But that algorithm will be undoubtedly complicated and involve a lot of code duplication. I think this is a sufficient reason to have catch_br.

@RossTate
Copy link
Contributor

RossTate commented Oct 9, 2020

Just to clarify, my thought experiment only requires duplicating the catch/unwind code, not the main body code. Destructors are already duplicated with the understanding that such code is typically small. So I was curious if the problem could be solved by providing a syntactic construct for sharing code across handlers rather than a control construct for jumping across handlers.

@taralx
Copy link

taralx commented Oct 10, 2020

catch_br is a syntactic construct, isn't it? It replaces the catch with a pointer to an outer catch instead.

@RossTate
Copy link
Contributor

Apparently it is a new control construct.

@aheejin
Copy link
Member Author

aheejin commented Oct 10, 2020

@RossTate Can you clarify how we can duplicate catch blocks to solve unwind mismatch problem?

@tlively
Copy link
Member

tlively commented Oct 10, 2020

Apparently it is a new control construct.

Yes, and to elaborate on why, consider the following:

try $outer
  try $mid
    try $inner
      throw
    delegate $outer
  unwind ;; mid
    instrMid*
  end
unwind ;; outer
  nop
end

If this were a purely syntactic construct the way @RossTate means that, the delegate $outer would be a syntax sugar for deduplicating an unwind clause shared by both $inner and $outer. If that were the case, running this code would run instrMid* because delegate $outer would just run nop then propagate the exception from there.

However, delegate $outer is not just a syntax sugar used for deduplication. Instead, when this code is run, delegate $outer transfers control to $outer's unwind block, skipping over and not running $mid's unwind block, so instrMid* is never run.

@RossTate
Copy link
Contributor

Thanks @tlively. And there's more discussion in #130. (Sorry, I replied to @taralx on my phone and thought we were on that thread.)

Can you clarify how we can duplicate catch blocks to solve unwind mismatch problem?

Happy to! Let's consider the useful concrete example you found for us above:
Unwinding-Mismatch Example

If you triplicate ehcleanup54 (and, if I understand correctly, its two children), then you could put a try directly around each of ehcleanup54's predecessors.

So one way to modify CFGStackify is to have placeTryMarker(MBB) first check if placing a single try marker around all the predecessors of MBB would cause an unwinding mismatch. If not, then place the marker. But if it does, then duplicate MBB, switch some of the predecessors to use the duplicate instead, and then try again.

@taralx
Copy link

taralx commented Oct 10, 2020

This makes sense. I'm not sure where the code of the handler would live, but "range splitting" the try does make sense, and does avoid the "jump over" which has more complex semantics.

@aheejin
Copy link
Member Author

aheejin commented Oct 12, 2020

@RossTate That might work. I think we considered something similar briefly 3 years ago when we first discovered the unwind mismatch problem but dismissed it quickly out of concerns on code size increase. In this particular example we only need to duplicate a small portion, but it can be a large catch block in other cases, which we need to duplicate many times. I still prefer to keep delegate, because it does not cause any code size increase and makes code generation much easier. Also, if we remove this, we impose all future toolchains that target wasm do the same nontrivial thing.

I'm little curious how much the code size increase caused by this will be in many real world programs, and I may want to experiment on that later, but removing this at the moment seems risky to me. Also, your concerns on this instruction don't sound very concrete at the moment, and you mostly talked about hypothetical uncertainties wrt your future instructions. I'd like to keep the same argument as I did for rethrow; I don't think the proposal should be the canonical and minimal set of instructions without which a program cannot be constructed. (In case of rethrow, I actually think is is in that canonical set though.)

@RossTate
Copy link
Contributor

Remember that a catch block in C++ is not the same as a catch block in wasm. The content of the catch block in wasm just needs to be the code that checks if the exception is meant to be caught, i.e. the filter code. If so, it branches to the relevant label ideally outside the catch block to indicate that unwinding is done and the (hidden) stack trace and exception-payload copy are no longer needed. So you wouldn't need to duplicate the body of a C++ catch block; you would just duplicate the filter code that's run before the body of the catch block.

Having looked at the code you need to change, I am not so sure that delegate is easier than the above change. delegate introduces a new kind of mismatch problem that you will have to fix in addition to the current mismatch problem.

I understand that redundancies across instructions are fine. But every instruction is additional implementation and testing complexity, especially so when it involves control and so can be particularly dangerous from a browser-security perspective. So culling the instruction set can help get the feature out the door quicker.

@aheejin
Copy link
Member Author

aheejin commented Oct 14, 2020

@RossTate

Remember that a catch block in C++ is not the same as a catch block in wasm. The content of the catch block in wasm just needs to be the code that checks if the exception is meant to be caught, i.e. the filter code.

Not really. rethrow can occur anywhere within the handler and we can't take that out of catch. Please don't start again on why this is the reason we should remove rethrow too. Also, contents of unwind blocks cannot be extracted.

Having looked at the code you need to change, I am not so sure that delegate is easier than the above change.

No, it is... easier to have delegate.

delegate introduces a new kind of mismatch problem that you will have to fix in addition to the current mismatch problem.

What is the new mismatch problem you are talking about?

I understand that redundancies across instructions are fine. But every instruction is additional implementation and testing complexity, especially so when it involves control and so can be particularly dangerous from a browser-security perspective. So culling the instruction set can help get the feature out the door quicker.

You've been saying the same thing for every single instruction that you are not going to use or are not interested in. Even now, you are trying remove rethrow, catch_all, and delegate. At this point I have an impression that you are trying to remove every instruction you don't need for yourself. This proposal is not only for your personal use.

Apparently other people, including VM developers, haven't expressed concerns with this so far, and you've been the only person arguing this is a serious security problem. So I'm curious "get the feature out the door quicker" means. Does that mean you won't let it get out the door unless I remove instructions you are telling me to remove?

@aheejin
Copy link
Member Author

aheejin commented Oct 15, 2020

@RossTate Can you answer this question?

delegate introduces a new kind of mismatch problem that you will have to fix in addition to the current mismatch problem.

What is the new mismatch problem you are talking about?

@RossTate
Copy link
Contributor

What is the new mismatch problem you are talking about?

If I understand the code correctly, fixUnwindMismatches patches calls that are unwinding to the wrong unwinder. These are call-unwind mismatches. But you also have to worry about unwind-unwind mismatches. That is, after an unwinder completes, the unwinding process will continue to another unwinder/catcher, and you need to make sure that's the correct one. To patch that, you'll have to wrap the whole try/unwind or try/catch with a try/delegate, which means that previously checked function calls in the try portion might now unwind to the wrong place with the new delegate you inserted. There's an algorithm for doing this all in the right order, but it seems more complicated than the algorithm I suggested above where you make sure to generate correct unwinding paths in the first place.

As for the concern about size caused by code-duplication, what I was pointing out is that the code in unwind and in (wasm-)catch will be small. For unwind, it is already the case that this code has been deemed small enough to duplicate it for each exit path (since that is what LLVM does for local exit paths). For wasm-catch, I was pointing out that it's not the code in the C++-catch that needs to be duplicated; it is just the filter code that happens before the C++ catch body is entered, which will be small.

@aheejin
Copy link
Member Author

aheejin commented Oct 15, 2020

@RossTate

To patch that, you'll have to wrap the whole try/unwind or try/catch with a try/delegate, which means that previously checked function calls in the try portion might now unwind to the wrong place with the new delegate you inserted.

I understand (and was aware of) the former part that we need to insert a new try-delegate in that case, but I don't understand "previous checked function calls in the try portion might now unwind to the wrong place with the new delegate you inserted". Can you provide an example?

For wasm-catch, I was pointing out that it's not the code in the C++-catch that needs to be duplicated; it is just the filter code that happens before the C++ catch body is entered, which will be small.

Did you read my response above? rethrow can occur anywhere within the handler and we can't take that out of catch.

@RossTate
Copy link
Contributor

Did you read my response above? rethrow can occur anywhere within the handler and we can't take that out of catch.

I did. My point was that the handler is small as it is just the filter code, so it's fine to duplicate it with rethrow and all. Or do you mean to say that a C++ throw; can happen anywhere within a C++ catch block and so cannot be factored out?

@aheejin
Copy link
Member Author

aheejin commented Oct 15, 2020

@RossTate

I did. My point was that the handler is small as it is just the filter code, so it's fine to duplicate it with rethrow and all.

rethrow cannot occur outside of catch, so it cannot be extracted.

To patch that, you'll have to wrap the whole try/unwind or try/catch with a try/delegate, which means that previously checked function calls in the try portion might now unwind to the wrong place with the new delegate you inserted.

I understand (and was aware of) the former part that we need to insert a new try-delegate in that case, but I don't understand "previous checked function calls in the try portion might now unwind to the wrong place with the new delegate you inserted". Can you provide an example?

Can you answer this question too?

@RossTate
Copy link
Contributor

rethrow cannot occur outside of catch, so it cannot be extracted.

I am not saying it needs to be extracted. Duplicate the entire catch clause, including the rethrows. It is small.

Working on the other question.

@aheejin
Copy link
Member Author

aheejin commented Oct 15, 2020

@RossTate You might be able to say unwind or cleanup code tends to be smaller, but I don't think you can say catches are small..? They are user handlers and we don't know what it takes.

@RossTate
Copy link
Contributor

Given C++ code:

before
try {
  try-body
} catch (SomeException e) {
  catch-body
}
after

this should presumably compile to roughly

before
(block $outer
  (block $caught
    (try
      try-body
      br $outer
    catch $__cpp_exception
      (if (payload matches SomeException personality type)
      then (br $caught)
      else rethrow
      )
    )
  ) ;; $caught
  catch-body
) ;; $outer
after

Note that none of the user code is inside the wasm-catch body. The wasm-catch body is small as it is just the filter code.

@aheejin
Copy link
Member Author

aheejin commented Oct 15, 2020

@RossTate

  1. There are cases users use rethrow themselves (not in C++, but in other languages), in which we can't predict where that to occur. And this unwind mismatch problem is common to all CFG-using compilers, not only C++.

  2. Even in C++, code checking for that is not as short as one line if. There can be multiple catch clauses, and usually it takes one basic block with several instructions to check for one condition and branch to the next basic block, in which we check for the next condition. So the number of instruction is going to be that times the number of clauses. (This doesn't mean we inline the personality function; we call it, but still it takes multiple basic blocks)

@RossTate
Copy link
Contributor

There are cases users use rethrow themselves (not in C++, but in other languages), in which we can't predict where that to occur.

I have investigated a number of languages and found they do not need rethrow (except at the end).

And this unwind mismatch problem is common to all CFG-using compilers, not only C++.

And the algorithm I am suggesting would work just as well for them.

Even in C++, code checking for that is not as short as one line if.

Certainly. But we are already designing under the expectation that catch is infrequent, and within those cases unwind mismatches are uncommon, and within those cases it seems uncommon to have a large multi-catch code in the block. (Your example above seems to be unwinding and not catching, for example.) So it seems unlikely that there will be a particularly meaningful increase in code size. In prior situations where people have suggested even a simple extension for code-size reduction, let alone an entire new control construct like delegate, they have been asked to provide evidence that the complexity this would introduce to the process and implementations would actually be worth the code-size reduction.

@aheejin
Copy link
Member Author

aheejin commented Oct 15, 2020

@RossTate

There are cases users use rethrow themselves (not in C++, but in other languages), in which we can't predict where that to occur.

I have investigated a number of languages and found they do not need rethrow (except at the end).

I think that was your conclusion; there are languages that certainly preserve stack traces when rethrown. You said some of them don't preserve it consistently, but that doesn't mean we should remove stack traces altogether. Usually they are auxiliary info and people want to have them for their debugging.

Also as I pointed out in multiple issues, rethrow is the only way to rethrow within catch_all.

Certainly. But we are already designing under the expectation that catch is infrequent, and within those cases unwind mismatches are uncommon, and within those cases it seems uncommon to have a large multi-catch code in the block. (Your example above seems to be unwinding and not catching, for example.) So it seems unlikely that there will be a particularly meaningful increase in code size. In prior situations where people have suggested even a simple extension for code-size reduction, let alone an entire new control construct like delegate, they have been asked to provide evidence that the complexity this would introduce to the process and implementations would actually be worth the code-size reduction.

Currently you are the only person arguing this is a security threat, and I asked many stakeholders, including VM developers, about this instruction and they didn't express concerns. This will increase code size and also code generation complexity. We don't have a data on exactly how many percents, and I don't think you do either. Stakeholders and VM developers agreed to this proposal and the CG passed it, so I think you should provide enough evidence that the code size increase will be negligible in real world programs to overturn this. Also, I don't think you convinced people that this is a security threat as you keep suggesting, which you also did for all other instructions you want to remove.

To patch that, you'll have to wrap the whole try/unwind or try/catch with a try/delegate, which means that previously checked function calls in the try portion might now unwind to the wrong place with the new delegate you inserted.

I understand (and was aware of) the former part that we need to insert a new try-delegate in that case, but I don't understand "previous checked function calls in the try portion might now unwind to the wrong place with the new delegate you inserted". Can you provide an example?

Are you going to answer this question?

pull bot pushed a commit to wenyuzhao/v8 that referenced this issue Dec 3, 2020
First step towards the new exception handling proposal:
WebAssembly/exception-handling#125

This is essentially a revert of:
"[wasm] Switch to new 'catch' and 'br_on_exn' proposal."

The changes are:
- "catch" instruction takes a tag immediate,
- "rethrow" instruction takes a label immediate,
- Add "catch_all" instruction,
- Remove "br_on_exn" instruction,
- Do not push exceptions on the stack, only the encoded values

[email protected]
CC=​[email protected]

Bug: v8:8091
Change-Id: Iea4d8d5a5d3ad50693f645e93c13e8de117aa884
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2484514
Commit-Queue: Thibaud Michaud <[email protected]>
Reviewed-by: Clemens Backes <[email protected]>
Cr-Commit-Position: refs/heads/master@{#71602}
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants