[PATCH] D77936: [Windows SEH] Fix abnormal-exits in _try

[Ten Tzen via Phabricator via cfe-commits]

tentzen marked 2 inline comments as done.
tentzen added inline comments.


================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:848
 
       // Build a switch-out if we need it:
       //   - if there are branch-afters threaded through the scope
----------------
rnk wrote:
> Are we sure this is the right set of predicates? It seems like we'll get the wrong flag in cases like:
> ```
>   __try {
>     mayThrow();
>     goto out;  // AbnormalTermination should be 1
>   } __finally {
>     tearDown();
>   }
> out:
>   return 0;
> ```
> 
> I think these conditions are designed to avoid the switch when there is no normal fallthrough destination (!HasFallthrough). I see clang emits no switch for this C++ input:
> 
> ```
> $ cat t.cpp
> void mayThrow();
> struct Dtor {
>   ~Dtor();
> };
> int f() {
>   do {
>     Dtor o;
>     mayThrow();
>     break;
>   } while (0);
>   return 0;
> }
> 
> $ clang t.cpp  -S -emit-llvm -fno-exceptions -o - | grep switch
> # empty
> ```
Yes, this test passed.

```
  store i32 3, i32* %cleanup.dest.slot, align 4
  %0 = call i8* @llvm.localaddress()
  %cleanup.dest = load i32, i32* %cleanup.dest.slot, align 4
  %1 = icmp ne i32 %cleanup.dest, 0
  %2 = zext i1 %1 to i8
  call void @"?fin$0 at 0@Test84@@"(i8 %2, i8* %0)
  %cleanup.dest1 = load i32, i32* %cleanup.dest.slot, align 4
  switch i32 %cleanup.dest1, label %unreachable [
    i32 3, label %out
  ]
```




================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:863-864
 
+        // pass the abnormal exit flag to Fn (SEH cleanup)
+        cleanupFlags.setHasSehAbnormalExits();
+
----------------
rnk wrote:
> This flag is set for any cleanup that needs to switch-out to multiple normal destinations. You have named this an "seh abnormal exit", an exit that is abnormal for the purposes of SEH, but I think it would be clearer to give the flag a more general name.
> 
> Based on the existing terminology, maybe (F_)HasBranchAfter would be the closest to what we are looking for?
my original implementation in https://github.com/tentzen/llvm-project is still using  (F_)HasBranchAfters.  it's renamed because I'm afraid it may mis-imply it to Scope.getNumBranchAfters(). 

Yes, Agree, it should not be SEH specific.  
how about F_HasExitSwitch?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77936