[WinEH] Emit state stores for SEH scopes

[momo5502]

At the moment Windows 32 bit SEH state stores are only emitted for throwing calls.

Windows 32 bit SEH state stores should also be emitted before SEH scope begin and before SEH scope end.
An invalid inline memory access would otherwise not trigger unwinding, in combination with /EHa.

This fixes #90946

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[momo5502]

The bug from the issue seems fixed at this point, but there are further issues:

#include <cstdio>

struct Destructor {
  Destructor(int* destructorCalled) : mDestructorCalled(destructorCalled) {}
  ~Destructor() { ++*mDestructorCalled; }
  int* mDestructorCalled;
};

void HandleDestructorCallWithException(int* destructorCalled, bool _throw_1)
{
  {
  Destructor x(destructorCalled);

  if(_throw_1)
    *reinterpret_cast<int*>(1) = 1;

  }
  
  *reinterpret_cast<int*>(2) = 2;
}

void CatchNativeExceptions(int* destructorCalled, bool* exceptionThrown, bool _throw_1)
{
  try {
    HandleDestructorCallWithException(destructorCalled, _throw_1);
  }
  catch(...) {
    *exceptionThrown = true;
  }
}


int main()
{
  int destructorCalled = 0;
  bool exceptionThrown = false;

  CatchNativeExceptions(&destructorCalled, &exceptionThrown, true);

  printf("Destructor called %i times\n",destructorCalled );
  puts(exceptionThrown ? "Exception thrown" : "Exception NOT thrown");

  
  destructorCalled = 0;
  exceptionThrown = false;

  CatchNativeExceptions(&destructorCalled, &exceptionThrown, false);

  printf("Destructor called %i times\n",destructorCalled );
  puts(exceptionThrown ? "Exception thrown" : "Exception NOT thrown");

  return 0;
}

Output using cl:

Destructor called 1 times
Exception thrown
Destructor called 1 times
Exception thrown

Output using clang-cl with the fix here:

Destructor called 1 times
Exception thrown
Destructor called 2 times
Exception thrown

--> state restore is still needed
should be fixed now

[momo5502]

See issue #90946 for analysis details

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: Maurice Heumann (momo5502)

Changes

At the moment Windows 32 bit SEH state stores are only emitted for throwing calls.

Windows 32 bit SEH state stores should also be emitted before SEH scope begin and after SEH scope end.
An invalid inline memory access would otherwise not trigger unwding, in combination with /EHa.

This fixes #90946

---

Full diff: https://github.com/llvm/llvm-project/pull/116546.diff

2 Files Affected:

• (modified) llvm/lib/Target/X86/X86WinEHState.cpp (+34-1)
• (added) llvm/test/CodeGen/WinEH/wineh-scope-statenumbering.ll (+46)

diff --git a/llvm/lib/Target/X86/X86WinEHState.cpp b/llvm/lib/Target/X86/X86WinEHState.cpp
index ef212736730114..d5bcbd2a08217d 100644
--- a/llvm/lib/Target/X86/X86WinEHState.cpp
+++ b/llvm/lib/Target/X86/X86WinEHState.cpp
@@ -604,8 +604,25 @@ static int getSuccState(DenseMap<BasicBlock *, int> &InitialStates, Function &F,
   return CommonState;
 }
 
+static bool isIntrinsic(const CallBase &Call, Intrinsic::ID ID) {
+  const Function *CF = Call.getCalledFunction();
+  return CF && CF->isIntrinsic() && CF->getIntrinsicID() == ID;
+}
+
+static bool isSehScopeEnd(const CallBase &Call) {
+  return isIntrinsic(Call, Intrinsic::seh_scope_end);
+}
+
+static bool isSehScopeBegin(const CallBase &Call) {
+  return isIntrinsic(Call, Intrinsic::seh_scope_begin);
+}
+
 bool WinEHStatePass::isStateStoreNeeded(EHPersonality Personality,
                                         CallBase &Call) {
+  if (isSehScopeBegin(Call)) {
+    return true;
+  }
+
   // If the function touches memory, it needs a state store.
   if (isAsynchronousEHPersonality(Personality))
     return !Call.doesNotAccessMemory();
@@ -642,6 +659,8 @@ void WinEHStatePass::addStateStores(Function &F, WinEHFuncInfo &FuncInfo) {
   DenseMap<BasicBlock *, int> InitialStates;
   // FinalStates yields the state of the last call-site for a BasicBlock.
   DenseMap<BasicBlock *, int> FinalStates;
+  // SEH scope end target blocks
+  SmallPtrSet<BasicBlock *, 4> ScopeEndBlocks;
   // Worklist used to revisit BasicBlocks with indeterminate
   // Initial/Final-States.
   std::deque<BasicBlock *> Worklist;
@@ -653,7 +672,15 @@ void WinEHStatePass::addStateStores(Function &F, WinEHFuncInfo &FuncInfo) {
       InitialState = FinalState = ParentBaseState;
     for (Instruction &I : *BB) {
       auto *Call = dyn_cast<CallBase>(&I);
-      if (!Call || !isStateStoreNeeded(Personality, *Call))
+      if (!Call)
+        continue;
+
+      if (isSehScopeEnd(*Call)) {
+        auto *Invoke = cast<InvokeInst>(Call);
+        ScopeEndBlocks.insert(Invoke->getNormalDest());
+      }
+
+      if (!isStateStoreNeeded(Personality, *Call))
         continue;
 
       int State = getStateForCall(BlockColors, FuncInfo, *Call);
@@ -706,6 +733,12 @@ void WinEHStatePass::addStateStores(Function &F, WinEHFuncInfo &FuncInfo) {
     FinalStates.insert({BB, SuccState});
   }
 
+  // Insert state restores after SEH scope ends
+  for (BasicBlock *BB : ScopeEndBlocks) {
+    int state = getBaseStateForBB(BlockColors, FuncInfo, BB);
+    insertStateNumberStore(BB->getFirstNonPHI(), state);
+  }
+
   // Finally, insert state stores before call-sites which transition us to a new
   // state.
   for (BasicBlock *BB : RPOT) {
diff --git a/llvm/test/CodeGen/WinEH/wineh-scope-statenumbering.ll b/llvm/test/CodeGen/WinEH/wineh-scope-statenumbering.ll
new file mode 100644
index 00000000000000..40c59970faae12
--- /dev/null
+++ b/llvm/test/CodeGen/WinEH/wineh-scope-statenumbering.ll
@@ -0,0 +1,46 @@
+; RUN: opt -mtriple=i386-pc-windows-msvc -S -x86-winehstate < %s | FileCheck %s
+
+target datalayout = "e-m:x-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32-a:0:32-S32"
+target triple = "i386-pc-windows-msvc19.42.34433"
+
+%struct.Destructor = type { ptr }
+
+define dso_local void @"?HandleDestructorCallWithException@@YAXPA_N@Z"(ptr noundef %destructorCalled) personality ptr @__CxxFrameHandler3 {
+entry:
+  %destructorCalled.addr = alloca ptr, align 4
+  %x = alloca %struct.Destructor, align 4
+  store ptr %destructorCalled, ptr %destructorCalled.addr, align 4
+  %0 = load ptr, ptr %destructorCalled.addr, align 4
+  %call = call x86_thiscallcc noundef ptr @"??0Destructor@@QAE@PA_N@Z"(ptr noundef nonnull align 4 dereferenceable(4) %x, ptr noundef %0)
+  ; CHECK:  store i32 0, ptr %9, align 4
+  ; CHECK-NEXT:  invoke void @llvm.seh.scope.begin()
+  invoke void @llvm.seh.scope.begin()
+          to label %invoke.cont unwind label %ehcleanup
+
+invoke.cont:
+  store i32 1, ptr inttoptr (i32 1 to ptr), align 4
+  invoke void @llvm.seh.scope.end()
+          to label %invoke.cont1 unwind label %ehcleanup
+
+invoke.cont1:
+  ; CHECK: invoke.cont1:
+  ; CHECK-NEXT:%10 = getelementptr inbounds nuw %CXXExceptionRegistration, ptr %0, i32 0, i32 2
+  ; CHECK-NEXT:  store i32 -1, ptr %10, align 4
+  call x86_thiscallcc void @"??1Destructor@@QAE@XZ"(ptr noundef nonnull align 4 dereferenceable(4) %x) #1
+  ret void
+
+ehcleanup:
+  %1 = cleanuppad within none []
+  call x86_thiscallcc void @"??1Destructor@@QAE@XZ"(ptr noundef nonnull align 4 dereferenceable(4) %x) #1 [ "funclet"(token %1) ]
+  cleanupret from %1 unwind to caller
+}
+
+declare dso_local i32 @__CxxFrameHandler3(...)
+declare dso_local void @llvm.seh.scope.begin() #0
+declare dso_local void @llvm.seh.scope.end() #0
+
+declare dso_local x86_thiscallcc noundef ptr @"??0Destructor@@QAE@PA_N@Z"(ptr noundef nonnull returned align 4 dereferenceable(4) %this, ptr noundef %destructorCalled)
+declare dso_local x86_thiscallcc void @"??1Destructor@@QAE@XZ"(ptr noundef nonnull align 4 dereferenceable(4) %this) #1
+
+attributes #0 = { nounwind memory(none) }
+attributes #1 = { nounwind }

[rnk]

SEH scope ends are effectively non-throwing no-op pseudo instructions, so there should be no functional difference between inserting the store before or after the no-op. For simplicity, can we insert the state stores before the scope ends? That would make this a bit more uniform.

I haven't played around with the /EHa SEH scope codegen patterns since they were added, but it would be pretty reasonable for codegen to lower them to a direct branch, which after branch folding would get optimized out anyway.

[momo5502]

I added the state store before the scope end. To do that, I added a condition to getStateForCall as, unlike other invoke instructions that map to the state of the unwind destination, this one should map to the state of the normal destination (as the unwind destination is the same as for SEH scope begin).

I also thought about adapting calculateStateNumbersForInvokes within WinEHPrepare.cpp, which fills the InvokeStateMap. However, I wasn't sure what is better, so I put the SEH scope end check into getStateForCall, as that would be the smallest change.

[rnk]

Thanks for the patch!