[PATCH] D94049: [WebAssembly] Fix catch unwind mismatches

[Thomas Lively via Phabricator via llvm-commits]

tlively accepted this revision.
tlively added a comment.
This revision is now accepted and ready to land.

Sorry for dropping this for such a long time!



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:905-906
+    // put the 'delegate' BB in between. We normally create a split BB and make
+    // it a successor of the original BB (PostSplit == true), but in case the BB
+    // is an EH pad and the split pos is before 'catch', we should preserve the
+    // BB's property, including that it is an EH pad, in the later part of the
----------------
aheejin wrote:
> tlively wrote:
> > What is an example of when the split position could be before `catch`?
> When fixing catch mismatches. When an inner try is included in a body of an outer try:
> ```
> bb0:
>   try
>     try
>       ...
> bb1 (ehpad):
>     catch ...
>       ...
> bb2: (ehpad)
>     end_try
>   catch ...
>     ...
> ```
> 
> Suppose we need to wrap the inner try with `try`-`delegate`:
> ```
> bb0:
>   try
>     try          ;; new inst
>       try
>         ...
> bb1 (ehpad):
>       catch ...
>         ...
> bb-pre: (ehpad)  ;; new BB
>       end_try
> bb-delegate:     ;; new BB
>     delegate    ;; new inst
> bb2 (ehpad):
>   catch ...
>     ...
> ```
> 
> The split pos is before the `catch` in bb2. Because `catch`'s BB should be preserved, because that's what `WasmEHFuncInfo` remembers.
I see, I was thinking that `catch` would always be at the beginning of a BB, but I hadn't considered that various end markers may precede it.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:1315-1316
+        // anything
+        if (MI.getOpcode() == WebAssembly::CATCH_ALL)
+          ;
+
----------------
aheejin wrote:
> tlively wrote:
> > ~Is this supposed to be a `continue`?~ Oh, I see it's just there in service of the `else if`s below. I found this confusing when I first saw it (especially without braces). I think it would be clearer if structured a different way.
> The reason this is not a `continue` is it needs to run the common code at the very bottom:
> ```
> EHPadStack.push_back(EHPad);
> ```
> I can add the braces. Do you have any other restructuring suggestions?
No, this looks clear now. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94049/new/

https://reviews.llvm.org/D94049