[clang] [SEH] Fix assertin when return scalar value from __try block. (PR #71488)

[via cfe-commits]

https://github.com/jyu2-git updated https://github.com/llvm/llvm-project/pull/71488

>From 3313aca0622da3882a9e5bf304b89f28fecce7fe Mon Sep 17 00:00:00 2001
From: Jennifer Yu <jennifer.yu at intel.com>
Date: Mon, 6 Nov 2023 20:51:39 -0800
Subject: [PATCH 1/2] [SEH] Fix assertin when return scalar value from __try
 block.

Current compler assert with `!SI->isAtomic() && !SI->isVolatile()' failed

This due to following rule:
First, no exception can move in or out of _try region., i.e., no "potential
faulty instruction can be moved across _try boundary.
Second, the order of exceptions for instructions 'directly' under a _try
must be preserved (not applied to those in callees).
Finally, global states (local/global/heap variables) that can be read
outside of _try region must be updated in memory (not just in register)
before the subsequent exception occurs.

All memory instructions inside a _try are considered as 'volatile' to assure
2nd and 3rd rules for C-code above. This is a little sub-optimized. But it's
acceptable as the amount of code directly under _try is very small.
However during findDominatingStoreToReturnValue: those are not allowed.

To fix just skip the assertion when current function has seh try.
---
 clang/lib/CodeGen/CGCall.cpp                  |  3 +++
 .../CodeGen/windows-seh-EHa-TryInFinally.cpp  | 23 +++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index e7951b3a3f4d855..bc367b89992bbd9 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -3507,6 +3507,9 @@ static llvm::StoreInst *findDominatingStoreToReturnValue(CodeGenFunction &CGF) {
       return nullptr;
     // These aren't actually possible for non-coerced returns, and we
     // only care about non-coerced returns on this code path.
+    // All memory instructions inside __try block are volatile.
+    if (CGF.currentFunctionUsesSEHTry() && SI->isVolatile())
+      return SI;
     assert(!SI->isAtomic() && !SI->isVolatile());
     return SI;
   };
diff --git a/clang/test/CodeGen/windows-seh-EHa-TryInFinally.cpp b/clang/test/CodeGen/windows-seh-EHa-TryInFinally.cpp
index fab567e763df443..ce2a9528e1908a9 100644
--- a/clang/test/CodeGen/windows-seh-EHa-TryInFinally.cpp
+++ b/clang/test/CodeGen/windows-seh-EHa-TryInFinally.cpp
@@ -67,3 +67,26 @@ void foo()
     }
   }
 }
+
+// CHECK-LABEL:@"?bar@@YAHXZ"()
+// CHECK: invoke.cont:
+// CHECK: invoke void @llvm.seh.try.begin()
+// CHECK: invoke.cont1:
+// CHECK: store volatile i32 1, ptr %cleanup.dest.slot
+// CHECK: invoke void @llvm.seh.try.end()
+// CHECK: invoke.cont2:
+// CHECK: call void @"?fin$0 at 0@bar@@"
+// CHECK: %cleanup.dest3 = load i32, ptr %cleanup.dest.slot
+// CHECK: return:
+// CHECK: ret i32 11
+int bar()
+{
+  int x;
+  __try {
+    return 11;
+  } __finally {
+    if (_abnormal_termination()) {
+      x = 9;
+    }
+  }
+}

>From 4caaaef0ada4d22b91c38704da90a2c1debe8e55 Mon Sep 17 00:00:00 2001
From: Jennifer Yu <jennifer.yu at intel.com>
Date: Tue, 7 Nov 2023 15:24:47 -0800
Subject: [PATCH 2/2] Thanks @rnk for the review.  This address his comment.

---
 clang/lib/CodeGen/CGCall.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index bc367b89992bbd9..8fd876825b7ea08 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -3508,9 +3508,8 @@ static llvm::StoreInst *findDominatingStoreToReturnValue(CodeGenFunction &CGF) {
     // These aren't actually possible for non-coerced returns, and we
     // only care about non-coerced returns on this code path.
     // All memory instructions inside __try block are volatile.
-    if (CGF.currentFunctionUsesSEHTry() && SI->isVolatile())
-      return SI;
-    assert(!SI->isAtomic() && !SI->isVolatile());
+    assert(!SI->isAtomic() &&
+           (!SI->isVolatile() || CGF.currentFunctionUsesSEHTry()));
     return SI;
   };
   // If there are multiple uses of the return-value slot, just check