[PATCH] D97687: [SEH] Fix capture of this in lambda functions

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

Looks like changing CurCodeDecl was not a good idea.
Fixed in https://reviews.llvm.org/D102027


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97687/new/

https://reviews.llvm.org/D97687

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102027: [SEH] Fix regression with SEH in noexpect functions

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

As reported in https://reviews.llvm.org/D97687#2741449


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102027/new/

https://reviews.llvm.org/D102027

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102027: [SEH] Fix regression with SEH in noexpect functions

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart created this revision.
ogoffart requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Commit 5baea0560160a693b19022c5d0ba637b6b46b2d8 
 set the 
CurCodeDecl
because it was needed to pass the assert in 
CodeGenFunction::EmitLValueForLambdaField,
But this was not right to do as CodeGenFunction::FinishFunction passes it to 
EmitEndEHSpec
and cause corruption of the EHStack.

  

Revert the part of the commit that changes the CurCodeDecl, and just change the 
assert
to allow more conditions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102027

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGenCXX/exceptions-seh.cpp


Index: clang/test/CodeGenCXX/exceptions-seh.cpp
===
--- clang/test/CodeGenCXX/exceptions-seh.cpp
+++ clang/test/CodeGenCXX/exceptions-seh.cpp
@@ -164,3 +164,5 @@
 // CHECK: store i32 1234, i32* @my_unique_global
 
 // CHECK: attributes #[[NOINLINE]] = { {{.*noinline.*}} }
+
+void seh_in_noexcept() noexcept { __try {} __finally {} }
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -4185,8 +4185,8 @@
 /// Given that we are currently emitting a lambda, emit an l-value for
 /// one of its members.
 LValue CodeGenFunction::EmitLValueForLambdaField(const FieldDecl *Field) {
-  assert(cast(CurCodeDecl)->getParent()->isLambda());
-  assert(cast(CurCodeDecl)->getParent() == Field->getParent());
+  assert(IsOutlinedSEHHelper || 
cast(CurCodeDecl)->getParent()->isLambda());
+  assert(IsOutlinedSEHHelper || cast(CurCodeDecl)->getParent() 
== Field->getParent());
   QualType LambdaTagType =
 getContext().getTagDeclType(Field->getParent());
   LValue LambdaLV = MakeNaturalAlignAddrLValue(CXXABIThisValue, LambdaTagType);
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -1966,7 +1966,6 @@
   StartFunction(GlobalDecl(), RetTy, Fn, FnInfo, Args,
 OutlinedStmt->getBeginLoc(), OutlinedStmt->getBeginLoc());
   CurSEHParent = ParentCGF.CurSEHParent;
-  CurCodeDecl = ParentCGF.CurCodeDecl;
 
   CGM.SetInternalFunctionAttributes(GlobalDecl(), CurFn, FnInfo);
   EmitCapturedLocals(ParentCGF, OutlinedStmt, IsFilter);


Index: clang/test/CodeGenCXX/exceptions-seh.cpp
===
--- clang/test/CodeGenCXX/exceptions-seh.cpp
+++ clang/test/CodeGenCXX/exceptions-seh.cpp
@@ -164,3 +164,5 @@
 // CHECK: store i32 1234, i32* @my_unique_global
 
 // CHECK: attributes #[[NOINLINE]] = { {{.*noinline.*}} }
+
+void seh_in_noexcept() noexcept { __try {} __finally {} }
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -4185,8 +4185,8 @@
 /// Given that we are currently emitting a lambda, emit an l-value for
 /// one of its members.
 LValue CodeGenFunction::EmitLValueForLambdaField(const FieldDecl *Field) {
-  assert(cast(CurCodeDecl)->getParent()->isLambda());
-  assert(cast(CurCodeDecl)->getParent() == Field->getParent());
+  assert(IsOutlinedSEHHelper || cast(CurCodeDecl)->getParent()->isLambda());
+  assert(IsOutlinedSEHHelper || cast(CurCodeDecl)->getParent() == Field->getParent());
   QualType LambdaTagType =
 getContext().getTagDeclType(Field->getParent());
   LValue LambdaLV = MakeNaturalAlignAddrLValue(CXXABIThisValue, LambdaTagType);
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -1966,7 +1966,6 @@
   StartFunction(GlobalDecl(), RetTy, Fn, FnInfo, Args,
 OutlinedStmt->getBeginLoc(), OutlinedStmt->getBeginLoc());
   CurSEHParent = ParentCGF.CurSEHParent;
-  CurCodeDecl = ParentCGF.CurCodeDecl;
 
   CGM.SetInternalFunctionAttributes(GlobalDecl(), CurFn, FnInfo);
   EmitCapturedLocals(ParentCGF, OutlinedStmt, IsFilter);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97687: [SEH] Fix capture of this in lambda functions

[Olivier Goffart via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5baea0560160: [SEH] Fix capture of this in lambda functions 
(authored by ogoffart).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97687/new/

https://reviews.llvm.org/D97687

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp


Index: clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
===
--- clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
+++ clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
@@ -123,3 +123,25 @@
 // CHECK: %[[l1_ref:[^ ]*]] = load i32*, i32** %[[l1_ref_ptr]]
 // CHECK: %[[l1:[^ ]*]] = load i32, i32* %[[l1_ref]]
 // CHECK: call i32 (i32, ...) @basic_filter(i32 %[[l1]], i32 %[[l2]])
+
+struct U {
+  void this_in_lambda();
+};
+
+void U::this_in_lambda() {
+  auto lambda = [=]() {
+__try {
+  might_crash();
+} __except (basic_filter(0, this)) {
+}
+  };
+  lambda();
+}
+
+// CHECK-LABEL: define internal i32 
@"?filt$0@0@?R@?0??this_in_lambda@U@@QEAAXXZ@"(i8* 
%exception_pointers, i8* %frame_pointer)
+// CHECK: %[[this_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void 
(%class.anon.0*)* @"??R@?0??this_in_lambda@U@@QEAAXXZ@QEBA@XZ" to 
i8*), i8* %[[fp:[^ ]*]], i32 0)
+// CHECK: %[[this_ptr:[^ ]*]] = bitcast i8* %[[this_i8]] to %class.anon.0**
+// CHECK: %[[this:[^ ]*]] = load %class.anon.0*, %class.anon.0** 
%[[this_ptr]], align 8
+// CHECK: %[[actual_this_ptr:[^ ]*]] = getelementptr inbounds %class.anon.0, 
%class.anon.0* %[[this]], i32 0, i32 0
+// CHECK: %[[actual_this:[^ ]*]] = load %struct.U*, %struct.U** 
%[[actual_this_ptr]], align 8
+// CHECK: call i32 (i32, ...) @basic_filter(i32 0, %struct.U* %[[actual_this]])
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -1879,8 +1879,24 @@
 setAddrOfLocalVar(VD, Recovered);
 
 if (isa(VD)) {
-  CXXThisValue = Builder.CreateLoad(Recovered, "this");
-  CXXABIThisValue = CXXThisValue;
+  CXXABIThisAlignment = ParentCGF.CXXABIThisAlignment;
+  CXXThisAlignment = ParentCGF.CXXThisAlignment;
+  CXXABIThisValue = Builder.CreateLoad(Recovered, "this");
+  if (ParentCGF.LambdaThisCaptureField) {
+LambdaThisCaptureField = ParentCGF.LambdaThisCaptureField;
+// We are in a lambda function where "this" is captured so the
+// CXXThisValue need to be loaded from the lambda capture
+LValue ThisFieldLValue =
+EmitLValueForLambdaField(LambdaThisCaptureField);
+if (!LambdaThisCaptureField->getType()->isPointerType()) {
+  CXXThisValue = ThisFieldLValue.getAddress(*this).getPointer();
+} else {
+  CXXThisValue = EmitLoadOfLValue(ThisFieldLValue, SourceLocation())
+ .getScalarVal();
+}
+  } else {
+CXXThisValue = CXXABIThisValue;
+  }
 }
   }
 
@@ -1949,6 +1965,7 @@
   StartFunction(GlobalDecl(), RetTy, Fn, FnInfo, Args,
 OutlinedStmt->getBeginLoc(), OutlinedStmt->getBeginLoc());
   CurSEHParent = ParentCGF.CurSEHParent;
+  CurCodeDecl = ParentCGF.CurCodeDecl;
 
   CGM.SetInternalFunctionAttributes(GlobalDecl(), CurFn, FnInfo);
   EmitCapturedLocals(ParentCGF, OutlinedStmt, IsFilter);


Index: clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
===
--- clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
+++ clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
@@ -123,3 +123,25 @@
 // CHECK: %[[l1_ref:[^ ]*]] = load i32*, i32** %[[l1_ref_ptr]]
 // CHECK: %[[l1:[^ ]*]] = load i32, i32* %[[l1_ref]]
 // CHECK: call i32 (i32, ...) @basic_filter(i32 %[[l1]], i32 %[[l2]])
+
+struct U {
+  void this_in_lambda();
+};
+
+void U::this_in_lambda() {
+  auto lambda = [=]() {
+__try {
+  might_crash();
+} __except (basic_filter(0, this)) {
+}
+  };
+  lambda();
+}
+
+// CHECK-LABEL: define internal i32 @"?filt$0@0@?R@?0??this_in_lambda@U@@QEAAXXZ@"(i8* %exception_pointers, i8* %frame_pointer)
+// CHECK: %[[this_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%class.anon.0*)* @"??R@?0??this_in_lambda@U@@QEAAXXZ@QEBA@XZ" to i8*), i8* %[[fp:[^ ]*]], i32 0)
+// CHECK: %[[this_ptr:[^ ]*]] = bitcast i8* %[[this_i8]] to %class.anon.0**
+// CHECK: %[[this:[^ ]*]] = load %class.anon.0*, %class.anon.0** %[[this_ptr]], align 8
+// CHECK: %[[actual_this_ptr:[^ ]*]] = getelementptr inbounds %class.anon.0, %class.anon.0* %[[this]], i32 0, i32 0
+// CHECK: %[[actual_this:[^ ]*]] = load %struct.U*, %struct.U** %[[actual_this_ptr]], align 8
+// CHECK: call i32 (i32, ...) @basic_filter(i32 0, %struct.U* %[[actual_this]])
Index: clang/lib/CodeGen/CGException.cpp

[PATCH] D97687: [SEH] Fix capture of this in lambda functions

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

Ping?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97687/new/

https://reviews.llvm.org/D97687

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97687: [SEH] Fix capture of this in lambda functions

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart created this revision.
ogoffart added a reviewer: rnk.
ogoffart requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Commit 1b04bdc2f3ffaa7a0e1e3dbdc3a0cd08f0b9a4ce 
 added 
support for
capturing the 'this' pointer in a SEH context (__finally or __except),
But the case in which the 'this' pointer is part of a lamdda capture
was not handled properly


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97687

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp


Index: clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
===
--- clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
+++ clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
@@ -123,3 +123,25 @@
 // CHECK: %[[l1_ref:[^ ]*]] = load i32*, i32** %[[l1_ref_ptr]]
 // CHECK: %[[l1:[^ ]*]] = load i32, i32* %[[l1_ref]]
 // CHECK: call i32 (i32, ...) @basic_filter(i32 %[[l1]], i32 %[[l2]])
+
+struct U {
+  void this_in_lambda();
+};
+
+void U::this_in_lambda() {
+  auto lambda = [=]() {
+__try {
+  might_crash();
+} __except (basic_filter(0, this)) {
+}
+  };
+  lambda();
+}
+
+// CHECK-LABEL: define internal i32 
@"?filt$0@0@?R@?0??this_in_lambda@U@@QEAAXXZ@"(i8* 
%exception_pointers, i8* %frame_pointer)
+// CHECK: %[[this_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void 
(%class.anon.0*)* @"??R@?0??this_in_lambda@U@@QEAAXXZ@QEBA@XZ" to 
i8*), i8* %[[fp:[^ ]*]], i32 0)
+// CHECK: %[[this_ptr:[^ ]*]] = bitcast i8* %[[this_i8]] to %class.anon.0**
+// CHECK: %[[this:[^ ]*]] = load %class.anon.0*, %class.anon.0** 
%[[this_ptr]], align 8
+// CHECK: %[[actual_this_ptr:[^ ]*]] = getelementptr inbounds %class.anon.0, 
%class.anon.0* %[[this]], i32 0, i32 0
+// CHECK: %[[actual_this:[^ ]*]] = load %struct.U*, %struct.U** 
%[[actual_this_ptr]], align 8
+// CHECK: call i32 (i32, ...) @basic_filter(i32 0, %struct.U* %[[actual_this]])
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -1888,8 +1888,24 @@
 setAddrOfLocalVar(VD, Recovered);
 
 if (isa(VD)) {
-  CXXThisValue = Builder.CreateLoad(Recovered, "this");
-  CXXABIThisValue = CXXThisValue;
+  CXXABIThisAlignment = ParentCGF.CXXABIThisAlignment;
+  CXXThisAlignment = ParentCGF.CXXThisAlignment;
+  CXXABIThisValue = Builder.CreateLoad(Recovered, "this");
+  if (ParentCGF.LambdaThisCaptureField) {
+LambdaThisCaptureField = ParentCGF.LambdaThisCaptureField;
+// We are in a lambda function where "this" is captured so the
+// CXXThisValue need to be loaded from the lambda capture
+LValue ThisFieldLValue =
+EmitLValueForLambdaField(LambdaThisCaptureField);
+if (!LambdaThisCaptureField->getType()->isPointerType()) {
+  CXXThisValue = ThisFieldLValue.getAddress(*this).getPointer();
+} else {
+  CXXThisValue = EmitLoadOfLValue(ThisFieldLValue, SourceLocation())
+ .getScalarVal();
+}
+  } else {
+CXXThisValue = CXXABIThisValue;
+  }
 }
   }
 
@@ -1958,6 +1974,7 @@
   StartFunction(GlobalDecl(), RetTy, Fn, FnInfo, Args,
 OutlinedStmt->getBeginLoc(), OutlinedStmt->getBeginLoc());
   CurSEHParent = ParentCGF.CurSEHParent;
+  CurCodeDecl = ParentCGF.CurCodeDecl;
 
   CGM.SetInternalFunctionAttributes(GlobalDecl(), CurFn, FnInfo);
   EmitCapturedLocals(ParentCGF, OutlinedStmt, IsFilter);


Index: clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
===
--- clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
+++ clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
@@ -123,3 +123,25 @@
 // CHECK: %[[l1_ref:[^ ]*]] = load i32*, i32** %[[l1_ref_ptr]]
 // CHECK: %[[l1:[^ ]*]] = load i32, i32* %[[l1_ref]]
 // CHECK: call i32 (i32, ...) @basic_filter(i32 %[[l1]], i32 %[[l2]])
+
+struct U {
+  void this_in_lambda();
+};
+
+void U::this_in_lambda() {
+  auto lambda = [=]() {
+__try {
+  might_crash();
+} __except (basic_filter(0, this)) {
+}
+  };
+  lambda();
+}
+
+// CHECK-LABEL: define internal i32 @"?filt$0@0@?R@?0??this_in_lambda@U@@QEAAXXZ@"(i8* %exception_pointers, i8* %frame_pointer)
+// CHECK: %[[this_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%class.anon.0*)* @"??R@?0??this_in_lambda@U@@QEAAXXZ@QEBA@XZ" to i8*), i8* %[[fp:[^ ]*]], i32 0)
+// CHECK: %[[this_ptr:[^ ]*]] = bitcast i8* %[[this_i8]] to %class.anon.0**
+// CHECK: %[[this:[^ ]*]] = load %class.anon.0*, %class.anon.0** %[[this_ptr]], align 8
+// CHECK: %[[actual_this_ptr:[^ ]*]] = getelementptr inbounds %class.anon.0, %class.anon.0* %[[this]], i32 0, i32 

[PATCH] D97534: SEH: capture 'this'

[Olivier Goffart via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1b04bdc2f3ff: [SEH] capture this (authored by 
ogoffart).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97534/new/

https://reviews.llvm.org/D97534

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp

Index: clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
===
--- clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
+++ clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
@@ -39,13 +39,13 @@
   int l1 = 13;
   __try {
 might_crash();
-  } __except(basic_filter(l1)) {
-// FIXME: Test capturing 'this' and 'm1'.
+  } __except (basic_filter(l1, m1)) {
   }
 }
 
 // CHECK-LABEL: define dso_local void @"?test_method@S@@QEAAXXZ"(%struct.S* {{[^,]*}} %this)
-// CHECK: @llvm.localescape(i32* %[[l1_addr:[^, ]*]])
+// CHECK: @llvm.localescape(i32* %[[l1_addr:[^, ]*]], %struct.S** %[[this_addr:[^, ]*]])
+// CHECK: store %struct.S* %this, %struct.S** %[[this_addr]], align 8
 // CHECK: store i32 13, i32* %[[l1_addr]], align 4
 // CHECK: invoke void @might_crash()
 
@@ -53,8 +53,45 @@
 // CHECK: %[[fp:[^ ]*]] = call i8* @llvm.eh.recoverfp(i8* bitcast (void (%struct.S*)* @"?test_method@S@@QEAAXXZ" to i8*), i8* %frame_pointer)
 // CHECK: %[[l1_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.S*)* @"?test_method@S@@QEAAXXZ" to i8*), i8* %[[fp]], i32 0)
 // CHECK: %[[l1_ptr:[^ ]*]] = bitcast i8* %[[l1_i8]] to i32*
+// CHECK: %[[this_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.S*)* @"?test_method@S@@QEAAXXZ" to i8*), i8* %[[fp]], i32 1)
+// CHECK: %[[this_ptr:[^ ]*]] = bitcast i8* %[[this_i8]] to %struct.S**
+// CHECK: %[[this:[^ ]*]] = load %struct.S*, %struct.S** %[[this_ptr]], align 8
+// CHECK: %[[m1_ptr:[^ ]*]] = getelementptr inbounds %struct.S, %struct.S* %[[this]], i32 0, i32 0
+// CHECK: %[[m1:[^ ]*]] = load i32, i32* %[[m1_ptr]]
 // CHECK: %[[l1:[^ ]*]] = load i32, i32* %[[l1_ptr]]
-// CHECK: call i32 (i32, ...) @basic_filter(i32 %[[l1]])
+// CHECK: call i32 (i32, ...) @basic_filter(i32 %[[l1]], i32 %[[m1]])
+
+struct V {
+  void test_virtual(int p1);
+  virtual void virt(int p1);
+};
+
+void V::test_virtual(int p1) {
+  __try {
+might_crash();
+  } __finally {
+virt(p1);
+  }
+}
+
+// CHECK-LABEL: define dso_local void @"?test_virtual@V@@QEAAXH@Z"(%struct.V* {{[^,]*}} %this, i32 %p1)
+// CHECK: @llvm.localescape(%struct.V** %[[this_addr:[^, ]*]], i32* %[[p1_addr:[^, ]*]])
+// CHECK: store i32 %p1, i32* %[[p1_addr]], align 4
+// CHECK: store %struct.V* %this, %struct.V** %[[this_addr]], align 8
+// CHECK: invoke void @might_crash()
+
+// CHECK-LABEL: define internal void @"?fin$0@0@test_virtual@V@@"(i8 %abnormal_termination, i8* %frame_pointer)
+// CHECK: %[[this_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.V*, i32)* @"?test_virtual@V@@QEAAXH@Z" to i8*), i8* %frame_pointer, i32 0)
+// CHECK: %[[this_ptr:[^ ]*]] = bitcast i8* %[[this_i8]] to %struct.V**
+// CHECK: %[[this:[^ ]*]] = load %struct.V*, %struct.V** %[[this_ptr]], align 8
+// CHECK: %[[p1_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.V*, i32)* @"?test_virtual@V@@QEAAXH@Z" to i8*), i8* %frame_pointer, i32 1)
+// CHECK: %[[p1_ptr:[^ ]*]] = bitcast i8* %[[p1_i8]] to i32*
+// CHECK: %[[p1:[^ ]*]] = load i32, i32* %[[p1_ptr]]
+// CHECK: %[[this_2:[^ ]*]] = bitcast %struct.V* %[[this]] to void (%struct.V*, i32)***
+// CHECK: %[[vtable:[^ ]*]] = load void (%struct.V*, i32)**, void (%struct.V*, i32)*** %[[this_2]], align 8
+// CHECK: %[[vfn:[^ ]*]] = getelementptr inbounds void (%struct.V*, i32)*, void (%struct.V*, i32)** %[[vtable]], i64 0
+// CHECK: %[[virt:[^ ]*]] = load void (%struct.V*, i32)*, void (%struct.V*, i32)** %[[vfn]], align 8
+// CHECK: call void %[[virt]](%struct.V* {{[^,]*}} %[[this]], i32 %[[p1]])
 
 void test_lambda() {
   int l1 = 13;
@@ -62,21 +99,27 @@
 int l2 = 42;
 __try {
   might_crash();
-} __except(basic_filter(l2)) {
-  // FIXME: Test 'l1' when we can capture the lambda's 'this' decl.
+} __except (basic_filter(l1, l2)) {
 }
   };
   lambda();
 }
 
 // CHECK-LABEL: define internal void @"??R@?0??test_lambda@@YAXXZ@QEBA@XZ"(%class.anon* {{[^,]*}} %this)
-// CHECK: @llvm.localescape(i32* %[[l2_addr:[^, ]*]])
+// CHECK: @llvm.localescape(%class.anon** %[[this_addr:[^, ]*]], i32* %[[l2_addr:[^, ]*]])
+// CHECK: store %class.anon* %this, %class.anon** %[[this_addr]], align 8
 // CHECK: store i32 42, i32* %[[l2_addr]], align 4
 // CHECK: invoke void @might_crash()
 
 // CHECK-LABEL: define internal i32 @"?filt$0@0@?R@?0??test_lambda@@YAXXZ@"(i8* %exception_pointers, i8* %frame_pointer)
 // CHECK: %[[fp:[^ ]*]] = call i8* @llvm.eh.recoverfp(i8* bitcast (void (%class.anon*)* @"??R@?0??test_lambda@@YAXXZ@QEBA@XZ" to i8*), i8* %frame_pointer)
-// 

[PATCH] D97534: SEH: capture 'this'

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart updated this revision to Diff 326656.
ogoffart added a comment.

(git-clang-format)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97534/new/

https://reviews.llvm.org/D97534

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp

Index: clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
===
--- clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
+++ clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
@@ -39,13 +39,13 @@
   int l1 = 13;
   __try {
 might_crash();
-  } __except(basic_filter(l1)) {
-// FIXME: Test capturing 'this' and 'm1'.
+  } __except (basic_filter(l1, m1)) {
   }
 }
 
 // CHECK-LABEL: define dso_local void @"?test_method@S@@QEAAXXZ"(%struct.S* {{[^,]*}} %this)
-// CHECK: @llvm.localescape(i32* %[[l1_addr:[^, ]*]])
+// CHECK: @llvm.localescape(i32* %[[l1_addr:[^, ]*]], %struct.S** %[[this_addr:[^, ]*]])
+// CHECK: store %struct.S* %this, %struct.S** %[[this_addr]], align 8
 // CHECK: store i32 13, i32* %[[l1_addr]], align 4
 // CHECK: invoke void @might_crash()
 
@@ -53,8 +53,45 @@
 // CHECK: %[[fp:[^ ]*]] = call i8* @llvm.eh.recoverfp(i8* bitcast (void (%struct.S*)* @"?test_method@S@@QEAAXXZ" to i8*), i8* %frame_pointer)
 // CHECK: %[[l1_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.S*)* @"?test_method@S@@QEAAXXZ" to i8*), i8* %[[fp]], i32 0)
 // CHECK: %[[l1_ptr:[^ ]*]] = bitcast i8* %[[l1_i8]] to i32*
+// CHECK: %[[this_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.S*)* @"?test_method@S@@QEAAXXZ" to i8*), i8* %[[fp]], i32 1)
+// CHECK: %[[this_ptr:[^ ]*]] = bitcast i8* %[[this_i8]] to %struct.S**
+// CHECK: %[[this:[^ ]*]] = load %struct.S*, %struct.S** %[[this_ptr]], align 8
+// CHECK: %[[m1_ptr:[^ ]*]] = getelementptr inbounds %struct.S, %struct.S* %[[this]], i32 0, i32 0
+// CHECK: %[[m1:[^ ]*]] = load i32, i32* %[[m1_ptr]]
 // CHECK: %[[l1:[^ ]*]] = load i32, i32* %[[l1_ptr]]
-// CHECK: call i32 (i32, ...) @basic_filter(i32 %[[l1]])
+// CHECK: call i32 (i32, ...) @basic_filter(i32 %[[l1]], i32 %[[m1]])
+
+struct V {
+  void test_virtual(int p1);
+  virtual void virt(int p1);
+};
+
+void V::test_virtual(int p1) {
+  __try {
+might_crash();
+  } __finally {
+virt(p1);
+  }
+}
+
+// CHECK-LABEL: define dso_local void @"?test_virtual@V@@QEAAXH@Z"(%struct.V* {{[^,]*}} %this, i32 %p1)
+// CHECK: @llvm.localescape(%struct.V** %[[this_addr:[^, ]*]], i32* %[[p1_addr:[^, ]*]])
+// CHECK: store i32 %p1, i32* %[[p1_addr]], align 4
+// CHECK: store %struct.V* %this, %struct.V** %[[this_addr]], align 8
+// CHECK: invoke void @might_crash()
+
+// CHECK-LABEL: define internal void @"?fin$0@0@test_virtual@V@@"(i8 %abnormal_termination, i8* %frame_pointer)
+// CHECK: %[[this_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.V*, i32)* @"?test_virtual@V@@QEAAXH@Z" to i8*), i8* %frame_pointer, i32 0)
+// CHECK: %[[this_ptr:[^ ]*]] = bitcast i8* %[[this_i8]] to %struct.V**
+// CHECK: %[[this:[^ ]*]] = load %struct.V*, %struct.V** %[[this_ptr]], align 8
+// CHECK: %[[p1_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.V*, i32)* @"?test_virtual@V@@QEAAXH@Z" to i8*), i8* %frame_pointer, i32 1)
+// CHECK: %[[p1_ptr:[^ ]*]] = bitcast i8* %[[p1_i8]] to i32*
+// CHECK: %[[p1:[^ ]*]] = load i32, i32* %[[p1_ptr]]
+// CHECK: %[[this_2:[^ ]*]] = bitcast %struct.V* %[[this]] to void (%struct.V*, i32)***
+// CHECK: %[[vtable:[^ ]*]] = load void (%struct.V*, i32)**, void (%struct.V*, i32)*** %[[this_2]], align 8
+// CHECK: %[[vfn:[^ ]*]] = getelementptr inbounds void (%struct.V*, i32)*, void (%struct.V*, i32)** %[[vtable]], i64 0
+// CHECK: %[[virt:[^ ]*]] = load void (%struct.V*, i32)*, void (%struct.V*, i32)** %[[vfn]], align 8
+// CHECK: call void %[[virt]](%struct.V* {{[^,]*}} %[[this]], i32 %[[p1]])
 
 void test_lambda() {
   int l1 = 13;
@@ -62,21 +99,27 @@
 int l2 = 42;
 __try {
   might_crash();
-} __except(basic_filter(l2)) {
-  // FIXME: Test 'l1' when we can capture the lambda's 'this' decl.
+} __except (basic_filter(l1, l2)) {
 }
   };
   lambda();
 }
 
 // CHECK-LABEL: define internal void @"??R@?0??test_lambda@@YAXXZ@QEBA@XZ"(%class.anon* {{[^,]*}} %this)
-// CHECK: @llvm.localescape(i32* %[[l2_addr:[^, ]*]])
+// CHECK: @llvm.localescape(%class.anon** %[[this_addr:[^, ]*]], i32* %[[l2_addr:[^, ]*]])
+// CHECK: store %class.anon* %this, %class.anon** %[[this_addr]], align 8
 // CHECK: store i32 42, i32* %[[l2_addr]], align 4
 // CHECK: invoke void @might_crash()
 
 // CHECK-LABEL: define internal i32 @"?filt$0@0@?R@?0??test_lambda@@YAXXZ@"(i8* %exception_pointers, i8* %frame_pointer)
 // CHECK: %[[fp:[^ ]*]] = call i8* @llvm.eh.recoverfp(i8* bitcast (void (%class.anon*)* @"??R@?0??test_lambda@@YAXXZ@QEBA@XZ" to i8*), i8* %frame_pointer)
-// CHECK: %[[l2_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%class.anon*)* 

[PATCH] D97534: SEH: capture 'this'

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart created this revision.
ogoffart added reviewers: rnk, asl, cfe-commits.
ogoffart requested review of this revision.
Herald added a project: clang.

Simply make sure that the CodeGenFunction::CXXThisValue and CXXABIThisValue
are correctly initialized to the recovered value.
For lambda capture, we also need to make sure to fill the LambdaCaptureFields


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97534

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp

Index: clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
===
--- clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
+++ clang/test/CodeGenCXX/exceptions-seh-filter-captures.cpp
@@ -39,13 +39,13 @@
   int l1 = 13;
   __try {
 might_crash();
-  } __except(basic_filter(l1)) {
-// FIXME: Test capturing 'this' and 'm1'.
+  } __except(basic_filter(l1, m1)) {
   }
 }
 
 // CHECK-LABEL: define dso_local void @"?test_method@S@@QEAAXXZ"(%struct.S* {{[^,]*}} %this)
-// CHECK: @llvm.localescape(i32* %[[l1_addr:[^, ]*]])
+// CHECK: @llvm.localescape(i32* %[[l1_addr:[^, ]*]], %struct.S** %[[this_addr:[^, ]*]])
+// CHECK: store %struct.S* %this, %struct.S** %[[this_addr]], align 8
 // CHECK: store i32 13, i32* %[[l1_addr]], align 4
 // CHECK: invoke void @might_crash()
 
@@ -53,8 +53,46 @@
 // CHECK: %[[fp:[^ ]*]] = call i8* @llvm.eh.recoverfp(i8* bitcast (void (%struct.S*)* @"?test_method@S@@QEAAXXZ" to i8*), i8* %frame_pointer)
 // CHECK: %[[l1_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.S*)* @"?test_method@S@@QEAAXXZ" to i8*), i8* %[[fp]], i32 0)
 // CHECK: %[[l1_ptr:[^ ]*]] = bitcast i8* %[[l1_i8]] to i32*
+// CHECK: %[[this_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.S*)* @"?test_method@S@@QEAAXXZ" to i8*), i8* %[[fp]], i32 1)
+// CHECK: %[[this_ptr:[^ ]*]] = bitcast i8* %[[this_i8]] to %struct.S**
+// CHECK: %[[this:[^ ]*]] = load %struct.S*, %struct.S** %[[this_ptr]], align 8
+// CHECK: %[[m1_ptr:[^ ]*]] = getelementptr inbounds %struct.S, %struct.S* %[[this]], i32 0, i32 0
+// CHECK: %[[m1:[^ ]*]] = load i32, i32* %[[m1_ptr]]
 // CHECK: %[[l1:[^ ]*]] = load i32, i32* %[[l1_ptr]]
-// CHECK: call i32 (i32, ...) @basic_filter(i32 %[[l1]])
+// CHECK: call i32 (i32, ...) @basic_filter(i32 %[[l1]], i32 %[[m1]])
+
+
+struct V {
+  void test_virtual(int p1);
+  virtual void virt(int p1);
+};
+
+void V::test_virtual(int p1) {
+  __try {
+might_crash();
+  } __finally {
+virt(p1);
+  }
+}
+
+// CHECK-LABEL: define dso_local void @"?test_virtual@V@@QEAAXH@Z"(%struct.V* {{[^,]*}} %this, i32 %p1)
+// CHECK: @llvm.localescape(%struct.V** %[[this_addr:[^, ]*]], i32* %[[p1_addr:[^, ]*]])
+// CHECK: store i32 %p1, i32* %[[p1_addr]], align 4
+// CHECK: store %struct.V* %this, %struct.V** %[[this_addr]], align 8
+// CHECK: invoke void @might_crash()
+
+// CHECK-LABEL: define internal void @"?fin$0@0@test_virtual@V@@"(i8 %abnormal_termination, i8* %frame_pointer)
+// CHECK: %[[this_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.V*, i32)* @"?test_virtual@V@@QEAAXH@Z" to i8*), i8* %frame_pointer, i32 0)
+// CHECK: %[[this_ptr:[^ ]*]] = bitcast i8* %[[this_i8]] to %struct.V**
+// CHECK: %[[this:[^ ]*]] = load %struct.V*, %struct.V** %[[this_ptr]], align 8
+// CHECK: %[[p1_i8:[^ ]*]] = call i8* @llvm.localrecover(i8* bitcast (void (%struct.V*, i32)* @"?test_virtual@V@@QEAAXH@Z" to i8*), i8* %frame_pointer, i32 1)
+// CHECK: %[[p1_ptr:[^ ]*]] = bitcast i8* %[[p1_i8]] to i32*
+// CHECK: %[[p1:[^ ]*]] = load i32, i32* %[[p1_ptr]]
+// CHECK: %[[this_2:[^ ]*]] = bitcast %struct.V* %[[this]] to void (%struct.V*, i32)***
+// CHECK: %[[vtable:[^ ]*]] = load void (%struct.V*, i32)**, void (%struct.V*, i32)*** %[[this_2]], align 8
+// CHECK: %[[vfn:[^ ]*]] = getelementptr inbounds void (%struct.V*, i32)*, void (%struct.V*, i32)** %[[vtable]], i64 0
+// CHECK: %[[virt:[^ ]*]] = load void (%struct.V*, i32)*, void (%struct.V*, i32)** %[[vfn]], align 8
+// CHECK: call void %[[virt]](%struct.V* {{[^,]*}} %[[this]], i32 %[[p1]])
 
 void test_lambda() {
   int l1 = 13;
@@ -62,21 +100,28 @@
 int l2 = 42;
 __try {
   might_crash();
-} __except(basic_filter(l2)) {
-  // FIXME: Test 'l1' when we can capture the lambda's 'this' decl.
+} __except(basic_filter(l1, l2)) {
 }
   };
   lambda();
 }
 
 // CHECK-LABEL: define internal void @"??R@?0??test_lambda@@YAXXZ@QEBA@XZ"(%class.anon* {{[^,]*}} %this)
-// CHECK: @llvm.localescape(i32* %[[l2_addr:[^, ]*]])
+// CHECK: @llvm.localescape(%class.anon** %[[this_addr:[^, ]*]], i32* %[[l2_addr:[^, ]*]])
+// CHECK: store %class.anon* %this, %class.anon** %[[this_addr]], align 8
 // CHECK: store i32 42, i32* %[[l2_addr]], align 4
 // CHECK: invoke void @might_crash()
 
 // CHECK-LABEL: define internal i32 @"?filt$0@0@?R@?0??test_lambda@@YAXXZ@"(i8* %exception_pointers, i8* %frame_pointer)
 // CHECK: 

[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored

[Olivier Goffart via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL318900: Do not perform the analysis based warning if the 
warnings are ignored (authored by ogoffart).

Changed prior to commit:
  https://reviews.llvm.org/D40242?vs=123720=124041#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D40242

Files:
  cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
  cfe/trunk/unittests/Tooling/ToolingTest.cpp


Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2080,10 +2080,10 @@
   // time.
   DiagnosticsEngine  = S.getDiagnostics();
 
-  // Do not do any analysis for declarations in system headers if we are
-  // going to just ignore them.
-  if (Diags.getSuppressSystemWarnings() &&
-  S.SourceMgr.isInSystemHeader(D->getLocation()))
+  // Do not do any analysis if we are going to just ignore them.
+  if (Diags.getIgnoreAllWarnings() ||
+  (Diags.getSuppressSystemWarnings() &&
+   S.SourceMgr.isInSystemHeader(D->getLocation(
 return;
 
   // For code in dependent contexts, we'll do this at instantiation time.
Index: cfe/trunk/unittests/Tooling/ToolingTest.cpp
===
--- cfe/trunk/unittests/Tooling/ToolingTest.cpp
+++ cfe/trunk/unittests/Tooling/ToolingTest.cpp
@@ -564,5 +564,31 @@
 }
 #endif
 
+TEST(runToolOnCode, TestResetDiagnostics) {
+  // This is a tool that resets the diagnostic during the compilation.
+  struct ResetDiagnosticAction : public clang::ASTFrontendAction {
+std::unique_ptr CreateASTConsumer(CompilerInstance ,
+   StringRef) override {
+  struct Consumer : public clang::ASTConsumer {
+bool HandleTopLevelDecl(clang::DeclGroupRef D) override {
+  auto  = (*D.begin())->getASTContext().getDiagnostics();
+  // Ignore any error
+  Diags.Reset();
+  // Disable warnings because computing the CFG might crash.
+  Diags.setIgnoreAllWarnings(true);
+  return true;
+}
+  };
+  return llvm::make_unique();
+}
+  };
+
+  // Should not crash
+  EXPECT_FALSE(
+  runToolOnCode(new ResetDiagnosticAction,
+"struct Foo { Foo(int); ~Foo(); struct Fwd _fwd; };"
+"void func() { long x; Foo f(x); }"));
+}
+
 } // end namespace tooling
 } // end namespace clang


Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2080,10 +2080,10 @@
   // time.
   DiagnosticsEngine  = S.getDiagnostics();
 
-  // Do not do any analysis for declarations in system headers if we are
-  // going to just ignore them.
-  if (Diags.getSuppressSystemWarnings() &&
-  S.SourceMgr.isInSystemHeader(D->getLocation()))
+  // Do not do any analysis if we are going to just ignore them.
+  if (Diags.getIgnoreAllWarnings() ||
+  (Diags.getSuppressSystemWarnings() &&
+   S.SourceMgr.isInSystemHeader(D->getLocation(
 return;
 
   // For code in dependent contexts, we'll do this at instantiation time.
Index: cfe/trunk/unittests/Tooling/ToolingTest.cpp
===
--- cfe/trunk/unittests/Tooling/ToolingTest.cpp
+++ cfe/trunk/unittests/Tooling/ToolingTest.cpp
@@ -564,5 +564,31 @@
 }
 #endif
 
+TEST(runToolOnCode, TestResetDiagnostics) {
+  // This is a tool that resets the diagnostic during the compilation.
+  struct ResetDiagnosticAction : public clang::ASTFrontendAction {
+std::unique_ptr CreateASTConsumer(CompilerInstance ,
+   StringRef) override {
+  struct Consumer : public clang::ASTConsumer {
+bool HandleTopLevelDecl(clang::DeclGroupRef D) override {
+  auto  = (*D.begin())->getASTContext().getDiagnostics();
+  // Ignore any error
+  Diags.Reset();
+  // Disable warnings because computing the CFG might crash.
+  Diags.setIgnoreAllWarnings(true);
+  return true;
+}
+  };
+  return llvm::make_unique();
+}
+  };
+
+  // Should not crash
+  EXPECT_FALSE(
+  runToolOnCode(new ResetDiagnosticAction,
+"struct Foo { Foo(int); ~Foo(); struct Fwd _fwd; };"
+"void func() { long x; Foo f(x); }"));
+}
+
 } // end namespace tooling
 } // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart updated this revision to Diff 123720.
ogoffart added a comment.
Herald added a subscriber: klimek.

I added a test.
I did not add such test before because i know that calling Reset() on the 
diagnostics is kind of a hack.

This change does not have any behavioral difference normally, it is just an 
optimization that skips the computation of the CFG if all warnings are ignored.


https://reviews.llvm.org/D40242

Files:
  lib/Sema/AnalysisBasedWarnings.cpp
  unittests/Tooling/ToolingTest.cpp


Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -533,5 +533,31 @@
 }
 #endif
 
+TEST(runToolOnCode, TestResetDiagnostics) {
+  // This is a tool that resets the diagnostic during the compilation.
+  struct ResetDiagnosticAction : public clang::ASTFrontendAction {
+std::unique_ptr CreateASTConsumer(CompilerInstance ,
+   StringRef) override {
+  struct Consumer : public clang::ASTConsumer {
+bool HandleTopLevelDecl(clang::DeclGroupRef D) override {
+  auto  = (*D.begin())->getASTContext().getDiagnostics();
+  // Ignore any error
+  Diags.Reset();
+  // Disable warnings because computing the CFG might crash.
+  Diags.setIgnoreAllWarnings(true);
+  return true;
+}
+  };
+  return llvm::make_unique();
+}
+  };
+
+  // Should not crash
+  EXPECT_FALSE(
+  runToolOnCode(new ResetDiagnosticAction,
+"struct Foo { Foo(int); ~Foo(); struct Fwd _fwd; };"
+"void func() { long x; Foo f(x); }"));
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -2080,10 +2080,10 @@
   // time.
   DiagnosticsEngine  = S.getDiagnostics();
 
-  // Do not do any analysis for declarations in system headers if we are
-  // going to just ignore them.
-  if (Diags.getSuppressSystemWarnings() &&
-  S.SourceMgr.isInSystemHeader(D->getLocation()))
+  // Do not do any analysis if we are going to just ignore them.
+  if (Diags.getIgnoreAllWarnings() ||
+  (Diags.getSuppressSystemWarnings() &&
+   S.SourceMgr.isInSystemHeader(D->getLocation(
 return;
 
   // For code in dependent contexts, we'll do this at instantiation time.


Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -533,5 +533,31 @@
 }
 #endif
 
+TEST(runToolOnCode, TestResetDiagnostics) {
+  // This is a tool that resets the diagnostic during the compilation.
+  struct ResetDiagnosticAction : public clang::ASTFrontendAction {
+std::unique_ptr CreateASTConsumer(CompilerInstance ,
+   StringRef) override {
+  struct Consumer : public clang::ASTConsumer {
+bool HandleTopLevelDecl(clang::DeclGroupRef D) override {
+  auto  = (*D.begin())->getASTContext().getDiagnostics();
+  // Ignore any error
+  Diags.Reset();
+  // Disable warnings because computing the CFG might crash.
+  Diags.setIgnoreAllWarnings(true);
+  return true;
+}
+  };
+  return llvm::make_unique();
+}
+  };
+
+  // Should not crash
+  EXPECT_FALSE(
+  runToolOnCode(new ResetDiagnosticAction,
+"struct Foo { Foo(int); ~Foo(); struct Fwd _fwd; };"
+"void func() { long x; Foo f(x); }"));
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -2080,10 +2080,10 @@
   // time.
   DiagnosticsEngine  = S.getDiagnostics();
 
-  // Do not do any analysis for declarations in system headers if we are
-  // going to just ignore them.
-  if (Diags.getSuppressSystemWarnings() &&
-  S.SourceMgr.isInSystemHeader(D->getLocation()))
+  // Do not do any analysis if we are going to just ignore them.
+  if (Diags.getIgnoreAllWarnings() ||
+  (Diags.getSuppressSystemWarnings() &&
+   S.SourceMgr.isInSystemHeader(D->getLocation(
 return;
 
   // For code in dependent contexts, we'll do this at instantiation time.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart marked an inline comment as done.
ogoffart added a comment.

> You should add a test case that demonstrates code which would otherwise 
> trigger an analysis-based warning but doesn't due to disabling all warnings.

No warnings are triggered. Because the diagnostic engine ignores them.
This just optimize the code so no analysis are preformed when no warning can be 
triggered.


https://reviews.llvm.org/D40242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart marked an inline comment as done.
ogoffart added inline comments.



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:2084
+  // Do not do any analysis if we are going to just ignore them.
+  if (Diags.getIgnoreAllWarnings() ||
+  (Diags.getSuppressSystemWarnings() &&

lebedev.ri wrote:
> I guess you can try commenting-out that entire check and seeing whether any 
> test fails.
Commenting out the entire if does not cause any test to fail.


https://reviews.llvm.org/D40242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

I do not know how to add a test: there is no real visible change for clang. It 
just should be faster when passing "-w".


https://reviews.llvm.org/D40242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40242: Do not perform the analysis based warning if all warnings are ignored

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart created this revision.

Do not perform the analysis based warning if all warnings are ignored.

This saves some cycles when compiling with "-w".

But more importantly, for my tool, this fixes a potential crash which may 
happen on invalid code. Because the analysis might compute the CFG which 
crashes if the code contains invalid declaration. This does not happen normally 
with because we also don't perform these analysis if there was an error.  But 
the tool is better at recovering and hit this condition.


https://reviews.llvm.org/D40242

Files:
  lib/Sema/AnalysisBasedWarnings.cpp


Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -2080,10 +2080,10 @@
   // time.
   DiagnosticsEngine  = S.getDiagnostics();
 
-  // Do not do any analysis for declarations in system headers if we are
-  // going to just ignore them.
-  if (Diags.getSuppressSystemWarnings() &&
-  S.SourceMgr.isInSystemHeader(D->getLocation()))
+  // Do not do any analysis if we are going to just ignore them.
+  if (Diags.getIgnoreAllWarnings() ||
+  (Diags.getSuppressSystemWarnings() &&
+   S.SourceMgr.isInSystemHeader(D->getLocation(
 return;
 
   // For code in dependent contexts, we'll do this at instantiation time.


Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -2080,10 +2080,10 @@
   // time.
   DiagnosticsEngine  = S.getDiagnostics();
 
-  // Do not do any analysis for declarations in system headers if we are
-  // going to just ignore them.
-  if (Diags.getSuppressSystemWarnings() &&
-  S.SourceMgr.isInSystemHeader(D->getLocation()))
+  // Do not do any analysis if we are going to just ignore them.
+  if (Diags.getIgnoreAllWarnings() ||
+  (Diags.getSuppressSystemWarnings() &&
+   S.SourceMgr.isInSystemHeader(D->getLocation(
 return;
 
   // For code in dependent contexts, we'll do this at instantiation time.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26350: Keep invalid Switch in the AST

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart updated this revision to Diff 119142.
ogoffart added a comment.

Updated the patch so that ActOnStartOfSwitchStmt returns void, and 
ActOnFinishSwitchStmt will skip some checks in case of error


https://reviews.llvm.org/D26350

Files:
  include/clang/Sema/Sema.h
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaStmt.cpp
  lib/Sema/TreeTransform.h
  test/Misc/ast-dump-invalid-switch.cpp
  test/SemaCXX/switch.cpp

Index: test/SemaCXX/switch.cpp
===
--- test/SemaCXX/switch.cpp
+++ test/SemaCXX/switch.cpp
@@ -130,3 +130,20 @@
 }
 
 }
+
+namespace InvalidCondition {
+enum class color { red,
+   blue,
+   green };
+void test() {
+  // When the condition is invalid, there should be no errors or warnings
+  switch (invalidCode) { // expected-error {{use of undeclared identifier}}
+  case 0:
+  case -(1ll << 62) - 1:
+  case (1ll << 62) + 1:
+  case color::red:
+  default:
+break;
+  }
+}
+} // namespace InvalidCondition
Index: test/Misc/ast-dump-invalid-switch.cpp
===
--- /dev/null
+++ test/Misc/ast-dump-invalid-switch.cpp
@@ -0,0 +1,105 @@
+// RUN: not %clang_cc1 -std=c++11 -triple x86_64-linux-gnu -fms-extensions -ast-dump -ast-dump-filter Test %s | FileCheck -check-prefix CHECK -strict-whitespace %s
+
+/* This test ensures that the AST is still complete, even for invalid code */
+
+namespace TestInvalidSwithCondition {
+int f(int x) {
+  switch (_invalid_) {
+  case 0:
+return 1;
+  default:
+return 2;
+  }
+}
+}
+
+// CHECK: NamespaceDecl {{.*}} TestInvalidSwithCondition
+// CHECK-NEXT: `-FunctionDecl
+// CHECK-NEXT:   |-ParmVarDecl
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT: `-SwitchStmt
+// CHECK-NEXT:   |-<<>>
+// CHECK-NEXT:   |-<<>>
+// CHECK-NEXT:   |-OpaqueValueExpr
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT: |-CaseStmt
+// CHECK-NEXT: | |-IntegerLiteral {{.*}} 'int' 0
+// CHECK-NEXT: | |-<<>>
+// CHECK-NEXT: | `-ReturnStmt
+// CHECK-NEXT: |   `-IntegerLiteral {{.*}} 'int' 1
+// CHECK-NEXT: `-DefaultStmt
+// CHECK-NEXT:   `-ReturnStmt
+// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
+
+namespace TestSwitchConditionNotIntegral {
+int g(int *x) {
+  switch (x) {
+  case 0:
+return 1;
+  default:
+return 2;
+  }
+}
+}
+
+// CHECK: NamespaceDecl {{.*}} TestSwitchConditionNotIntegral
+// CHECK-NEXT: `-FunctionDecl
+// CHECK-NEXT:   |-ParmVarDecl
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT: `-SwitchStmt
+// CHECK-NEXT:   |-<<>>
+// CHECK-NEXT:   |-<<>>
+// CHECK-NEXT:   |-ImplicitCastExpr
+// CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'x' 'int *'
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT: |-CaseStmt
+// CHECK-NEXT: | |-IntegerLiteral {{.*}} 'int' 0
+// CHECK-NEXT: | |-<<>>
+// CHECK-NEXT: | `-ReturnStmt
+// CHECK-NEXT: |   `-IntegerLiteral {{.*}} 'int' 1
+// CHECK-NEXT: `-DefaultStmt
+// CHECK-NEXT:   `-ReturnStmt
+// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
+
+namespace TestSwitchInvalidCases {
+int g(int x) {
+  switch (x) {
+  case _invalid_:
+return 1;
+  case _invalid_:
+return 2;
+  case x:
+return 3;
+  default:
+return 4;
+  default:
+return 5;
+  }
+}
+}
+
+// CHECK: NamespaceDecl {{.*}} TestSwitchInvalidCases
+// CHECK-NEXT: `-FunctionDecl
+// CHECK-NEXT:   |-ParmVarDecl
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT: `-SwitchStmt
+// CHECK-NEXT:   |-<<>>
+// CHECK-NEXT:   |-<<>>
+// CHECK-NEXT:   |-ImplicitCastExpr
+// CHECK-NEXT:   | `-DeclRefExpr {{.*}}'x' 'int'
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT: |-ReturnStmt
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK-NEXT: |-ReturnStmt
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 2
+// CHECK-NEXT: |-CaseStmt
+// CHECK-NEXT: | |-DeclRefExpr {{.*}} 'x' 'int'
+// CHECK-NEXT: | |-<<>>
+// CHECK-NEXT: | `-ReturnStmt
+// CHECK-NEXT: |   `-IntegerLiteral {{.*}} 'int' 3
+// CHECK-NEXT: |-DefaultStmt
+// CHECK-NEXT: | `-ReturnStmt
+// CHECK-NEXT: |   `-IntegerLiteral {{.*}} 'int' 4
+// CHECK-NEXT: `-DefaultStmt
+// CHECK-NEXT:   `-ReturnStmt
+// CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 5
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -1256,18 +1256,17 @@
   ///
   /// By default, performs semantic analysis to build the new statement.
   /// Subclasses may override this routine to provide different behavior.
-  StmtResult RebuildSwitchStmtStart(SourceLocation SwitchLoc, Stmt *Init,
-Sema::ConditionResult Cond) {
-return 

[PATCH] D35190: __builtin_constant_p should consider the parameter of a constexpr function as constant

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

ping


https://reviews.llvm.org/D35190



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26350: Keep invalid Switch in the AST

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

Thanks for your review and i'll try to address the concerns.

I believe tools that really need valid code relies on the diagnostics and bail 
out on error. On the other hand, tools that may work on code containing error 
do a best effort to work on the remaining AST .
And patches like this one improve the remaining AST, so tools like clang-tidy 
will be able to also do valid transformation inside the switch statement, which 
could not have been possible when the whole body is gone.

The AST stays "valid" in the sense that all the nodes exist (no nullptr) and so 
conform to the expectation of the code.  The condition of a switch may now be 
an OpaqueValueExpr which should not disturb the matchers.




Comment at: lib/Parse/ParseStmt.cpp:1297
 
-  if (Switch.isInvalid()) {
-// Skip the switch body.
-// FIXME: This is not optimal recovery, but parsing the body is more
-// dangerous due to the presence of case and default statements, which
-// will have no place to connect back with the switch.
-if (Tok.is(tok::l_brace)) {
-  ConsumeBrace();
-  SkipUntil(tok::r_brace);
-} else
-  SkipUntil(tok::semi);
-return Switch;
-  }
+  assert(!Switch.isInvalid() && "ActOnStartOfSwitchStmt always succeed");
 

aaron.ballman wrote:
> succeed -> succeeds
> 
> Are the concerns pointed out in the FIXME addressed by code not in this patch?
The FIXME is pointing out problems occuring if the parser found 'default' or 
'case' statement, but cannot connect it to the corresponding 'switch' statement 
(because that switch statement did not exist as it was removed from the AST). 

Now that we always keep the 'switch' statement, this is no longer a problem.



Comment at: lib/Sema/SemaStmt.cpp:669
   if (Cond.isInvalid())
-return StmtError();
+Cond = ConditionResult(
+*this, nullptr,

aaron.ballman wrote:
> This makes the condition result valid when it isn't. Users of this condition 
> result may expect a valid condition result to return nonnull values when 
> calling `get()`, which makes me uncomfortable.
Get return a non-null value.
That's why i'm constructing an OpaqueValueExpr placeholder expression.

The ConditionVar (nullptr in the line bellow) can be null. It is null in valid 
code most of the time actually, when one does not declare a new variable in in 
condition.

But the result is that users of this condition will get a OpaqueValueExpr when 
calling get and should not be disturbed by that as they will just take that as 
an expression.



https://reviews.llvm.org/D26350



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26350: Keep invalid Switch in the AST

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

The problem i'm trying to solve is precisely to keep as much as possible of the 
valid AST in the main AST, despite errors.
I've already done some work with r249982, r272962 and more, and there is still 
a lot to do. But the goal is to keep as much as possible of it.

The reason i'm working on this is highlighting of code where some code might be 
potentially invalid (because you are editing it, or because the tool don't have 
access to all headers)
Things like if statement from my previous patch, or switch statement like this 
patch are the things which have the more impact, because not keeping them 
removes highlighting for potentially big blocks of code.

This is useful for example for IDE such as KDevelop which use clang for 
highlighting, or my own tool [code.woboq.org]

A random example is 
https://code.woboq.org/linux/linux/arch/arm/kernel/module.c.html?style=kdevelop#101
(generated with this patch applied.) There are errors because this is an arm 
file built with the option for an intel kernel. Yet, most of the file is 
properly highlighted. If this patch was not applied, the whole switch statement 
would be removed from the AST and nothing within would be hightlighted, only 
because some case label are invalid.


https://reviews.llvm.org/D26350



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35172: Keep the IdentifierInfo in the Token for alternative operator keyword

[Olivier Goffart via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL308008: Keep the IdentifierInfo in the Token for alternative 
operator keyword (authored by ogoffart).

Changed prior to commit:
  https://reviews.llvm.org/D35172?vs=105816=106594#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D35172

Files:
  cfe/trunk/include/clang/Basic/IdentifierTable.h
  cfe/trunk/lib/Lex/PPDirectives.cpp
  cfe/trunk/lib/Lex/PPExpressions.cpp
  cfe/trunk/lib/Lex/Preprocessor.cpp
  cfe/trunk/test/Parser/MicrosoftExtensions.cpp
  cfe/trunk/test/Preprocessor/cxx_oper_keyword.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/lib/Lex/PPDirectives.cpp
===
--- cfe/trunk/lib/Lex/PPDirectives.cpp
+++ cfe/trunk/lib/Lex/PPDirectives.cpp
@@ -220,26 +220,18 @@
 return Diag(MacroNameTok, diag::err_pp_missing_macro_name);
 
   IdentifierInfo *II = MacroNameTok.getIdentifierInfo();
-  if (!II) {
-bool Invalid = false;
-std::string Spelling = getSpelling(MacroNameTok, );
-if (Invalid)
-  return Diag(MacroNameTok, diag::err_pp_macro_not_identifier);
-II = getIdentifierInfo(Spelling);
-
-if (!II->isCPlusPlusOperatorKeyword())
-  return Diag(MacroNameTok, diag::err_pp_macro_not_identifier);
+  if (!II)
+return Diag(MacroNameTok, diag::err_pp_macro_not_identifier);
 
+  if (II->isCPlusPlusOperatorKeyword()) {
 // C++ 2.5p2: Alternative tokens behave the same as its primary token
 // except for their spellings.
 Diag(MacroNameTok, getLangOpts().MicrosoftExt
? diag::ext_pp_operator_used_as_macro_name
: diag::err_pp_operator_used_as_macro_name)
 << II << MacroNameTok.getKind();
-
 // Allow #defining |and| and friends for Microsoft compatibility or
 // recovery when legacy C headers are included in C++.
-MacroNameTok.setIdentifierInfo(II);
   }
 
   if ((isDefineUndef != MU_Other) && II->getPPKeywordID() == tok::pp_defined) {
Index: cfe/trunk/lib/Lex/Preprocessor.cpp
===
--- cfe/trunk/lib/Lex/Preprocessor.cpp
+++ cfe/trunk/lib/Lex/Preprocessor.cpp
@@ -712,14 +712,6 @@
 II.setIsFutureCompatKeyword(false);
   }
 
-  // C++ 2.11p2: If this is an alternative representation of a C++ operator,
-  // then we act as if it is the actual operator and not the textual
-  // representation of it.
-  if (II.isCPlusPlusOperatorKeyword() &&
-  !(getLangOpts().MSVCCompat &&
-getSourceManager().isInSystemHeader(Identifier.getLocation(
-Identifier.setIdentifierInfo(nullptr);
-
   // If this is an extension token, diagnose its use.
   // We avoid diagnosing tokens that originate from macro definitions.
   // FIXME: This warning is disabled in cases where it shouldn't be,
Index: cfe/trunk/lib/Lex/PPExpressions.cpp
===
--- cfe/trunk/lib/Lex/PPExpressions.cpp
+++ cfe/trunk/lib/Lex/PPExpressions.cpp
@@ -237,35 +237,32 @@
 PP.setCodeCompletionReached();
 PP.LexNonComment(PeekTok);
   }
-  
-  // If this token's spelling is a pp-identifier, check to see if it is
-  // 'defined' or if it is a macro.  Note that we check here because many
-  // keywords are pp-identifiers, so we can't check the kind.
-  if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {
-// Handle "defined X" and "defined(X)".
-if (II->isStr("defined"))
-  return EvaluateDefined(Result, PeekTok, DT, ValueLive, PP);
-
-// If this identifier isn't 'defined' or one of the special
-// preprocessor keywords and it wasn't macro expanded, it turns
-// into a simple 0, unless it is the C++ keyword "true", in which case it
-// turns into "1".
-if (ValueLive &&
-II->getTokenID() != tok::kw_true &&
-II->getTokenID() != tok::kw_false)
-  PP.Diag(PeekTok, diag::warn_pp_undef_identifier) << II;
-Result.Val = II->getTokenID() == tok::kw_true;
-Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
-Result.setIdentifier(II);
-Result.setRange(PeekTok.getLocation());
-DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
-   II->getTokenID() != tok::kw_false);
-PP.LexNonComment(PeekTok);
-return false;
-  }
 
   switch (PeekTok.getKind()) {
-  default:  // Non-value token.
+  default:
+// If this token's spelling is a pp-identifier, check to see if it is
+// 'defined' or if it is a macro.  Note that we check here because many
+// keywords are pp-identifiers, so we can't check the kind.
+if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {
+  // Handle "defined X" and "defined(X)".
+  if (II->isStr("defined"))
+return EvaluateDefined(Result, PeekTok, DT, ValueLive, PP);
+
+  if (!II->isCPlusPlusOperatorKeyword()) {
+// If this identifier isn't 'defined' or 

[PATCH] D26350: Keep invalid Switch in the AST

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

re-ping


https://reviews.llvm.org/D26350



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35190: __builtin_constant_p should consider the parameter of a constexpr function as constant

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

Is there any objections for this patch?


https://reviews.llvm.org/D35190



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35190: __builtin_constant_p should consider the parameter of a constexpr function as constant

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

Thanks for the link to the bug report.
This patch mostly address the evaluation within a constexpr, which is 
orthogonal to the changes in the backend suggered in that bug report.


https://reviews.llvm.org/D35190



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35190: __builtin_constant_p should consider the parameter of a constexpr function as constant

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

>   What if the constexpr function is called with in a non-constexpr context, 
> e.g. with a random global variable as argument?

__builtin_constant_p will still return false in this case, contrary to gcc 
(whose behavior actually depends on the optimization level).

I think if we really want to get the exact gcc behavior, we need to mix it with 
the optimizer and generate some LLVM intrinsics. But what I was interested on 
with this patch is at least get  a closer behavior as GCC in a constexpr context


https://reviews.llvm.org/D35190



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35190: __builtin_constant_p should consider the parameter of a constexpr function as constant

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

Ping


https://reviews.llvm.org/D35190



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35190: __builtin_constant_p should consider the parameter of a constexpr function as constant

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart created this revision.

This allows to do something like:

  
  constexpr int func(int x) {
return __builtin_constant_p(x) ? compute_constexpr(x) : compute_runtime(x);
  }


This kind of code is accepted by GCC since GCC 4.8.

  

The problem was that EvaluateBuiltinConstantP was discarding the EvalInfo and 
its frames. So just keep the EvalInfo


https://reviews.llvm.org/D35190

Files:
  lib/AST/ExprConstant.cpp
  test/SemaCXX/constant-expression-cxx11.cpp
  test/SemaCXX/constant-expression-cxx1y.cpp


Index: test/SemaCXX/constant-expression-cxx1y.cpp
===
--- test/SemaCXX/constant-expression-cxx1y.cpp
+++ test/SemaCXX/constant-expression-cxx1y.cpp
@@ -982,3 +982,8 @@
   int *p = 
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 
18446744073709551615 of non-array object in a constant expression}}
 }
+
+namespace BuiltinConstantP {
+  constexpr int f1(int i) { return __builtin_constant_p(i += 4) ? 0 : i += 3; }
+  static_assert(f1(10) == 13, "");
+}
Index: test/SemaCXX/constant-expression-cxx11.cpp
===
--- test/SemaCXX/constant-expression-cxx11.cpp
+++ test/SemaCXX/constant-expression-cxx11.cpp
@@ -2169,3 +2169,13 @@
   constexpr int *q = ( + 1) - (unsigned __int128)-1; // expected-error 
{{constant expression}} expected-note {{cannot refer to element -3402}}
   constexpr int *r = &( + 1)[(unsigned __int128)-1]; // expected-error 
{{constant expression}} expected-note {{cannot refer to element 3402}}
 }
+
+namespace BuiltinConstantP {
+  int computeRuntime(int);
+  constexpr int compute(int x) {
+return __builtin_constant_p(x) ? x + 2 : computeRuntime(x);
+  }
+  constexpr int x = compute(25);
+  int n; // expected-note{{declared here}}
+  constexpr int z = compute(n); // expected-error {{constant expression}} 
expected-note{{non-const variable}}
+}
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -7203,7 +7203,7 @@
 
 /// EvaluateBuiltinConstantP - Evaluate __builtin_constant_p as similarly to
 /// GCC as we can manage.
-static bool EvaluateBuiltinConstantP(ASTContext , const Expr *Arg) {
+static bool EvaluateBuiltinConstantP(EvalInfo , const Expr *Arg) {
   QualType ArgType = Arg->getType();
 
   // __builtin_constant_p always has one operand. The rules which gcc follows
@@ -7219,25 +7219,24 @@
   //
   // FIXME: GCC also intends to return 1 for literals of aggregate types, but
   // its support for this does not currently work.
-  if (ArgType->isIntegralOrEnumerationType()) {
-Expr::EvalResult Result;
-if (!Arg->EvaluateAsRValue(Result, Ctx) || Result.HasSideEffects)
+  SpeculativeEvaluationRAII SpeculativeEval(Info);
+  FoldConstant Fold(Info, true);
+  if (ArgType->isIntegralOrEnumerationType() || ArgType->isFloatingType() ||
+  ArgType->isAnyComplexType()) {
+APValue V;
+if (!EvaluateAsRValue(Info, Arg, V) || Info.EvalStatus.HasSideEffects)
   return false;
-
-APValue  = Result.Val;
-if (V.getKind() == APValue::Int)
+if (V.getKind() == APValue::Int || V.getKind() == APValue::Float ||
+V.getKind() == APValue::ComplexInt ||
+V.getKind() == APValue::ComplexFloat)
   return true;
 if (V.getKind() == APValue::LValue)
   return EvaluateBuiltinConstantPForLValue(V);
-  } else if (ArgType->isFloatingType() || ArgType->isAnyComplexType()) {
-return Arg->isEvaluatable(Ctx);
   } else if (ArgType->isPointerType() || Arg->isGLValue()) {
 LValue LV;
-Expr::EvalStatus Status;
-EvalInfo Info(Ctx, Status, EvalInfo::EM_ConstantFold);
 if ((Arg->isGLValue() ? EvaluateLValue(Arg, LV, Info)
   : EvaluatePointer(Arg, LV, Info)) &&
-!Status.HasSideEffects)
+!Info.EvalStatus.HasSideEffects)
   return EvaluateBuiltinConstantPForLValue(LV);
   }
 
@@ -7628,7 +7627,7 @@
   }
 
   case Builtin::BI__builtin_constant_p:
-return Success(EvaluateBuiltinConstantP(Info.Ctx, E->getArg(0)), E);
+return Success(EvaluateBuiltinConstantP(Info, E->getArg(0)), E);
 
   case Builtin::BI__builtin_ctz:
   case Builtin::BI__builtin_ctzl:


Index: test/SemaCXX/constant-expression-cxx1y.cpp
===
--- test/SemaCXX/constant-expression-cxx1y.cpp
+++ test/SemaCXX/constant-expression-cxx1y.cpp
@@ -982,3 +982,8 @@
   int *p = 
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
 }
+
+namespace BuiltinConstantP {
+  constexpr int f1(int i) { return __builtin_constant_p(i += 4) ? 0 : i += 3; }
+  static_assert(f1(10) == 13, "");
+}
Index: test/SemaCXX/constant-expression-cxx11.cpp
===
--- 

[PATCH] D35172: Keep the IdentifierInfo in the Token for alternative operator keyword

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart updated this revision to Diff 105816.

https://reviews.llvm.org/D35172

Files:
  include/clang/Basic/IdentifierTable.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPExpressions.cpp
  lib/Lex/Preprocessor.cpp
  test/Parser/MicrosoftExtensions.cpp
  test/Preprocessor/cxx_oper_keyword.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -333,6 +333,16 @@
   verifyFormat("x = (a) xor (b);");
 }
 
+TEST_F(FormatTest, RecognizesUnaryOperatorKeywords) {
+  verifyFormat("x = compl(a);");
+  verifyFormat("x = not(a);");
+  verifyFormat("x = bitand(a);");
+  // Unary operator must not be merged with the next identifier
+  verifyFormat("x = compl a;");
+  verifyFormat("x = not a;");
+  verifyFormat("x = bitand a;");
+}
+
 //===--===//
 // Tests for control statements.
 //===--===//
Index: test/Preprocessor/cxx_oper_keyword.cpp
===
--- test/Preprocessor/cxx_oper_keyword.cpp
+++ test/Preprocessor/cxx_oper_keyword.cpp
@@ -29,3 +29,15 @@
 #ifdef and
 #warning and is defined
 #endif
+
+#ifdef OPERATOR_NAMES
+//expected-error@+2 {{invalid token at start of a preprocessor expression}}
+#endif
+#if or
+#endif
+
+#ifdef OPERATOR_NAMES
+//expected-error@+2 {{invalid token at start of a preprocessor expression}}
+#endif
+#if and_eq
+#endif
Index: test/Parser/MicrosoftExtensions.cpp
===
--- test/Parser/MicrosoftExtensions.cpp
+++ test/Parser/MicrosoftExtensions.cpp
@@ -261,9 +261,7 @@
 #define identifier_weird(x) __identifier(x
 int k = identifier_weird(if)); // expected-error {{use of undeclared identifier 'if'}}
 
-// This is a bit weird, but the alternative tokens aren't keywords, and this
-// behavior matches MSVC. FIXME: Consider supporting this anyway.
-extern int __identifier(and) r; // expected-error {{cannot convert '&&' token to an identifier}}
+extern int __identifier(and);
 
 void f() {
   __identifier(() // expected-error {{cannot convert '(' token to an identifier}}
Index: lib/Lex/Preprocessor.cpp
===
--- lib/Lex/Preprocessor.cpp
+++ lib/Lex/Preprocessor.cpp
@@ -710,14 +710,6 @@
 II.setIsFutureCompatKeyword(false);
   }
 
-  // C++ 2.11p2: If this is an alternative representation of a C++ operator,
-  // then we act as if it is the actual operator and not the textual
-  // representation of it.
-  if (II.isCPlusPlusOperatorKeyword() &&
-  !(getLangOpts().MSVCCompat &&
-getSourceManager().isInSystemHeader(Identifier.getLocation(
-Identifier.setIdentifierInfo(nullptr);
-
   // If this is an extension token, diagnose its use.
   // We avoid diagnosing tokens that originate from macro definitions.
   // FIXME: This warning is disabled in cases where it shouldn't be,
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -237,35 +237,32 @@
 PP.setCodeCompletionReached();
 PP.LexNonComment(PeekTok);
   }
-  
-  // If this token's spelling is a pp-identifier, check to see if it is
-  // 'defined' or if it is a macro.  Note that we check here because many
-  // keywords are pp-identifiers, so we can't check the kind.
-  if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {
-// Handle "defined X" and "defined(X)".
-if (II->isStr("defined"))
-  return EvaluateDefined(Result, PeekTok, DT, ValueLive, PP);
-
-// If this identifier isn't 'defined' or one of the special
-// preprocessor keywords and it wasn't macro expanded, it turns
-// into a simple 0, unless it is the C++ keyword "true", in which case it
-// turns into "1".
-if (ValueLive &&
-II->getTokenID() != tok::kw_true &&
-II->getTokenID() != tok::kw_false)
-  PP.Diag(PeekTok, diag::warn_pp_undef_identifier) << II;
-Result.Val = II->getTokenID() == tok::kw_true;
-Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
-Result.setIdentifier(II);
-Result.setRange(PeekTok.getLocation());
-DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
-   II->getTokenID() != tok::kw_false);
-PP.LexNonComment(PeekTok);
-return false;
-  }
 
   switch (PeekTok.getKind()) {
-  default:  // Non-value token.
+  default:
+// If this token's spelling is a pp-identifier, check to see if it is
+// 'defined' or if it is a macro.  Note that we check here because many
+// keywords are pp-identifiers, so we can't check the kind.
+if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {
+  // Handle "defined X" and 

[PATCH] D35172: Keep the IdentifierInfo in the Token for alternative operator keyword

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart updated this revision to Diff 105815.
ogoffart marked an inline comment as done.
ogoffart added a comment.

Added check for  "#if and_eq"


https://reviews.llvm.org/D35172

Files:
  include/clang/Basic/IdentifierTable.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPExpressions.cpp
  lib/Lex/Preprocessor.cpp
  test/Parser/MicrosoftExtensions.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -333,6 +333,16 @@
   verifyFormat("x = (a) xor (b);");
 }
 
+TEST_F(FormatTest, RecognizesUnaryOperatorKeywords) {
+  verifyFormat("x = compl(a);");
+  verifyFormat("x = not(a);");
+  verifyFormat("x = bitand(a);");
+  // Unary operator must not be merged with the next identifier
+  verifyFormat("x = compl a;");
+  verifyFormat("x = not a;");
+  verifyFormat("x = bitand a;");
+}
+
 //===--===//
 // Tests for control statements.
 //===--===//
Index: test/Parser/MicrosoftExtensions.cpp
===
--- test/Parser/MicrosoftExtensions.cpp
+++ test/Parser/MicrosoftExtensions.cpp
@@ -261,9 +261,7 @@
 #define identifier_weird(x) __identifier(x
 int k = identifier_weird(if)); // expected-error {{use of undeclared identifier 'if'}}
 
-// This is a bit weird, but the alternative tokens aren't keywords, and this
-// behavior matches MSVC. FIXME: Consider supporting this anyway.
-extern int __identifier(and) r; // expected-error {{cannot convert '&&' token to an identifier}}
+extern int __identifier(and);
 
 void f() {
   __identifier(() // expected-error {{cannot convert '(' token to an identifier}}
Index: lib/Lex/Preprocessor.cpp
===
--- lib/Lex/Preprocessor.cpp
+++ lib/Lex/Preprocessor.cpp
@@ -712,14 +712,6 @@
 II.setIsFutureCompatKeyword(false);
   }
 
-  // C++ 2.11p2: If this is an alternative representation of a C++ operator,
-  // then we act as if it is the actual operator and not the textual
-  // representation of it.
-  if (II.isCPlusPlusOperatorKeyword() &&
-  !(getLangOpts().MSVCCompat &&
-getSourceManager().isInSystemHeader(Identifier.getLocation(
-Identifier.setIdentifierInfo(nullptr);
-
   // If this is an extension token, diagnose its use.
   // We avoid diagnosing tokens that originate from macro definitions.
   // FIXME: This warning is disabled in cases where it shouldn't be,
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -237,35 +237,30 @@
 PP.setCodeCompletionReached();
 PP.LexNonComment(PeekTok);
   }
-  
-  // If this token's spelling is a pp-identifier, check to see if it is
-  // 'defined' or if it is a macro.  Note that we check here because many
-  // keywords are pp-identifiers, so we can't check the kind.
-  if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {
-// Handle "defined X" and "defined(X)".
-if (II->isStr("defined"))
-  return EvaluateDefined(Result, PeekTok, DT, ValueLive, PP);
-
-// If this identifier isn't 'defined' or one of the special
-// preprocessor keywords and it wasn't macro expanded, it turns
-// into a simple 0, unless it is the C++ keyword "true", in which case it
-// turns into "1".
-if (ValueLive &&
-II->getTokenID() != tok::kw_true &&
-II->getTokenID() != tok::kw_false)
-  PP.Diag(PeekTok, diag::warn_pp_undef_identifier) << II;
-Result.Val = II->getTokenID() == tok::kw_true;
-Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
-Result.setIdentifier(II);
-Result.setRange(PeekTok.getLocation());
-DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
-   II->getTokenID() != tok::kw_false);
-PP.LexNonComment(PeekTok);
-return false;
-  }
 
   switch (PeekTok.getKind()) {
-  default:  // Non-value token.
+  default:
+// If this token's spelling is a pp-identifier, check to see if it is
+// 'defined' or if it is a macro.  Note that we check here because many
+// keywords are pp-identifiers, so we can't check the kind.
+if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {
+  // Handle "defined X" and "defined(X)".
+  if (II->isStr("defined"))
+return EvaluateDefined(Result, PeekTok, DT, ValueLive, PP);
+
+  // If this identifier isn't 'defined' or one of the special
+  // preprocessor keywords and it wasn't macro expanded, it turns
+  // into a simple 0
+  if (ValueLive)
+PP.Diag(PeekTok, diag::warn_pp_undef_identifier) << II;
+  Result.Val = 0;
+  Result.Val.setIsUnsigned(false); // "0" is signed intmax_t 0.
+  

[PATCH] D35172: Keep the IdentifierInfo in the Token for alternative operator keyword

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart marked an inline comment as done.
ogoffart added inline comments.



Comment at: lib/Lex/PPExpressions.cpp:242
   switch (PeekTok.getKind()) {
-  default:  // Non-value token.
+  default:
+// If this token's spelling is a pp-identifier, check to see if it is

rsmith wrote:
> I'm concerned that this will do the wrong thing for a keyword operator that 
> is not permitted in a pp expression, like and_eq. It seems safer to revert 
> this change and instead add a isCPlusPlusOperatorKeyword check to the "if" 
> condition above.
Well spotted.  I kept this code in the default:  so true and false are handled 
separatly, but added a condition for isCPlusPlusOperatorKeyword, and an 
additional test.


https://reviews.llvm.org/D35172



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35172: Keep the IdentifierInfo in the Token for alternative operator keyword

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart created this revision.
Herald added a subscriber: klimek.

The goal of this commit is to fix clang-format so it does not merge tokens when
using the alternative spelling keywords. (eg: "not foo" should not become 
"notfoo")

The problem is that Preprocessor::HandleIdentifier used to drop the identifier 
info
from the token for these keyword. This means the first condition of
TokenAnnotator::spaceRequiredBefore is not met. We could add explicit check for
the spelling in that condition, but I think it is better to keep the 
IdentifierInfo
and handle the operator keyword explicitly when needed. That actually leads to 
simpler
code, and probably slightly more efficient as well.

Another side effect of this change is that __identifier(and) will now work as
one would expect, removing a FIXME from the MicrosoftExtensions.cpp test


https://reviews.llvm.org/D35172

Files:
  include/clang/Basic/IdentifierTable.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPExpressions.cpp
  lib/Lex/Preprocessor.cpp
  test/Parser/MicrosoftExtensions.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -333,6 +333,16 @@
   verifyFormat("x = (a) xor (b);");
 }
 
+TEST_F(FormatTest, RecognizesUnaryOperatorKeywords) {
+  verifyFormat("x = compl(a);");
+  verifyFormat("x = not(a);");
+  verifyFormat("x = bitand(a);");
+  // Unary operator must not be merged with the next identifier
+  verifyFormat("x = compl a;");
+  verifyFormat("x = not a;");
+  verifyFormat("x = bitand a;");
+}
+
 //===--===//
 // Tests for control statements.
 //===--===//
Index: test/Parser/MicrosoftExtensions.cpp
===
--- test/Parser/MicrosoftExtensions.cpp
+++ test/Parser/MicrosoftExtensions.cpp
@@ -261,9 +261,7 @@
 #define identifier_weird(x) __identifier(x
 int k = identifier_weird(if)); // expected-error {{use of undeclared identifier 'if'}}
 
-// This is a bit weird, but the alternative tokens aren't keywords, and this
-// behavior matches MSVC. FIXME: Consider supporting this anyway.
-extern int __identifier(and) r; // expected-error {{cannot convert '&&' token to an identifier}}
+extern int __identifier(and);
 
 void f() {
   __identifier(() // expected-error {{cannot convert '(' token to an identifier}}
Index: lib/Lex/Preprocessor.cpp
===
--- lib/Lex/Preprocessor.cpp
+++ lib/Lex/Preprocessor.cpp
@@ -712,14 +712,6 @@
 II.setIsFutureCompatKeyword(false);
   }
 
-  // C++ 2.11p2: If this is an alternative representation of a C++ operator,
-  // then we act as if it is the actual operator and not the textual
-  // representation of it.
-  if (II.isCPlusPlusOperatorKeyword() &&
-  !(getLangOpts().MSVCCompat &&
-getSourceManager().isInSystemHeader(Identifier.getLocation(
-Identifier.setIdentifierInfo(nullptr);
-
   // If this is an extension token, diagnose its use.
   // We avoid diagnosing tokens that originate from macro definitions.
   // FIXME: This warning is disabled in cases where it shouldn't be,
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -237,35 +237,30 @@
 PP.setCodeCompletionReached();
 PP.LexNonComment(PeekTok);
   }
-  
-  // If this token's spelling is a pp-identifier, check to see if it is
-  // 'defined' or if it is a macro.  Note that we check here because many
-  // keywords are pp-identifiers, so we can't check the kind.
-  if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {
-// Handle "defined X" and "defined(X)".
-if (II->isStr("defined"))
-  return EvaluateDefined(Result, PeekTok, DT, ValueLive, PP);
-
-// If this identifier isn't 'defined' or one of the special
-// preprocessor keywords and it wasn't macro expanded, it turns
-// into a simple 0, unless it is the C++ keyword "true", in which case it
-// turns into "1".
-if (ValueLive &&
-II->getTokenID() != tok::kw_true &&
-II->getTokenID() != tok::kw_false)
-  PP.Diag(PeekTok, diag::warn_pp_undef_identifier) << II;
-Result.Val = II->getTokenID() == tok::kw_true;
-Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
-Result.setIdentifier(II);
-Result.setRange(PeekTok.getLocation());
-DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
-   II->getTokenID() != tok::kw_false);
-PP.LexNonComment(PeekTok);
-return false;
-  }
 
   switch (PeekTok.getKind()) {
-  default:  // Non-value token.
+  default:
+// If this token's spelling is a pp-identifier, check to see if it is
+// 

[PATCH] D35108: Fix crash parsing invalid code

[Olivier Goffart via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL307371: Fix crash parsing invalid code (authored by 
ogoffart).

Changed prior to commit:
  https://reviews.llvm.org/D35108?vs=105590=105604#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D35108

Files:
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/test/Sema/address-packed.c


Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -12097,6 +12097,8 @@
 if (ME->isArrow())
   BaseType = BaseType->getPointeeType();
 RecordDecl *RD = BaseType->getAs()->getDecl();
+if (RD->isInvalidDecl())
+  return;
 
 ValueDecl *MD = ME->getMemberDecl();
 auto *FD = dyn_cast(MD);
Index: cfe/trunk/test/Sema/address-packed.c
===
--- cfe/trunk/test/Sema/address-packed.c
+++ cfe/trunk/test/Sema/address-packed.c
@@ -329,3 +329,12 @@
   uint32_t *p32;
   p32 = [0].x; // no-warning
 }
+
+struct Invalid0 {
+  void *x;
+  struct fwd f; // expected-error {{incomplete type}} expected-note {{forward 
declaration}}
+} __attribute__((packed));
+
+void *g14(struct Invalid0 *ivl) {
+  return &(ivl->x);
+}


Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -12097,6 +12097,8 @@
 if (ME->isArrow())
   BaseType = BaseType->getPointeeType();
 RecordDecl *RD = BaseType->getAs()->getDecl();
+if (RD->isInvalidDecl())
+  return;
 
 ValueDecl *MD = ME->getMemberDecl();
 auto *FD = dyn_cast(MD);
Index: cfe/trunk/test/Sema/address-packed.c
===
--- cfe/trunk/test/Sema/address-packed.c
+++ cfe/trunk/test/Sema/address-packed.c
@@ -329,3 +329,12 @@
   uint32_t *p32;
   p32 = [0].x; // no-warning
 }
+
+struct Invalid0 {
+  void *x;
+  struct fwd f; // expected-error {{incomplete type}} expected-note {{forward declaration}}
+} __attribute__((packed));
+
+void *g14(struct Invalid0 *ivl) {
+  return &(ivl->x);
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35108: Fix crash parsing invalid code

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart created this revision.

The code in the test caused a crash with this backtrace:

RecordLayoutBuilder.cpp:2934: const clang::ASTRecordLayout 
::ASTContext::getASTRecordLayout(const clang::RecordDecl *) const: 
Assertion `!D->isInvalidDecl() && "Cannot get layout of invalid decl!"' failed.
 [...]
 #7 0x7f63963d845a __assert_fail_base (/usr/lib/libc.so.6+0x2c45a)
 #8 0x7f63963d84d2 (/usr/lib/libc.so.6+0x2c4d2)
 #9 0x7f63937a0631 clang::ASTContext::getASTRecordLayout(clang::RecordDecl 
const*) const 
/home/olivier/prog/llvm/tools/clang/lib/AST/RecordLayoutBuilder.cpp:2935:3
 #10 0x7f63937a1ad5 getFieldOffset(clang::ASTContext const&, 
clang::FieldDecl const*) 
/home/olivier/prog/llvm/tools/clang/lib/AST/RecordLayoutBuilder.cpp:3057:37
 #11 0x7f6391869f14 
clang::Sema::RefersToMemberWithReducedAlignment(clang::Expr*, 
llvm::function_ref) 
/home/olivier/prog/llvm/tools/clang/lib/Sema/SemaChecking.cpp:12139:23
 #12 0x7f639186a2f8 clang::Sema::CheckAddressOfPackedMember(clang::Expr*) 
/home/olivier/prog/llvm/tools/clang/lib/Sema/SemaChecking.cpp:12190:1
 #13 0x7f6391a7a81c 
clang::Sema::CheckAddressOfOperand(clang::ActionResult&, 
clang::SourceLocation) 
/home/olivier/prog/llvm/tools/clang/lib/Sema/SemaExpr.cpp:1:10
 #14 0x7f6391a7f5d2 
clang::Sema::CreateBuiltinUnaryOp(clang::SourceLocation, 
clang::UnaryOperatorKind, clang::Expr*) 
/home/olivier/prog/llvm/tools/clang/lib/Sema/SemaExpr.cpp:11932:18

Fixing by bailing out for invalid classes.


https://reviews.llvm.org/D35108

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/address-packed.c


Index: test/Sema/address-packed.c
===
--- test/Sema/address-packed.c
+++ test/Sema/address-packed.c
@@ -329,3 +329,14 @@
   uint32_t *p32;
   p32 = [0].x; // no-warning
 }
+
+struct Invalid0 {
+  void *x;
+  struct fwd f; // expected-error {{incomplete type}} expected-note {{forward 
declaration}}
+} __attribute__((packed));
+
+
+void *g14(struct Invalid0 *ivl)
+{
+return &(ivl->x);
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -12101,7 +12101,7 @@
 ValueDecl *MD = ME->getMemberDecl();
 auto *FD = dyn_cast(MD);
 // We do not care about non-data members.
-if (!FD || FD->isInvalidDecl())
+if (!FD || FD->isInvalidDecl() || RD->isInvalidDecl())
   return;
 
 AnyIsPacked =


Index: test/Sema/address-packed.c
===
--- test/Sema/address-packed.c
+++ test/Sema/address-packed.c
@@ -329,3 +329,14 @@
   uint32_t *p32;
   p32 = [0].x; // no-warning
 }
+
+struct Invalid0 {
+  void *x;
+  struct fwd f; // expected-error {{incomplete type}} expected-note {{forward declaration}}
+} __attribute__((packed));
+
+
+void *g14(struct Invalid0 *ivl)
+{
+return &(ivl->x);
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -12101,7 +12101,7 @@
 ValueDecl *MD = ME->getMemberDecl();
 auto *FD = dyn_cast(MD);
 // We do not care about non-data members.
-if (!FD || FD->isInvalidDecl())
+if (!FD || FD->isInvalidDecl() || RD->isInvalidDecl())
   return;
 
 AnyIsPacked =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26350: Keep invalid Switch in the AST

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

Ping


https://reviews.llvm.org/D26350



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26350: Keep invalid Switch in the AST

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

ping2


https://reviews.llvm.org/D26350



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26465: [Diag] Optimize DiagnosticIDs::getDiagnosticSeverity

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart updated this revision to Diff 82325.
ogoffart marked an inline comment as done.

https://reviews.llvm.org/D26465

Files:
  include/clang/Basic/Diagnostic.h
  include/clang/Basic/DiagnosticIDs.h
  lib/Basic/Diagnostic.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/Serialization/ASTReader.cpp

Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -5328,6 +5328,10 @@
 diag::Severity Map = (diag::Severity)F.PragmaDiagMappings[Idx++];
 DiagnosticMapping Mapping = Diag.makeUserMapping(Map, Loc);
 Diag.GetCurDiagState()->setMapping(DiagID, Mapping);
+
+// We are overriding a default behavior
+Diag.DiagStates.front().getOrAddMapping(DiagID).setMightBeOverriden(
+true);
   }
 }
   }
Index: lib/Basic/DiagnosticIDs.cpp
===
--- lib/Basic/DiagnosticIDs.cpp
+++ lib/Basic/DiagnosticIDs.cpp
@@ -396,6 +396,23 @@
   return toLevel(getDiagnosticSeverity(DiagID, Loc, Diag));
 }
 
+DiagnosticMapping &
+DiagnosticIDs::getDiagnosticMapping(unsigned DiagID, SourceLocation Loc,
+const DiagnosticsEngine ) const {
+  // Calling GetDiagStatePointForLoc might be costly as it needs to create
+  // FullSourceLoc and compare them to the ones in the states. So first query
+  // the base state and check if it might be overridden.
+  DiagnosticsEngine::DiagState  = Diag.DiagStates.front();
+  assert( == Diag.DiagStatePoints.front().State);
+  DiagnosticMapping  = BaseState.getOrAddMapping((diag::kind)DiagID);
+  if (!Mapping.mightBeOverriden())
+return Mapping;
+
+  DiagnosticsEngine::DiagStatePointsTy::iterator Pos =
+  Diag.GetDiagStatePointForLoc(Loc);
+  return Pos->State->getOrAddMapping((diag::kind)DiagID);
+}
+
 /// \brief Based on the way the client configured the Diagnostic
 /// object, classify the specified diagnostic ID into a Level, consumable by
 /// the DiagnosticClient.
@@ -406,17 +423,11 @@
 DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
  const DiagnosticsEngine ) const {
   assert(getBuiltinDiagClass(DiagID) != CLASS_NOTE);
-
   // Specific non-error diagnostics may be mapped to various levels from ignored
   // to error.  Errors can only be mapped to fatal.
   diag::Severity Result = diag::Severity::Fatal;
 
-  DiagnosticsEngine::DiagStatePointsTy::iterator
-Pos = Diag.GetDiagStatePointForLoc(Loc);
-  DiagnosticsEngine::DiagState *State = Pos->State;
-
-  // Get the mapping information, or compute it lazily.
-  DiagnosticMapping  = State->getOrAddMapping((diag::kind)DiagID);
+  DiagnosticMapping  = getDiagnosticMapping(DiagID, Loc, Diag);
 
   // TODO: Can a null severity really get here?
   if (Mapping.getSeverity() != diag::Severity())
Index: lib/Basic/Diagnostic.cpp
===
--- lib/Basic/Diagnostic.cpp
+++ lib/Basic/Diagnostic.cpp
@@ -201,6 +201,11 @@
   }
   DiagnosticMapping Mapping = makeUserMapping(Map, L);
 
+  if (Loc.isValid()) {
+// We are potentially overriding a default behavior
+DiagStates.front().getOrAddMapping(Diag).setMightBeOverriden(true);
+  }
+
   // Common case; setting all the diagnostics of a group in one place.
   if (Loc.isInvalid() || Loc == LastStateChangePos) {
 GetCurDiagState()->setMapping(Diag, Mapping);
Index: include/clang/Basic/DiagnosticIDs.h
===
--- include/clang/Basic/DiagnosticIDs.h
+++ include/clang/Basic/DiagnosticIDs.h
@@ -84,6 +84,7 @@
   unsigned IsPragma : 1;
   unsigned HasNoWarningAsError : 1;
   unsigned HasNoErrorAsFatal : 1;
+  unsigned MightBeOverriden : 1;
 
 public:
   static DiagnosticMapping Make(diag::Severity Severity, bool IsUser,
@@ -94,6 +95,8 @@
 Result.IsPragma = IsPragma;
 Result.HasNoWarningAsError = 0;
 Result.HasNoErrorAsFatal = 0;
+Result.MightBeOverriden = 0;
+
 return Result;
   }
 
@@ -108,6 +111,11 @@
 
   bool hasNoErrorAsFatal() const { return HasNoErrorAsFatal; }
   void setNoErrorAsFatal(bool Value) { HasNoErrorAsFatal = Value; }
+
+  /// Only makes sense for the base state: specify if the diagnostic might be
+  /// changed for other source location
+  bool mightBeOverriden() const { return MightBeOverriden; }
+  void setMightBeOverriden(bool Value) { MightBeOverriden = Value; }
 };
 
 /// \brief Used for handling and querying diagnostic IDs.
@@ -261,6 +269,10 @@
   getDiagnosticLevel(unsigned DiagID, SourceLocation Loc,
  const DiagnosticsEngine ) const LLVM_READONLY;
 
+  DiagnosticMapping &
+  getDiagnosticMapping(unsigned DiagID, SourceLocation Loc,
+   const DiagnosticsEngine ) const LLVM_READONLY;
+
   diag::Severity
   getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,

[PATCH] D26465: [Diag] Optimize DiagnosticIDs::getDiagnosticSeverity

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart marked an inline comment as done.
ogoffart added a comment.

In https://reviews.llvm.org/D26465#607860, @arphaman wrote:

> What did you test the parsing on? Will this patch get similar improvements 
> for code that compiles without errors and warnings?


It was benchamerked with https://github.com/woboq/woboq_codebrowser generating 
itself.  
The code does not contain warnings.  (unless you really consider the 
warn_cxx98_compat_* to be warnings)




Comment at: lib/Basic/DiagnosticIDs.cpp:423
+Mapping = >State->getOrAddMapping((diag::kind)DiagID);
+  }
 

arphaman wrote:
> I think it would be better if you wrap this piece of code in a static 
> function that returns `DiagnosticMapping &`, as it should allow you to get 
> rid of all these `.` to `->` changes below.
The problem is that most things are private in DiagnosticsEngine, so i made it 
a privte member of DiagnosticIds (which is a friend)


https://reviews.llvm.org/D26465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26465: [Diag] Optimize DiagnosticIDs::getDiagnosticSeverity

[Olivier Goffart via Phabricator via cfe-commits]

ogoffart added a comment.

Ping 2


https://reviews.llvm.org/D26465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits