[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-04-05 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa5e1a93ea10f: [clang] Fix crash when handling nested 
immediate invocations (authored by Fznamznon).

Changed prior to commit:
  https://reviews.llvm.org/D146234?vs=509039=511027#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -1065,3 +1065,41 @@
 static_assert(B == 0);
 
 }
+
+namespace GH58207 {
+struct tester {
+consteval tester(const char* name) noexcept { }
+};
+consteval const char* make_name(const char* name) { return name;}
+consteval const char* pad(int P) { return "thestring"; }
+
+int bad = 10; // expected-note 6{{declared here}}
+
+tester glob1(make_name("glob1"));
+tester glob2(make_name("glob2"));
+constexpr tester cglob(make_name("cglob"));
+tester paddedglob(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+constexpr tester glob3 = { make_name("glob3") };
+constexpr tester glob4 = { make_name(pad(bad)) }; // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+  // expected-error {{constexpr variable 'glob4' must be initialized by a constant expression}} \
+  // expected-note 2{{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+auto V = make_name(pad(3));
+auto V1 = make_name(pad(bad)); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+   // expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+
+void foo() {
+  static tester loc1(make_name("loc1"));
+  static constexpr tester loc2(make_name("loc2"));
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+
+void bar() {
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -17860,6 +17860,7 @@
   bool Result = CE->EvaluateAsConstantExpr(
   Eval, SemaRef.getASTContext(), ConstantExprKind::ImmediateInvocation);
   if (!Result || !Notes.empty()) {
+SemaRef.FailedImmediateInvocations.insert(CE);
 Expr *InnerExpr = CE->getSubExpr()->IgnoreImplicit();
 if (auto *FunctionalCast = dyn_cast(InnerExpr))
   InnerExpr = FunctionalCast->getSubExpr();
@@ -17904,10 +17905,16 @@
  [E](Sema::ImmediateInvocationCandidate Elem) {
return Elem.getPointer() == E;
  });
-  assert(It != IISet.rend() &&
- "ConstantExpr marked IsImmediateInvocation should "
- "be present");
-  It->setInt(1); // Mark as deleted
+  // It is possible that some subexpression of the current immediate
+  // invocation was handled from another expression evaluation context. Do
+  // not handle the current immediate invocation if some of its
+  // subexpressions failed before.
+  if (It == IISet.rend()) {
+if (SemaRef.FailedImmediateInvocations.contains(E))
+  CurrentII->setInt(1);
+  } else {
+It->setInt(1); // Mark as deleted
+  }
 }
 ExprResult TransformConstantExpr(ConstantExpr *E) {
   if (!E->isImmediateInvocation())
@@ -17980,10 +17987,13 @@
   SemaRef.RebuildingImmediateInvocation)
 return;
 
-  /// When we have more then 1 ImmediateInvocationCandidates we need to check
-  /// for nested ImmediateInvocationCandidates. when we have only 1 we only
-  /// need to remove ReferenceToConsteval in the immediate invocation.
-  if (Rec.ImmediateInvocationCandidates.size() > 1) {
+  /// When we have more than 1 ImmediateInvocationCandidates or previously
+  /// failed immediate invocations, we need to check for nested

[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-04-03 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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


[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-04-03 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added a comment.

@aaron.ballman , @cor3ntin are you ok with the patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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


[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-30 Thread Shafik Yaghmour via Phabricator via cfe-commits
shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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


[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-29 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added a comment.

Ping @shafik ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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


[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-28 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon updated this revision to Diff 509039.
Fznamznon added a comment.

Fix formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -1065,3 +1065,41 @@
 static_assert(B == 0);
 
 }
+
+namespace GH58207 {
+struct tester {
+consteval tester(const char* name) noexcept { }
+};
+consteval const char* make_name(const char* name) { return name;}
+consteval const char* pad(int P) { return "thestring"; }
+
+int bad = 10; // expected-note 6{{declared here}}
+
+tester glob1(make_name("glob1"));
+tester glob2(make_name("glob2"));
+constexpr tester cglob(make_name("cglob"));
+tester paddedglob(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+constexpr tester glob3 = { make_name("glob3") };
+constexpr tester glob4 = { make_name(pad(bad)) }; // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+  // expected-error {{constexpr variable 'glob4' must be initialized by a constant expression}} \
+  // expected-note 2{{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+auto V = make_name(pad(3));
+auto V1 = make_name(pad(bad)); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+   // expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+
+void foo() {
+  static tester loc1(make_name("loc1"));
+  static constexpr tester loc2(make_name("loc2"));
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+
+void bar() {
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -17847,6 +17847,7 @@
   bool Result = CE->EvaluateAsConstantExpr(
   Eval, SemaRef.getASTContext(), ConstantExprKind::ImmediateInvocation);
   if (!Result || !Notes.empty()) {
+SemaRef.FailedImmediateInvocations.insert(CE);
 Expr *InnerExpr = CE->getSubExpr()->IgnoreImplicit();
 if (auto *FunctionalCast = dyn_cast(InnerExpr))
   InnerExpr = FunctionalCast->getSubExpr();
@@ -17891,10 +17892,16 @@
  [E](Sema::ImmediateInvocationCandidate Elem) {
return Elem.getPointer() == E;
  });
-  assert(It != IISet.rend() &&
- "ConstantExpr marked IsImmediateInvocation should "
- "be present");
-  It->setInt(1); // Mark as deleted
+  // It is possible that some subexpression of the current immediate
+  // invocation was handled from another expression evaluation context. Do
+  // not handle the current immediate invocation if some of its
+  // subexpressions failed before.
+  if (It == IISet.rend()) {
+if (SemaRef.FailedImmediateInvocations.contains(E))
+  CurrentII->setInt(1);
+  } else {
+It->setInt(1); // Mark as deleted
+  }
 }
 ExprResult TransformConstantExpr(ConstantExpr *E) {
   if (!E->isImmediateInvocation())
@@ -17967,10 +17974,13 @@
   SemaRef.RebuildingImmediateInvocation)
 return;
 
-  /// When we have more then 1 ImmediateInvocationCandidates we need to check
-  /// for nested ImmediateInvocationCandidates. when we have only 1 we only
-  /// need to remove ReferenceToConsteval in the immediate invocation.
-  if (Rec.ImmediateInvocationCandidates.size() > 1) {
+  /// When we have more than 1 ImmediateInvocationCandidates or previously
+  /// failed immediate invocations, we need to check for nested
+  /// ImmediateInvocationCandidates in order to avoid duplicate diagnostics.
+  /// Otherwise we only need to remove ReferenceToConsteval in the immediate
+  /// invocation.
+  if 

[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-28 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon updated this revision to Diff 508974.
Fznamznon added a comment.

Rebase, add release note, fix grammar


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -1065,3 +1065,41 @@
 static_assert(B == 0);
 
 }
+
+namespace GH58207 {
+struct tester {
+consteval tester(const char* name) noexcept { }
+};
+consteval const char* make_name(const char* name) { return name;}
+consteval const char* pad(int P) { return "thestring"; }
+
+int bad = 10; // expected-note 6{{declared here}}
+
+tester glob1(make_name("glob1"));
+tester glob2(make_name("glob2"));
+constexpr tester cglob(make_name("cglob"));
+tester paddedglob(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+constexpr tester glob3 = { make_name("glob3") };
+constexpr tester glob4 = { make_name(pad(bad)) }; // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+  // expected-error {{constexpr variable 'glob4' must be initialized by a constant expression}} \
+  // expected-note 2{{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+auto V = make_name(pad(3));
+auto V1 = make_name(pad(bad)); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+   // expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+
+void foo() {
+  static tester loc1(make_name("loc1"));
+  static constexpr tester loc2(make_name("loc2"));
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+
+void bar() {
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -17847,6 +17847,7 @@
   bool Result = CE->EvaluateAsConstantExpr(
   Eval, SemaRef.getASTContext(), ConstantExprKind::ImmediateInvocation);
   if (!Result || !Notes.empty()) {
+SemaRef.FailedImmediateInvocations.insert(CE);
 Expr *InnerExpr = CE->getSubExpr()->IgnoreImplicit();
 if (auto *FunctionalCast = dyn_cast(InnerExpr))
   InnerExpr = FunctionalCast->getSubExpr();
@@ -17891,10 +17892,16 @@
  [E](Sema::ImmediateInvocationCandidate Elem) {
return Elem.getPointer() == E;
  });
-  assert(It != IISet.rend() &&
- "ConstantExpr marked IsImmediateInvocation should "
- "be present");
-  It->setInt(1); // Mark as deleted
+  // It is possible that some subexpression of the current immediate
+  // invocation was handled from another expression evaluation context. Do
+  // not handle the current immediate invocation if some of its
+  // subexpressions failed before.
+  if (It == IISet.rend()) {
+if (SemaRef.FailedImmediateInvocations.contains(E))
+  CurrentII->setInt(1);
+  } else {
+It->setInt(1); // Mark as deleted
+  }
 }
 ExprResult TransformConstantExpr(ConstantExpr *E) {
   if (!E->isImmediateInvocation())
@@ -17967,10 +17974,13 @@
   SemaRef.RebuildingImmediateInvocation)
 return;
 
-  /// When we have more then 1 ImmediateInvocationCandidates we need to check
-  /// for nested ImmediateInvocationCandidates. when we have only 1 we only
-  /// need to remove ReferenceToConsteval in the immediate invocation.
-  if (Rec.ImmediateInvocationCandidates.size() > 1) {
+  /// When we have more than 1 ImmediateInvocationCandidates or previously
+  /// failed immediate invocations, we need to check for nested
+  /// ImmediateInvocationCandidates in order to avoid duplicate diagnostics.
+  /// Otherwise we only need to remove ReferenceToConsteval in the immediate
+  

[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment.

I think this LGTM, but I'm holding off on signing off until @shafik is 
satisfied with the part he was asking about. You should add a release note for 
the fix, too.




Comment at: clang/lib/Sema/SemaExpr.cpp:17891-17894
+  // It is possible that some subexpression of current immediate invocation
+  // was handled from another expression evaluation context. Do not handle
+  // current immediate invocation if some of its subexpressions failed
+  // before.

Minor grammar nits.



Comment at: clang/lib/Sema/SemaExpr.cpp:17973
 
-  /// When we have more then 1 ImmediateInvocationCandidates we need to check
-  /// for nested ImmediateInvocationCandidates. when we have only 1 we only
-  /// need to remove ReferenceToConsteval in the immediate invocation.
-  if (Rec.ImmediateInvocationCandidates.size() > 1) {
+  /// When we have more then 1 ImmediateInvocationCandidates or previously
+  /// failed immediate invocations, we need to check for nested





Comment at: clang/lib/Sema/SemaExpr.cpp:17979
+  if (Rec.ImmediateInvocationCandidates.size() > 1 ||
+  SemaRef.FailedImmediateInvocations.size()) {
 

Fznamznon wrote:
> cor3ntin wrote:
> > Shouln't we clear `FailedImmediateInvocations` at the end of a full 
> > expression to avoid going there if there was an error in another expression
> The idea sounds reasonable, but I'm not sure it can work with current 
> algorithm of handling immediate invocations. Immediate invocations are 
> handled when expression evaluation context is popped, so we need to remember 
> previously failed immediate invocations until then, even though they could 
> happen in two different full expressions. 
> 
> ```
> consteval foo() { ... }
> void bar() {
>   int a = foo(/* there might be a failed immediate invocation attached to 
> initializer context */); // this is a full-expression
>   int b = foo(); // this is another full-expression
> } // This is where immediate invocations that appear in function context are 
> processed
> ```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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


[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-23 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:17979
+  if (Rec.ImmediateInvocationCandidates.size() > 1 ||
+  SemaRef.FailedImmediateInvocations.size()) {
 

cor3ntin wrote:
> Shouln't we clear `FailedImmediateInvocations` at the end of a full 
> expression to avoid going there if there was an error in another expression
The idea sounds reasonable, but I'm not sure it can work with current algorithm 
of handling immediate invocations. Immediate invocations are handled when 
expression evaluation context is popped, so we need to remember previously 
failed immediate invocations until then, even though they could happen in two 
different full expressions. 

```
consteval foo() { ... }
void bar() {
  int a = foo(/* there might be a failed immediate invocation attached to 
initializer context */); // this is a full-expression
  int b = foo(); // this is another full-expression
} // This is where immediate invocations that appear in function context are 
processed
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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


[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-23 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added a comment.

Sorry for the late review.
I think this makes sense generally, i just have one question




Comment at: clang/lib/Sema/SemaExpr.cpp:17979
+  if (Rec.ImmediateInvocationCandidates.size() > 1 ||
+  SemaRef.FailedImmediateInvocations.size()) {
 

Shouln't we clear `FailedImmediateInvocations` at the end of a full expression 
to avoid going there if there was an error in another expression


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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


[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-23 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added a comment.

Ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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


[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-16 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:17897
+if (SemaRef.FailedImmediateInvocations.contains(E))
+  CurrentII->setInt(1);
+  } else {

shafik wrote:
> It is not obvious to me why this is correct, can you please explain.
The visitor which this function is a part of traverses all subexpressions of a 
given ImmediateInvocationCandidate, it is saved to `CurrentII` member. Each 
ImmediateIvocationCandidate is a pair of integer and a pointer to ConstantExpr. 
The non-zero integer indicates that we should not try to evaluate particular 
immediate invocation. So, this patch, once found out that current expression 
contains a subexpression from another evaluation context whose evaluation 
failed before, marks that we shouldn't evaluate it to avoid double diagnosing. 
Otherwise, if some subexpression was found in the same context that current 
ImmediateInvocationCandidate belongs to, mark that we should not evaluate it 
instead, again to avoid double diagnosing. Hope that helps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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


[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-16 Thread Shafik Yaghmour via Phabricator via cfe-commits
shafik added a comment.

Thank you for the fix.




Comment at: clang/lib/Sema/SemaExpr.cpp:17897
+if (SemaRef.FailedImmediateInvocations.contains(E))
+  CurrentII->setInt(1);
+  } else {

It is not obvious to me why this is correct, can you please explain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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


[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

2023-03-16 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon created this revision.
Herald added a project: All.
Fznamznon requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Before this patch it was expected that if there was several immediate
invocations they all belong to the same expression evaluation context.
During parsing of non local variable initializer a new evaluation context is
pushed, so code like this

  namespace scope {
  struct channel {
  consteval channel(const char* name) noexcept { }
  };
  consteval const char* make_channel_name(const char* name) { return name;}
  
  channel rsx_log(make_channel_name("rsx_log"));
  }

produced a nested immediate invocation whose subexpressions are attached
to different expression evaluation contexts. The constructor call
belongs to TU context and `make_channel_name` call to context of
variable initializer.

This patch removes this assumption and adds tracking of previously
failed immediate invocations, so it is possible when handling an
immediate invocation th check that its subexpressions from possibly another
evaluation context contains errors and not produce duplicate
diagnostics.

Fixes https://github.com/llvm/llvm-project/issues/58207


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146234

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -1050,3 +1050,41 @@
 
 }
 }
+
+namespace GH58207 {
+struct tester {
+consteval tester(const char* name) noexcept { }
+};
+consteval const char* make_name(const char* name) { return name;}
+consteval const char* pad(int P) { return "thestring"; }
+
+int bad = 10; // expected-note 6{{declared here}}
+
+tester glob1(make_name("glob1"));
+tester glob2(make_name("glob2"));
+constexpr tester cglob(make_name("cglob"));
+tester paddedglob(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+constexpr tester glob3 = { make_name("glob3") };
+constexpr tester glob4 = { make_name(pad(bad)) }; // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+  // expected-error {{constexpr variable 'glob4' must be initialized by a constant expression}} \
+  // expected-note 2{{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+auto V = make_name(pad(3));
+auto V1 = make_name(pad(bad)); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+   // expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+
+void foo() {
+  static tester loc1(make_name("loc1"));
+  static constexpr tester loc2(make_name("loc2"));
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+
+void bar() {
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -17843,6 +17843,7 @@
   bool Result = CE->EvaluateAsConstantExpr(
   Eval, SemaRef.getASTContext(), ConstantExprKind::ImmediateInvocation);
   if (!Result || !Notes.empty()) {
+SemaRef.FailedImmediateInvocations.insert(CE);
 Expr *InnerExpr = CE->getSubExpr()->IgnoreImplicit();
 if (auto *FunctionalCast = dyn_cast(InnerExpr))
   InnerExpr = FunctionalCast->getSubExpr();
@@ -17887,10 +17888,16 @@
  [E](Sema::ImmediateInvocationCandidate Elem) {
return Elem.getPointer() == E;
  });
-  assert(It != IISet.rend() &&
- "ConstantExpr marked IsImmediateInvocation should "
- "be present");
-  It->setInt(1); // Mark as deleted
+  // It is possible that some subexpression of current immediate invocation
+  // was handled from another expression evaluation context. Do not handle
+  // current immediate invocation if some of its subexpressions failed
+