Page MenuHomePhabricator

[Windows SEH]: Fix -O2 crash for Windows -EHa
ClosedPublic

Authored by tentzen on Thu, Jun 3, 7:43 PM.

Details

Summary

This patch fixes a Windows -EHa crash induced by previous commit 797ad701522988e212495285dade8efac41a24d4.
The crash was caused by "LifetimeMarker" scope (with option -O2) that should not be considered as SEH Scope.

This change also turns off -fasync-exceptions by default under -EHa option for now.

Diff Detail

Event Timeline

tentzen requested review of this revision.Thu, Jun 3, 7:43 PM
tentzen created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jun 3, 7:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tentzen edited the summary of this revision. (Show Details)Thu, Jun 3, 7:47 PM
tentzen edited the summary of this revision. (Show Details)
aganea added a comment.EditedFri, Jun 4, 5:05 AM

Thanks for the quick fix! Would you mind fixing the two failing tests please? (see above)

zahiraam added a comment.EditedFri, Jun 4, 8:07 AM
This comment has been deleted.
clang/lib/Driver/ToolChains/Clang.cpp
7155

Not really sure I understand this change. Isn't the case that if I compile with -EHa, I want the -fasync-exceptions option to be added to my options?

Thanks for the quick fix! Would you mind fixing the two failing tests please? (see above)

Hmm, I cannot repro locally..

@zahiraam, currently -EHa is not completely ready yet. we need a couple of more patches. So turn it off to preserve zero-diff, unless that -fasync-exceptions is explicitly specified.

tentzen updated this revision to Diff 349939.Fri, Jun 4, 12:31 PM

Thanks for the quick fix! Would you mind fixing the two failing tests please? (see above)

Hmm, I cannot repro locally..

@zahiraam, currently -EHa is not completely ready yet. we need a couple of more patches. So turn it off to preserve zero-diff, unless that -fasync-exceptions is explicitly specified.

Got it. Thanks.
I was able to repro the fails. And this is the fix for it:
diff --git a/clang/test/CodeGen/windows-seh-EHa-CppDtors01.cpp b/clang/test/CodeGen/windows-seh-EHa-CppDtors01.cpp
index 47117b4a9613..b9195a2920e6 100644

  • a/clang/test/CodeGen/windows-seh-EHa-CppDtors01.cpp

+++ b/clang/test/CodeGen/windows-seh-EHa-CppDtors01.cpp
@@ -1,60 +1,54 @@
// RUN: %clang_cc1 -triple x86_64-windows -fasync-exceptions -fcxx-exceptions -fexceptions -fms-extensions -x c++ -Wno-implicit-function-declaration -S -emit-llvm %s -o - | FileCheck %s

- CHECK: invoke void @llvm.seh.scope.begin()
-
CHECK: invoke void @llvm.seh.scope.begin()
- CHECK: invoke void @llvm.seh.scope.begin()
-
CHECK: invoke void @llvm.seh.scope.end()
- CHECK: invoke void @llvm.seh.scope.end()
-
CHECK: invoke void @llvm.seh.scope.end()
+// RUN: %clang_cc1 -triple x86_64-windows -fasync-exceptions -fcxx-exceptions -fexceptions -fms-extensions -x c++ -Wno-implicit-function-declaration -O2 -S -emit-llvm %s -o - | FileCheck %s

CHECK: invoke void @llvm.seh.try.begin()
-
CHECK: %[[src:[0-9-]+]] = load volatile i32, i32* %i
- CHECK-NEXT: invoke void @"?crash@@YAXH@Z"(i32 %[[src]])
+
CHECK: = load volatile i32, i32* %i
+ CHECK-NEXT: invoke void @"?crash@@YAXH@Z"(i32
CHECK: invoke void @llvm.seh.try.end()

I was able to repro the fail and this is the fix for it.

@zahiraam, are you removing all those CHECKs:

  • CHECK: invoke void @llvm.seh.scope.**

those are placed there to ensure SEH scope semantic is preserved for Od..

@zahiraam, are you removing all those CHECKs:

  • CHECK: invoke void @llvm.seh.scope.**

those are placed there to ensure SEH scope semantic is preserved for Od..

I was just showing you. The checks generated at O2 are only the ones that I have left. At O0 the checks need to stay. So you need to rewrite the LIT tests so that both are expected for one option and the other.

since I cannot repro it locally, let's have this patch in to resolve -EHa -O2 crashes for now.
I will add more -O2 tests in following patches.

since I cannot repro it locally, let's have this patch in to resolve -EHa -O2 crashes for now.
I will add more -O2 tests in following patches.

Sounds good to me!

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Jun 4, 2:10 PM
This revision was automatically updated to reflect the committed changes.