[Windows SEH] Fix abnormal-exits in _try

[tentzen]

Summary

Per Windows SEH Spec, except _leave, all other early exits of a _try (goto/return/continue/break) are considered abnormal exits. In those cases, the first parameter passes to its _finally funclet should be TRUE to indicate an abnormal-termination.

One way to implement abnormal exits in _try is to invoke Windows runtime _local_unwind() (MSVC approach) that will invoke _dtor funclet where abnormal-termination flag is always TRUE when calling _finally. Obviously this approach is less optimal and is complicated to implement in Clang.

Clang today has a NormalCleanupDestSlot mechanism to dispatch multiple exits at the end of _try. Since _leave (or try-end fall-through) is always Indexed with 0 in that NormalCleanupDestSlot, this fix takes the advantage of that mechanism and just passes NormalCleanupDest ID as 1st Arg to _finally.

[efriedma]

Needs a testcase in clang/test/CodeGen to verify the generated IR.

I haven't looked at this code in a while, but this looks reasonable.

clang/lib/CodeGen/EHScopeStack.h

164

Please add a trailing comma here.

[efriedma]

clang/lib/CodeGen/CGException.cpp

1651

Is just truncating the value really correct? What's the possible range of values stored in getNormalCleanupDestSlot()?

[tentzen]

Per Eli's feedbacks:
(1) a test case (windows-seh-abnormal-exit.c) is added under clang\test\CodeGen directory.
(2) an assert is added to catch the extremely rare case that the number of JumpDests in a function is greater than 255. The max. number of JumpDests is 1 (return) + 2*M + N, where M is the number of for/while-loops and N is the number of Goto targets.

[tentzen]

Add option -fms-extensions in test case

[efriedma]

Instead of asserting there are less than 256 cleanup destinations, can you emit an icmp against zero, or something like that?

[tentzen]

In D77936#1976938, @efriedma wrote:

Instead of asserting there are less than 256 cleanup destinations, can you emit an icmp against zero, or something like that?

I did consider that. but it splits the block and induces one compare and one branch in _try exit sequence for something that will not happen in RWC. And it's hard for Optimizer to deal with it. Optimizer can probably can tail-duplicate each path at the expense of code size which I don't think LLVM would do today. Do you really think it's worth to split the block here? thanks

clang/lib/CodeGen/CGException.cpp

1651

Good question!
Usually the number in NormalCleanupDestSlot is single digit (or at most double-digit) , the totoal number of Jump Destinations (return/goto/break/continue/..) in the function.
I thought about widening 1st arg from CHAR to unsigned, but dropped the idea as it seems unnecessary. Yes, someone can write a code to make more than 255 Jumps in a function, but it’s unlikely to happen in real world code. What do you think?
I can go either way..
For sure, we can add an assert to catch it.

[efriedma]

I'm not concerned about the performance implications of whatever approach we take here. In the worst case, an "icmp+zext" corresponds to two extra arithmetic instructions; that's not enough to matter. And I expect usually it'll get optimized away.

I'd prefer to avoid exposing our cleanup numbering to user code, if possible. The Microsoft documentation says it returns 0 or 1.

[tentzen]

In D77936#1976984, @efriedma wrote:

I'm not concerned about the performance implications of whatever approach we take here. In the worst case, an "icmp+zext" corresponds to two extra arithmetic instructions; that's not enough to matter. And I expect usually it'll get optimized away.

I'd prefer to avoid exposing our cleanup numbering to user code, if possible. The Microsoft documentation says it returns 0 or 1.

OK, will do..
thanks

[tentzen]

Per Eli's suggestion,
Use icmp (and zext) to check the JumpDest index, instead of directly passing the Index to _finally().

[efriedma]

LGTM

[rnk]

I like the implementation strategy, but I have one corner case, which is probably worth a test.

clang/lib/CodeGen/CGCleanup.cpp

848

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

863–864

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?

clang/lib/CodeGen/EHScopeStack.h

184

Please add a comment describing the accessor.

[tentzen]

clang/lib/CodeGen/CGCleanup.cpp

848

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@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
]

863–864

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?

[tentzen]

Replace F_HasSehAbnormalExits with F_HasExitSwitch

[tentzen]

remade a patch after re-sync

clang/lib/CodeGen/CGCleanup.cpp

// the cleanup block and then try to clean up after ourselves.
           NormalCleanupDestSlot->user_back()->eraseFromParent();
           NormalCleanupDestSlot->eraseFromParent();
           NormalCleanupDest = Address::invalid();
         }
 
         llvm::BasicBlock *BranchAfter = Scope.getBranchAfterBlock(0);
         InstsToAppend.push_back(llvm::BranchInst::Create(BranchAfter));
 
       // Build a switch-out if we need it:
       //   - if there are branch-afters threaded through the scope
       //   - if fall-through is a branch-after
       //   - if there are fixups that have nowhere left to go and
       //     so must be immediately resolved
       } else if (Scope.getNumBranchAfters() ||
                  (HasFallthrough && !FallthroughIsBranchThrough) ||
                  (HasFixups && !HasEnclosingCleanups)) {
 
         llvm::BasicBlock *Default =
           (BranchThroughDest ? BranchThroughDest : getUnreachableBlock());
 
         // TODO: base this on the number of branch-afters and fixups
         const unsigned SwitchCapacity = 10;
 
+        // pass the abnormal exit flag to Fn (SEH cleanup)
+        cleanupFlags.setHasExitSwitch();
+
         llvm::LoadInst *Load =
           createLoadInstBefore(getNormalCleanupDestSlot(), "cleanup.dest",
                                nullptr);
         llvm::SwitchInst *Switch =
           llvm::SwitchInst::Create(Load, Default, SwitchCapacity);
 
         InstsToAppend.push_back(Load);
         InstsToAppend.push_back(Switch);

clang/lib/CodeGen/CGException.cpp

void Emit(CodeGenFunction &CGF, Flags F) override {
     } else {
       llvm::Function *LocalAddrFn =
           CGM.getIntrinsic(llvm::Intrinsic::localaddress);
       FP = CGF.Builder.CreateCall(LocalAddrFn);
     }
 
     llvm::Value *IsForEH =
         llvm::ConstantInt::get(CGF.ConvertType(ArgTys[0]), F.isForEHCleanup());
+
+    // Except _leave and fall-through at the end, all other exits in a _try
+    //   (return/goto/continue/break) are considered as abnormal terminations
+    //   since _leave/fall-through is always Indexed 0,
+    //   just use NormalCleanupDestSlot (>= 1 for goto/return/..),
+    //   as 1st Arg to indicate abnormal termination
+    if (!F.isForEHCleanup() && F.hasExitSwitch()) {
+      Address Addr = CGF.getNormalCleanupDestSlot();
+      llvm::Value *Load = CGF.Builder.CreateLoad(Addr, "cleanup.dest");
+      llvm::Value *Zero = llvm::Constant::getNullValue(CGM.Int32Ty);
+      IsForEH = CGF.Builder.CreateICmpNE(Load, Zero);
+    }
+
     Args.add(RValue::get(IsForEH), ArgTys[0]);
     Args.add(RValue::get(FP), ArgTys[1]);
 
     // Arrange a two-arg function info and type.
     const CGFunctionInfo &FnInfo =
         CGM.getTypes().arrangeBuiltinFunctionCall(Context.VoidTy, Args);
 
     auto Callee = CGCallee::forDirect(OutlinedFinally);

clang/lib/CodeGen/EHScopeStack.h

public:
   public:
     Cleanup(const Cleanup &) = default;
     Cleanup(Cleanup &&) {}
     Cleanup() = default;
 
     /// Generation flags.
     class Flags {
       enum {
-        F_IsForEH             = 0x1,
+        F_IsForEH = 0x1,
         F_IsNormalCleanupKind = 0x2,
-        F_IsEHCleanupKind     = 0x4
+        F_IsEHCleanupKind = 0x4,
+        F_HasExitSwitch = 0x8,
       };
       unsigned flags;
 
     public:
       Flags() : flags(0) {}
 
       /// isForEH - true if the current emission is for an EH cleanup.
       bool isForEHCleanup() const { return flags & F_IsForEH; }
       bool isForNormalCleanup() const { return !isForEHCleanup(); }
       void setIsForEHCleanup() { flags |= F_IsForEH; }
 
       bool isNormalCleanupKind() const { return flags & F_IsNormalCleanupKind; }
       void setIsNormalCleanupKind() { flags |= F_IsNormalCleanupKind; }
 
       /// isEHCleanupKind - true if the cleanup was pushed as an EH
       /// cleanup.
       bool isEHCleanupKind() const { return flags & F_IsEHCleanupKind; }
       void setIsEHCleanupKind() { flags |= F_IsEHCleanupKind; }
-    };
 
+      bool hasExitSwitch() const { return flags & F_HasExitSwitch; }
+      void setHasExitSwitch() { flags |= F_HasExitSwitch; }
+    };
 
     /// Emit the cleanup.  For normal cleanups, this is run in the
     /// same EH context as when the cleanup was pushed, i.e. the
     /// immediately-enclosing context of the cleanup scope.  For
     /// EH cleanups, this is run in a terminate context.
     ///
     // \param flags cleanup kind.
     virtual void Emit(CodeGenFunction &CGF, Flags flags) = 0;

clang/test/CodeGen/windows-seh-abnormal-exits.c

+// RUN: %clang_cc1 -triple x86_64-windows -fms-extensions -Wno-implicit-function-declaration -S -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %[[src:[0-9-]+]] = call i8* @llvm.localaddress()
+// CHECK-NEXT: %cleanup.dest = load i32, i32* %cleanup.dest.slot, align 4
+// CHECK-NEXT: %[[src2:[0-9-]+]] = icmp ne i32 %cleanup.dest, 0
+// CHECK-NEXT: %[[src3:[0-9-]+]] = zext i1 %[[src2]] to i8
+// CHECK-NEXT: call void @"?fin$0@0@seh_abnormal_exits@@"(i8 %[[src3]], i8* %[[src]])
+
+void seh_abnormal_exits(int *Counter) {
+  for (int i = 0; i < 5; i++) {
+    __try {
+      if (i == 0)
+        continue;   // abnormal termination
+      else if (i == 1)
+        goto t10;   // abnormal termination
+      else if (i == 2)
+        __leave;  // normal execution
+      else if (i == 4)
+        return;  // abnormal termination
+    }
+    __finally {
+      if (AbnormalTermination()) {
+        *Counter += 1;
+      }
+    }
+  t10:;
+  }
+  return; // *Counter == 3
+}
+