[WinEH] Fix crash, object unwinding in the except block #172287
Conversation
|
Could someone please take a look at this patch when you have some free time? I understand it only needs to fix the crash issue. |
|
CC @MuellerMP |
|
My general take on this issue: since msvc rejects this it would not be unreasonable to reject this in clang aswell for compatibility reasons and to avoid overall complexity in the state handling. Regarding this patch: Now what actually causes the issue: =>But like I said: I would rather choose to reject this in the frontend. |
I checked the compiled code, and it didn't reject anything. You can run this case and then check it. |
|
@GkvJwa My point is that the |
Okay, so if we find a class object in llvm.eh.exceptioncode, is it feasible to directly refuse compilation? |
|
@GkvJwa Clang should probably catch this in the frontend and generate a similar error message as the |
It's okay, we can handle this simply first, and then improve SEH support later (I also encountered this problem during compilation. After reproducing it and creating a demo, I tried it with MSVC and found that it wasn't supported.) |
|
I would also like to note that using |
GkvJwa
force-pushed
the
fix_seh_crash
branch
from
10c244b to
1dc5b73
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (GkvJwa) ChangesConsider the following code: The following error will occur in mscv. However, using LLVM /EHa will cause it to crash. This patch is compatible with the crash. Full diff: https://github.com/llvm/llvm-project/pull/172287.diff 1 Files Affected:
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 1b1643250d05e..c64029f269c2b 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -4553,6 +4553,45 @@ StmtResult Sema::ActOnSEHTryBlock(bool IsCXXTry, SourceLocation TryLoc,
return SEHTryStmt::Create(Context, IsCXXTry, TryLoc, TryBlock, Handler);
}
+static bool containsNonTrivialObject(Sema &S, const Stmt *Node) {
+ (void)S;
+ if (!Node)
+ return false;
+
+ if (const DeclStmt *DS = dyn_cast<DeclStmt>(Node)) {
+ for (const Decl *D : DS->decls()) {
+ if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
+ QualType T = VD->getType();
+ if (const RecordType *RT = T->getAs<RecordType>()) {
+ if (const CXXRecordDecl *RD = RT->getAsCXXRecordDecl()) {
+ if (RD->hasDefinition() && !RD->hasTrivialDestructor())
+ return true;
+ }
+ }
+ }
+ }
+ }
+
+ if (const Expr *E = dyn_cast<Expr>(Node)) {
+ QualType T = E->getType();
+ if (T->isRecordType() && E->getValueKind() != VK_LValue) {
+ if (const RecordType *RT = T->getAs<RecordType>()) {
+ if (const CXXRecordDecl *RD = RT->getAsCXXRecordDecl()) {
+ if (RD->hasDefinition() && !RD->hasTrivialDestructor())
+ return true;
+ }
+ }
+ }
+ }
+
+ // children.
+ for (const Stmt *Child : Node->children())
+ if (containsNonTrivialObject(S, Child))
+ return true;
+
+ return false;
+}
+
StmtResult Sema::ActOnSEHExceptBlock(SourceLocation Loc, Expr *FilterExpr,
Stmt *Block) {
assert(FilterExpr && Block);
@@ -4562,6 +4601,10 @@ StmtResult Sema::ActOnSEHExceptBlock(SourceLocation Loc, Expr *FilterExpr,
Diag(FilterExpr->getExprLoc(), diag::err_filter_expression_integral)
<< FTy);
}
+ if (containsNonTrivialObject(*this, Block)) {
+ Diag(Loc, diag::err_seh_try_outside_functions);
+ return StmtError();
+ }
return SEHExceptStmt::Create(Context, Loc, FilterExpr, Block);
}
|
On Windows, the latter is the SEH. The Also, if you have time, review it again. If there are no issues, I will add test. Of course, there shouldn't be any class objects in the |
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) ClangClang.SemaCXX/__try.cppIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
🪟 Windows x64 Test Results
Failed Tests(click on a test name to see its output) ClangClang.SemaCXX/__try.cppIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
|
@GkvJwa Regarding the failing test: I have to apologize. It seems there is some Borland C++ compatibility switch that I did not know about that allows these constructs to be parsed at least. I would suggest emitting the diagnostic only for non Borland C++. @efriedma-quic Can you confirm that we should move ahead with this diagnostic for non Borland C++? For reference we have a similar borland check here: |
4 participants
Consider the following code:
The following error will occur in mscv.
However, using LLVM /EHa will cause it to crash.
This patch is compatible with the crash(Unexpected state == -1)