[PATCH] D154861: [clang][AST] Propagate the contains-errors bit to DeclRefExpr from VarDecl's initializer.

2023-07-11 Thread Haojian Wu via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9ca395b5ade1: [clang][AST] Propagate the contains-errors bit 
to DeclRefExpr from VarDecls… (authored by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D154861?vs=538929=538931#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154861

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ComputeDependence.cpp
  clang/test/AST/ast-dump-recovery.c
  clang/test/SemaCXX/cxx11-crashes.cpp


Index: clang/test/SemaCXX/cxx11-crashes.cpp
===
--- clang/test/SemaCXX/cxx11-crashes.cpp
+++ clang/test/SemaCXX/cxx11-crashes.cpp
@@ -65,7 +65,7 @@
   struct S {}; // expected-note 3{{candidate}}
   void f() {
 S s(1, 2, 3); // expected-error {{no matching}}
-for (auto x : s) { // expected-error {{invalid range expression of}}
+for (auto x : s) {
   // We used to attempt to evaluate the initializer of this variable,
   // and crash because it has an undeduced type.
   const int (x);
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -126,3 +126,25 @@
   // CHECK-NEXT:   `-RecoveryExpr {{.*}} ''
   sizeof array / sizeof foo(undef);
 }
+
+// No crash on DeclRefExpr that refers to ValueDecl with invalid initializers.
+void test7() {
+  int b[] = {""()};
+
+  // CHECK:  CStyleCastExpr {{.*}} 'unsigned int' contains-errors
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'int[]' contains-errors
+  (unsigned) b; // GH50236
+
+  // CHECK:  BinaryOperator {{.*}} '' contains-errors '+'
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int[]' contains-errors
+  // CHECK-NEXT: `-IntegerLiteral {{.*}}
+  b + 1; // GH50243
+
+  // CHECK:  CallExpr {{.*}} '' contains-errors
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int ()' Function
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'int[]' contains-errors
+  return c(b); // GH48636
+}
+int test8_GH50320_b[] = {""()};
+// CHECK: ArraySubscriptExpr {{.*}} 'int' contains-errors lvalue
+int test8 = test_8GH50320_b[0];
Index: clang/lib/AST/ComputeDependence.cpp
===
--- clang/lib/AST/ComputeDependence.cpp
+++ clang/lib/AST/ComputeDependence.cpp
@@ -489,7 +489,7 @@
   // more bullets here that we handle by treating the declaration as having a
   // dependent type if they involve a placeholder type that can't be deduced.]
   if (Type->isDependentType())
-return Deps | ExprDependence::TypeValueInstantiation;
+Deps |= ExprDependence::TypeValueInstantiation;
   else if (Type->isInstantiationDependentType())
 Deps |= ExprDependence::Instantiation;
 
@@ -525,13 +525,13 @@
   //   - it names a potentially-constant variable that is initialized with an
   // expression that is value-dependent
   if (const auto *Var = dyn_cast(Decl)) {
-if (Var->mightBeUsableInConstantExpressions(Ctx)) {
-  if (const Expr *Init = Var->getAnyInitializer()) {
-if (Init->isValueDependent())
-  Deps |= ExprDependence::ValueInstantiation;
-if (Init->containsErrors())
-  Deps |= ExprDependence::Error;
-  }
+if (const Expr *Init = Var->getAnyInitializer()) {
+  if (Init->containsErrors())
+Deps |= ExprDependence::Error;
+
+  if (Var->mightBeUsableInConstantExpressions(Ctx) &&
+  Init->isValueDependent())
+Deps |= ExprDependence::ValueInstantiation;
 }
 
 // - it names a static data member that is a dependent member of the
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -576,6 +576,12 @@
 - Fixed false positive error diagnostic when pack expansion appears in template
   parameters of a member expression.
   (`#48731 `_)
+- Fix the contains-errors bit not being set for DeclRefExpr that refers to a
+  VarDecl with invalid initializer. This fixes:
+  (`#50236 `_),
+  (`#50243 `_),
+  (`#48636 `_),
+  (`#50320 `_).
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/cxx11-crashes.cpp
===
--- clang/test/SemaCXX/cxx11-crashes.cpp
+++ clang/test/SemaCXX/cxx11-crashes.cpp
@@ -65,7 +65,7 @@
   struct S {}; // expected-note 3{{candidate}}
   void f() {
 S s(1, 2, 3); // expected-error {{no matching}}
-for (auto x : s) { // expected-error

[PATCH] D154861: [clang][AST] Propagate the contains-errors bit to DeclRefExpr from VarDecl's initializer.

2023-07-11 Thread Haojian Wu via Phabricator via cfe-commits 
hokein updated this revision to Diff 538929.
hokein marked 2 inline comments as done.
hokein added a comment.

address comments and add release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154861

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/ComputeDependence.cpp
  clang/test/AST/ast-dump-recovery.c
  clang/test/SemaCXX/cxx11-crashes.cpp


Index: clang/test/SemaCXX/cxx11-crashes.cpp
===
--- clang/test/SemaCXX/cxx11-crashes.cpp
+++ clang/test/SemaCXX/cxx11-crashes.cpp
@@ -65,7 +65,7 @@
   struct S {}; // expected-note 3{{candidate}}
   void f() {
 S s(1, 2, 3); // expected-error {{no matching}}
-for (auto x : s) { // expected-error {{invalid range expression of}}
+for (auto x : s) {
   // We used to attempt to evaluate the initializer of this variable,
   // and crash because it has an undeduced type.
   const int (x);
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -126,3 +126,25 @@
   // CHECK-NEXT:   `-RecoveryExpr {{.*}} ''
   sizeof array / sizeof foo(undef);
 }
+
+// No crash on DeclRefExpr that refers to ValueDecl with invalid initializers.
+void test7() {
+  int b[] = {""()};
+
+  // CHECK:  CStyleCastExpr {{.*}} 'unsigned int' contains-errors
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'int[]' contains-errors
+  (unsigned) b; // GH50236
+
+  // CHECK:  BinaryOperator {{.*}} '' contains-errors '+'
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int[]' contains-errors
+  // CHECK-NEXT: `-IntegerLiteral {{.*}}
+  b + 1; // GH50243
+
+  // CHECK:  CallExpr {{.*}} '' contains-errors
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int ()' Function
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'int[]' contains-errors
+  return c(b); // GH48636
+}
+int test8_GH50320_b[] = {""()};
+// CHECK: ArraySubscriptExpr {{.*}} 'int' contains-errors lvalue
+int test8 = test_8GH50320_b[0];
Index: clang/lib/AST/ComputeDependence.cpp
===
--- clang/lib/AST/ComputeDependence.cpp
+++ clang/lib/AST/ComputeDependence.cpp
@@ -489,7 +489,7 @@
   // more bullets here that we handle by treating the declaration as having a
   // dependent type if they involve a placeholder type that can't be deduced.]
   if (Type->isDependentType())
-return Deps | ExprDependence::TypeValueInstantiation;
+Deps |= ExprDependence::TypeValueInstantiation;
   else if (Type->isInstantiationDependentType())
 Deps |= ExprDependence::Instantiation;
 
@@ -525,13 +525,13 @@
   //   - it names a potentially-constant variable that is initialized with an
   // expression that is value-dependent
   if (const auto *Var = dyn_cast(Decl)) {
-if (Var->mightBeUsableInConstantExpressions(Ctx)) {
-  if (const Expr *Init = Var->getAnyInitializer()) {
-if (Init->isValueDependent())
-  Deps |= ExprDependence::ValueInstantiation;
-if (Init->containsErrors())
-  Deps |= ExprDependence::Error;
-  }
+if (const Expr *Init = Var->getAnyInitializer()) {
+  if (Init->containsErrors())
+Deps |= ExprDependence::Error;
+
+  if (Var->mightBeUsableInConstantExpressions(Ctx) &&
+  Init->isValueDependent())
+Deps |= ExprDependence::ValueInstantiation;
 }
 
 // - it names a static data member that is a dependent member of the
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -573,6 +573,12 @@
 - Stop evaluating a constant expression if the condition expression which in
   switch statement contains errors.
   (`#63453 _`)
+- Fix the contains-errors bit not being set for DeclRefExpr that refers to a
+  VarDecl with invalid initializer. This fixes:
+  (`#50236 `_),
+  (`#50243 `_),
+  (`#48636 `_),
+  (`#50320 `_).
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/cxx11-crashes.cpp
===
--- clang/test/SemaCXX/cxx11-crashes.cpp
+++ clang/test/SemaCXX/cxx11-crashes.cpp
@@ -65,7 +65,7 @@
   struct S {}; // expected-note 3{{candidate}}
   void f() {
 S s(1, 2, 3); // expected-error {{no matching}}
-for (auto x : s) { // expected-error {{invalid range expression of}}
+for (auto x : s) {
   // We used to attempt to evaluate the initializer of this variable,
   // and crash because it has an undeduced type.
   const int

[PATCH] D154861: [clang][AST] Propagate the contains-errors bit to DeclRefExpr from VarDecl's initializer.

2023-07-10 Thread Shafik Yaghmour via Phabricator via cfe-commits 
shafik added inline comments.



Comment at: clang/lib/AST/ComputeDependence.cpp:461
 /// based on the declaration being referenced.
 ExprDependence clang::computeDependence(DeclRefExpr *E, const ASTContext ) 
{
   auto Deps = ExprDependence::None;

`computeDependence` does not feel particularly well organized, it is not clear 
to me how correct it is :-(



Comment at: clang/lib/AST/ComputeDependence.cpp:532
+
 if (Var->mightBeUsableInConstantExpressions(Ctx)) {
   if (const Expr *Init = Var->getAnyInitializer()) {

It feels like we could fold this under the `if (const Expr *Init = 
Var->getAnyInitializer(); Init ` above and not duplicate code. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154861

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

[PATCH] D154861: [clang][AST] Propagate the contains-errors bit to DeclRefExpr from VarDecl's initializer.

2023-07-10 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, but please be sure to add a release note (and the newline at the end of 
the test file). Thank you for the fix!




Comment at: clang/test/AST/ast-dump-recovery.c:151
+int test8 = test_8GH50320_b[0];
\ No newline at end of file


Please add a newline to the end of the file. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154861

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

[PATCH] D154861: [clang][AST] Propagate the contains-errors bit to DeclRefExpr from VarDecl's initializer.

2023-07-10 Thread Haojian Wu via Phabricator via cfe-commits 
hokein created this revision.
hokein added reviewers: aaron.ballman, shafik, sammccall.
Herald added a project: All.
hokein requested review of this revision.
Herald added a project: clang.

Similar to the https://reviews.llvm.org/D86048 (it only sets the bit for C++
code), we propagate the contains-errors bit for C-code path.

Fixes https://github.com/llvm/llvm-project/issues/50236
Fixes https://github.com/llvm/llvm-project/issues/50243
Fixes https://github.com/llvm/llvm-project/issues/48636
Fixes https://github.com/llvm/llvm-project/issues/50320


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154861

Files:
  clang/lib/AST/ComputeDependence.cpp
  clang/test/AST/ast-dump-recovery.c
  clang/test/SemaCXX/cxx11-crashes.cpp


Index: clang/test/SemaCXX/cxx11-crashes.cpp
===
--- clang/test/SemaCXX/cxx11-crashes.cpp
+++ clang/test/SemaCXX/cxx11-crashes.cpp
@@ -65,7 +65,7 @@
   struct S {}; // expected-note 3{{candidate}}
   void f() {
 S s(1, 2, 3); // expected-error {{no matching}}
-for (auto x : s) { // expected-error {{invalid range expression of}}
+for (auto x : s) {
   // We used to attempt to evaluate the initializer of this variable,
   // and crash because it has an undeduced type.
   const int (x);
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -126,3 +126,25 @@
   // CHECK-NEXT:   `-RecoveryExpr {{.*}} ''
   sizeof array / sizeof foo(undef);
 }
+
+// No crash on DeclRefExpr that refers to ValueDecl with invalid initializers.
+void test7() {
+  int b[] = {""()};
+
+  // CHECK:  CStyleCastExpr {{.*}} 'unsigned int' contains-errors
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'int[]' contains-errors
+  (unsigned) b; // GH50236
+
+  // CHECK:  BinaryOperator {{.*}} '' contains-errors '+'
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int[]' contains-errors
+  // CHECK-NEXT: `-IntegerLiteral {{.*}}
+  b + 1; // GH50243
+
+  // CHECK:  CallExpr {{.*}} '' contains-errors
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int ()' Function
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'int[]' contains-errors
+  return c(b); // GH48636
+}
+int test8_GH50320_b[] = {""()};
+// CHECK: ArraySubscriptExpr {{.*}} 'int' contains-errors lvalue
+int test8 = test_8GH50320_b[0];
\ No newline at end of file
Index: clang/lib/AST/ComputeDependence.cpp
===
--- clang/lib/AST/ComputeDependence.cpp
+++ clang/lib/AST/ComputeDependence.cpp
@@ -489,7 +489,7 @@
   // more bullets here that we handle by treating the declaration as having a
   // dependent type if they involve a placeholder type that can't be deduced.]
   if (Type->isDependentType())
-return Deps | ExprDependence::TypeValueInstantiation;
+Deps |= ExprDependence::TypeValueInstantiation;
   else if (Type->isInstantiationDependentType())
 Deps |= ExprDependence::Instantiation;
 
@@ -525,12 +525,14 @@
   //   - it names a potentially-constant variable that is initialized with an
   // expression that is value-dependent
   if (const auto *Var = dyn_cast(Decl)) {
+if (const Expr *Init = Var->getAnyInitializer();
+Init && Init->containsErrors())
+  Deps |= ExprDependence::Error;
+
 if (Var->mightBeUsableInConstantExpressions(Ctx)) {
   if (const Expr *Init = Var->getAnyInitializer()) {
 if (Init->isValueDependent())
   Deps |= ExprDependence::ValueInstantiation;
-if (Init->containsErrors())
-  Deps |= ExprDependence::Error;
   }
 }
 


Index: clang/test/SemaCXX/cxx11-crashes.cpp
===
--- clang/test/SemaCXX/cxx11-crashes.cpp
+++ clang/test/SemaCXX/cxx11-crashes.cpp
@@ -65,7 +65,7 @@
   struct S {}; // expected-note 3{{candidate}}
   void f() {
 S s(1, 2, 3); // expected-error {{no matching}}
-for (auto x : s) { // expected-error {{invalid range expression of}}
+for (auto x : s) {
   // We used to attempt to evaluate the initializer of this variable,
   // and crash because it has an undeduced type.
   const int (x);
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -126,3 +126,25 @@
   // CHECK-NEXT:   `-RecoveryExpr {{.*}} ''
   sizeof array / sizeof foo(undef);
 }
+
+// No crash on DeclRefExpr that refers to ValueDecl with invalid initializers.
+void test7() {
+  int b[] = {""()};
+
+  // CHECK:  CStyleCastExpr {{.*}} 'unsigned int' contains-errors
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'int[]' contains-errors
+  (unsigned) b; // GH50236
+
+  // CHECK:  BinaryOperator {{.*}} '' contains-errors '+'
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int[]' contains-errors
+  //