[PATCH] D132874: [clang] Don't emit debug vtable information for consteval functions

[Luke Nihlen via Phabricator via cfe-commits]

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

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132874

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/cxx20-consteval-crash.cpp


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-unknown-linux-gnu 
-std=c++20 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
@@ -71,3 +72,22 @@
   return function(Item{'a'}, Item{'a'});
 }
 } // namespace Issue58871
+
+namespace Issue55065 {
+struct Base {
+  consteval virtual int Get() const = 0;
+};
+
+struct Derived : Base {
+  consteval int Get() const override {
+return 42;
+  }
+};
+
+int foo() {
+  constexpr Derived a;
+
+  auto val = a.Get();
+  return val;
+}
+} // namespace Issue55065
\ No newline at end of file
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1758,7 +1758,7 @@
   llvm::DISubprogram::DISPFlags SPFlags = llvm::DISubprogram::SPFlagZero;
   int ThisAdjustment = 0;
 
-  if (Method->isVirtual()) {
+  if (Method->isVirtual() && !Method->isConsteval()) {
 if (Method->isPure())
   SPFlags |= llvm::DISubprogram::SPFlagPureVirtual;
 else
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -188,10 +188,11 @@
   and `DR1734 
`_.
 - Class member variables are now in scope when parsing a ``requires`` clause. 
Fixes
   `GH55216 `_.
-
 - Correctly set expression evaluation context as 'immediate function context' 
in
   consteval functions.
   This fixes `GH51182 `
+- Fixes an assert crash caused by looking up missing vtable information on 
``consteval``
+  virtual functions. Fixes `GH55065 
`_.
 
 
 C++2b Feature Support


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-unknown-linux-gnu -std=c++20 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
@@ -71,3 +72,22 @@
   return function(Item{'a'}, Item{'a'});
 }
 } // namespace Issue58871
+
+namespace Issue55065 {
+struct Base {
+  consteval virtual int Get() const = 0;
+};
+
+struct Derived : Base {
+  consteval int Get() const override {
+return 42;
+  }
+};
+
+int foo() {
+  constexpr Derived a;
+
+  auto val = a.Get();
+  return val;
+}
+} // namespace Issue55065
\ No newline at end of file
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1758,7 +1758,7 @@
   llvm::DISubprogram::DISPFlags SPFlags = llvm::DISubprogram::SPFlagZero;
   int ThisAdjustment = 0;
 
-  if (Method->isVirtual()) {
+  if (Method->isVirtual() && !Method->isConsteval()) {
 if (Method->isPure())
   SPFlags |= llvm::DISubprogram::SPFlagPureVirtual;
 else
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -188,10 +188,11 @@
   and `DR1734 `_.
 - Class member variables are now in scope when parsing a ``requires`` clause. Fixes
   `GH55216 `_.
-
 - Correctly set expression evaluation context as 'immediate function context' in
   consteval functions.
   This fixes `GH51182 `
+- Fixes an assert crash caused by looking up missing vtable information on ``consteval``
+  virtual functions. Fixes `GH55065 `_.
 
 
 C++2b Feature Support
___
cfe-commits mailing list
cfe-commits@lists.llvm.

[PATCH] D132874: [clang] Don't emit debug vtable information for consteval functions

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added reviewers: aaron.ballman, ilya-biryukov.
luken-google added a comment.

Note that there's still an underlying issue where the linker complains of 
missing vtable information for both Base and Derived, which I'm treating as a 
separate issue and will either file or de-dup against an existing issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132874

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

[PATCH] D132874: [clang] Don't emit debug vtable information for consteval functions

[Luke Nihlen via Phabricator via cfe-commits]

luken-google marked an inline comment as done.
luken-google added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1761
 
-  if (Method->isVirtual()) {
+  if (Method->isVirtual() && !Method->isConsteval()) {
 if (Method->isPure())

shafik wrote:
> It is not clear to me if this is the core issue or not. Can you explain this 
> a little better.
Thanks for the feedback!

Sure, here's what I'm thinking. This code calls 
ItaniumVTableContext::getMethodVTableIndex(GlobalDecl) on line 1771 below. That 
method asserts in VTableBuilder.cpp:2281 because it tries to look up a vtable 
entry for the consteval method and fails to do so. Any consteval methods are 
prevented from gaining vtable entries by the logic in 
VTableContextBase::hasVtableSlot(const CXXMethodDecl *MD):

```
bool VTableContextBase::hasVtableSlot(const CXXMethodDecl *MD) {
  return MD->isVirtual() && !MD->isConsteval();
}
```

This call is peppered throughout the VTableBuilder code, but the relevant call 
for our purposes is in ItaniumVTableBuilder::AddMethods() in 
VTableBuilder.cpp:1492, which causes any consteval method to be skipped when 
the code is iterating through all virtual member functions and adding them to 
the vtable.

My fix mirrors the logic in VTableContextBase::hasVtableSlot here, because the 
code assumes that if the method is virtual it has a vtable index, which 
consteval functions don't. Writing this, I've convinced myself it's better to 
just call that method directly, so I've refactored the code to avoid the 
duplicate logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132874

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

[PATCH] D132874: [clang] Don't emit debug vtable information for consteval functions

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 456641.
luken-google added a comment.

Change to method call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132874

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/cxx20-consteval-crash.cpp


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-unknown-linux-gnu 
-std=c++20 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
@@ -71,3 +72,23 @@
   return function(Item{'a'}, Item{'a'});
 }
 } // namespace Issue58871
+
+namespace Issue55065 {
+struct Base {
+  consteval virtual int Get() const = 0;
+};
+
+struct Derived : Base {
+  consteval int Get() const override {
+return 42;
+  }
+};
+
+int foo() {
+  constexpr Derived a;
+
+  auto val = a.Get();
+  return val;
+}
+} // namespace Issue55065
+
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -26,6 +26,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/VTableBuilder.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
@@ -1758,7 +1759,7 @@
   llvm::DISubprogram::DISPFlags SPFlags = llvm::DISubprogram::SPFlagZero;
   int ThisAdjustment = 0;
 
-  if (Method->isVirtual()) {
+  if (VTableContextBase::hasVtableSlot(Method)) {
 if (Method->isPure())
   SPFlags |= llvm::DISubprogram::SPFlagPureVirtual;
 else
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -188,10 +188,11 @@
   and `DR1734 
`_.
 - Class member variables are now in scope when parsing a ``requires`` clause. 
Fixes
   `GH55216 `_.
-
 - Correctly set expression evaluation context as 'immediate function context' 
in
   consteval functions.
   This fixes `GH51182 `
+- Fixes an assert crash caused by looking up missing vtable information on 
``consteval``
+  virtual functions. Fixes `GH55065 
`_.
 
 
 C++2b Feature Support


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-unknown-linux-gnu -std=c++20 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
@@ -71,3 +72,23 @@
   return function(Item{'a'}, Item{'a'});
 }
 } // namespace Issue58871
+
+namespace Issue55065 {
+struct Base {
+  consteval virtual int Get() const = 0;
+};
+
+struct Derived : Base {
+  consteval int Get() const override {
+return 42;
+  }
+};
+
+int foo() {
+  constexpr Derived a;
+
+  auto val = a.Get();
+  return val;
+}
+} // namespace Issue55065
+
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -26,6 +26,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/VTableBuilder.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
@@ -1758,7 +1759,7 @@
   llvm::DISubprogram::DISPFlags SPFlags = llvm::DISubprogram::SPFlagZero;
   int ThisAdjustment = 0;
 
-  if (Method->isVirtual()) {
+  if (VTableContextBase::hasVtableSlot(Method)) {
 if (Method->isPure())
   SPFlags |= llvm::DISubprogram::SPFlagPureVirtual;
 else
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -188,10 +188,11 @@
   and `DR1734 `_.
 - Class member variables are now in scope when parsing a ``requires`` clause. Fixes
   `GH55216 `_.
-
 - Correctly set expression evaluation context a

[PATCH] D132874: [clang] Don't emit debug vtable information for consteval functions

[Luke Nihlen via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc9aba6007451: [clang] Don't emit debug vtable 
information for consteval functions (authored by luken-google).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132874

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/cxx20-consteval-crash.cpp


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-unknown-linux-gnu 
-std=c++20 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
@@ -71,3 +72,23 @@
   return function(Item{'a'}, Item{'a'});
 }
 } // namespace Issue58871
+
+namespace Issue55065 {
+struct Base {
+  consteval virtual int Get() const = 0;
+};
+
+struct Derived : Base {
+  consteval int Get() const override {
+return 42;
+  }
+};
+
+int foo() {
+  constexpr Derived a;
+
+  auto val = a.Get();
+  return val;
+}
+} // namespace Issue55065
+
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -26,6 +26,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/VTableBuilder.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
@@ -1758,7 +1759,7 @@
   llvm::DISubprogram::DISPFlags SPFlags = llvm::DISubprogram::SPFlagZero;
   int ThisAdjustment = 0;
 
-  if (Method->isVirtual()) {
+  if (VTableContextBase::hasVtableSlot(Method)) {
 if (Method->isPure())
   SPFlags |= llvm::DISubprogram::SPFlagPureVirtual;
 else
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -198,10 +198,11 @@
   and `DR1734 
`_.
 - Class member variables are now in scope when parsing a ``requires`` clause. 
Fixes
   `GH55216 `_.
-
 - Correctly set expression evaluation context as 'immediate function context' 
in
   consteval functions.
   This fixes `GH51182 `
+- Fixes an assert crash caused by looking up missing vtable information on 
``consteval``
+  virtual functions. Fixes `GH55065 
`_.
 
 
 C++2b Feature Support


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-unknown-linux-gnu -std=c++20 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
@@ -71,3 +72,23 @@
   return function(Item{'a'}, Item{'a'});
 }
 } // namespace Issue58871
+
+namespace Issue55065 {
+struct Base {
+  consteval virtual int Get() const = 0;
+};
+
+struct Derived : Base {
+  consteval int Get() const override {
+return 42;
+  }
+};
+
+int foo() {
+  constexpr Derived a;
+
+  auto val = a.Get();
+  return val;
+}
+} // namespace Issue55065
+
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -26,6 +26,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/VTableBuilder.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
@@ -1758,7 +1759,7 @@
   llvm::DISubprogram::DISPFlags SPFlags = llvm::DISubprogram::SPFlagZero;
   int ThisAdjustment = 0;
 
-  if (Method->isVirtual()) {
+  if (VTableContextBase::hasVtableSlot(Method)) {
 if (Method->isPure())
   SPFlags |= llvm::DISubprogram::SPFlagPureVirtual;
 else
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -198,10 +198,11 @@
   and `DR1734 `_.
 - Class member variables are now in scope when parsing a ``requires`` clause. Fixes
   `GH55216 

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

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

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133052

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,20 @@
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 }
+
+namespace Issue50891 {
+
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+
+} // namespace Issue50891
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -22,6 +22,7 @@
 #include "clang/Sema/Designator.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/PointerIntPair.h"
@@ -4033,9 +4034,16 @@
 
 FunctionTemplateDecl *ConvTemplate = dyn_cast(D);
 CXXConversionDecl *Conv;
-if (ConvTemplate)
+if (ConvTemplate) {
+  // It is possible with C++20 constraints to cause an infinite 
template
+  // expansion loop. In those instances the conversion operator has the
+  // same specialized template argument type as the destination type,
+  // and we skip it to avoid the crash.
+  if (Initializer->getType().getCanonicalType() ==
+  DestType.getCanonicalType())
+continue;
   Conv = cast(ConvTemplate->getTemplatedDecl());
-else
+} else
   Conv = cast(D);
 
 if (ConvTemplate)
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -210,7 +210,8 @@
   This fixes `GH51182 `
 - Fixes an assert crash caused by looking up missing vtable information on 
``consteval``
   virtual functions. Fixes `GH55065 
`_.
-
+- Fixed a crash during template instantiation on a conversion operator during 
constraint
+  evaulation. Fixes `GH50891 
`_.
 
 C++2b Feature Support
 ^


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,20 @@
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 'C'}}
 }
+
+namespace Issue50891 {
+
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+
+} // namespace Issue50891
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -22,6 +22,7 @@
 #include "clang/Sema/Designator.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/Overload.h"
 #include "clang/Sema/SemaInternal.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/PointerIntPair.h"
@@ -4033,9 +4034,16 @@
 
 FunctionTemplateDecl *ConvTemplate = dyn_cast(D);
 CXXConversionDecl *Conv;
-if (ConvTemplate)
+if (ConvTemplate) {
+  // It is possible with C++20 constraints to cause an infinite template
+  // expansion loop. In those instances the conversion operator has the
+  // same specialized template argument type as the destination type,
+  // and we skip it to avoid the crash.
+  if (Initializer->getType().getCanonicalType() ==
+  DestType.getCanonicalType())
+continue;
   Conv = cast(ConvTemplate->getTemplatedDecl());
-else
+} else
   Conv = cast(D);
 
 if (ConvTemplate)
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -210,7 +210,8 @@
   This fixes `GH51182 

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 457084.
luken-google added a comment.

Remove extraneous header inclusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,20 @@
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 }
+
+namespace Issue50891 {
+
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+
+} // namespace Issue50891
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -4033,9 +4033,16 @@
 
 FunctionTemplateDecl *ConvTemplate = dyn_cast(D);
 CXXConversionDecl *Conv;
-if (ConvTemplate)
+if (ConvTemplate) {
+  // It is possible with C++20 constraints to cause an infinite 
template
+  // expansion loop. In those instances the conversion operator has the
+  // same specialized template argument type as the destination type,
+  // and we skip it to avoid the crash.
+  if (Initializer->getType().getCanonicalType() ==
+  DestType.getCanonicalType())
+continue;
   Conv = cast(ConvTemplate->getTemplatedDecl());
-else
+} else
   Conv = cast(D);
 
 if (ConvTemplate)
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -210,7 +210,8 @@
   This fixes `GH51182 `
 - Fixes an assert crash caused by looking up missing vtable information on 
``consteval``
   virtual functions. Fixes `GH55065 
`_.
-
+- Fixed a crash during template instantiation on a conversion operator during 
constraint
+  evaulation. Fixes `GH50891 
`_.
 
 C++2b Feature Support
 ^


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,20 @@
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 'C'}}
 }
+
+namespace Issue50891 {
+
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+
+} // namespace Issue50891
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -4033,9 +4033,16 @@
 
 FunctionTemplateDecl *ConvTemplate = dyn_cast(D);
 CXXConversionDecl *Conv;
-if (ConvTemplate)
+if (ConvTemplate) {
+  // It is possible with C++20 constraints to cause an infinite template
+  // expansion loop. In those instances the conversion operator has the
+  // same specialized template argument type as the destination type,
+  // and we skip it to avoid the crash.
+  if (Initializer->getType().getCanonicalType() ==
+  DestType.getCanonicalType())
+continue;
   Conv = cast(ConvTemplate->getTemplatedDecl());
-else
+} else
   Conv = cast(D);
 
 if (ConvTemplate)
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -210,7 +210,8 @@
   This fixes `GH51182 `
 - Fixes an assert crash caused by looking up missing vtable information on ``consteval``
   virtual functions. Fixes `GH55065 `_.
-
+- Fixed a crash during template instantiation on a conversion operator during constraint
+  evaulation. Fixes `GH50891 `_.
 
 C++2b Feature Support
 ^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a reviewer: ilya-biryukov.
luken-google added a comment.

I'm open to other suggestions if this doesn't seem like the right approach. The 
problem is the concept evaluation code is fairly separated in the call stack 
from this Sema code, so it's hard to think of a minimal change to the Sema code 
that doesn't break other parts of the language standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

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

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

In D133052#3762596 , @ychen wrote:

> Like mentioned in 
> https://stackoverflow.com/questions/68853472/is-this-a-bug-in-clangs-c20-concepts-implementation-unnecessary-checking-of,
>  could we not go down the path of considering conversion candidates? It seems 
> that's blessed by the standard.

If I'm understanding the code correctly, the intent of this patch is to 
definitely consider conversion candidates, only to exclude those conversion 
candidates that are templated methods where the From type is the same as the To 
type, which to me mean they are possibly redundant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

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

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 457262.
luken-google added a comment.

Refactor to use CodeSynthesisContexts


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,20 @@
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 }
+
+namespace Issue50891 {
+
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+
+} // namespace Issue50891
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -4009,6 +4009,16 @@
 }
   }
 
+  if (!S.CodeSynthesisContexts.empty())
+llvm::errs() << "Kind: " << S.CodeSynthesisContexts.back().Kind << "\n";
+
+  // Avoid an infinite template expansion loop in requirements checking by
+  // skipping the conversion functions check.
+  bool InRequirementInstantiation =
+  !S.CodeSynthesisContexts.empty() &&
+  S.CodeSynthesisContexts.back().Kind ==
+  Sema::CodeSynthesisContext::RequirementInstantiation;
+
   // FIXME: Work around a bug in C++17 guaranteed copy elision.
   //
   // When initializing an object of class type T by constructor
@@ -4021,7 +4031,7 @@
   // Note: SecondStepOfCopyInit is only ever true in this case when
   // evaluating whether to produce a C++98 compatibility warning.
   if (S.getLangOpts().CPlusPlus17 && Args.size() == 1 &&
-  !SecondStepOfCopyInit) {
+  !SecondStepOfCopyInit && !InRequirementInstantiation) {
 Expr *Initializer = Args[0];
 auto *SourceRD = Initializer->getType()->getAsCXXRecordDecl();
 if (SourceRD && S.isCompleteType(DeclLoc, Initializer->getType())) {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -210,7 +210,8 @@
   This fixes `GH51182 `
 - Fixes an assert crash caused by looking up missing vtable information on 
``consteval``
   virtual functions. Fixes `GH55065 
`_.
-
+- Fixed a crash during template instantiation on a conversion operator during 
constraint
+  evaulation. Fixes `GH50891 
`_.
 
 C++2b Feature Support
 ^


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,20 @@
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 'C'}}
 }
+
+namespace Issue50891 {
+
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+
+} // namespace Issue50891
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -4009,6 +4009,16 @@
 }
   }
 
+  if (!S.CodeSynthesisContexts.empty())
+llvm::errs() << "Kind: " << S.CodeSynthesisContexts.back().Kind << "\n";
+
+  // Avoid an infinite template expansion loop in requirements checking by
+  // skipping the conversion functions check.
+  bool InRequirementInstantiation =
+  !S.CodeSynthesisContexts.empty() &&
+  S.CodeSynthesisContexts.back().Kind ==
+  Sema::CodeSynthesisContext::RequirementInstantiation;
+
   // FIXME: Work around a bug in C++17 guaranteed copy elision.
   //
   // When initializing an object of class type T by constructor
@@ -4021,7 +4031,7 @@
   // Note: SecondStepOfCopyInit is only ever true in this case when
   // evaluating whether to produce a C++98 compatibility warning.
   if (S.getLangOpts().CPlusPlus17 && Args.size() == 1 &&
-  !SecondStepOfCopyInit) {
+  !SecondStepOfCopyInit && !InRequirementInstantiation) {
 Expr *Initializer = Args[0];
 auto *SourceRD = Initializer->getType()->getAsCXXRecordDecl();
 if (SourceRD && S.isCompleteType(DeclLoc, Initializer->getType())) {
Index: clang/docs/ReleaseNotes.rst
===
--

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 457264.
luken-google added a comment.

Remove debugging cruft


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,20 @@
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 }
+
+namespace Issue50891 {
+
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+
+} // namespace Issue50891
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -4009,6 +4009,13 @@
 }
   }
 
+  // Avoid an infinite template expansion loop in requirements checking by
+  // skipping the conversion functions check.
+  bool InRequirementInstantiation =
+  !S.CodeSynthesisContexts.empty() &&
+  S.CodeSynthesisContexts.back().Kind ==
+  Sema::CodeSynthesisContext::RequirementInstantiation;
+
   // FIXME: Work around a bug in C++17 guaranteed copy elision.
   //
   // When initializing an object of class type T by constructor
@@ -4021,7 +4028,7 @@
   // Note: SecondStepOfCopyInit is only ever true in this case when
   // evaluating whether to produce a C++98 compatibility warning.
   if (S.getLangOpts().CPlusPlus17 && Args.size() == 1 &&
-  !SecondStepOfCopyInit) {
+  !SecondStepOfCopyInit && !InRequirementInstantiation) {
 Expr *Initializer = Args[0];
 auto *SourceRD = Initializer->getType()->getAsCXXRecordDecl();
 if (SourceRD && S.isCompleteType(DeclLoc, Initializer->getType())) {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -210,7 +210,8 @@
   This fixes `GH51182 `
 - Fixes an assert crash caused by looking up missing vtable information on 
``consteval``
   virtual functions. Fixes `GH55065 
`_.
-
+- Fixed a crash during template instantiation on a conversion operator during 
constraint
+  evaulation. Fixes `GH50891 
`_.
 
 C++2b Feature Support
 ^


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,20 @@
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 'C'}}
 }
+
+namespace Issue50891 {
+
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+
+} // namespace Issue50891
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -4009,6 +4009,13 @@
 }
   }
 
+  // Avoid an infinite template expansion loop in requirements checking by
+  // skipping the conversion functions check.
+  bool InRequirementInstantiation =
+  !S.CodeSynthesisContexts.empty() &&
+  S.CodeSynthesisContexts.back().Kind ==
+  Sema::CodeSynthesisContext::RequirementInstantiation;
+
   // FIXME: Work around a bug in C++17 guaranteed copy elision.
   //
   // When initializing an object of class type T by constructor
@@ -4021,7 +4028,7 @@
   // Note: SecondStepOfCopyInit is only ever true in this case when
   // evaluating whether to produce a C++98 compatibility warning.
   if (S.getLangOpts().CPlusPlus17 && Args.size() == 1 &&
-  !SecondStepOfCopyInit) {
+  !SecondStepOfCopyInit && !InRequirementInstantiation) {
 Expr *Initializer = Args[0];
 auto *SourceRD = Initializer->getType()->getAsCXXRecordDecl();
 if (SourceRD && S.isCompleteType(DeclLoc, Initializer->getType())) {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -210,7 +210,8 @@
   This fixes `GH51182 `
 - Fixes an assert crash caused by looking up missing vtable information on ``consteval``
   virtua

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

In D133052#3763201 , @ychen wrote:

> In D133052#3763041 , @luken-google 
> wrote:
>
>> In D133052#3762596 , @ychen wrote:
>>
>>> Like mentioned in 
>>> https://stackoverflow.com/questions/68853472/is-this-a-bug-in-clangs-c20-concepts-implementation-unnecessary-checking-of,
>>>  could we not go down the path of considering conversion candidates? It 
>>> seems that's blessed by the standard.
>>
>> If I'm understanding the code correctly, the intent of this patch is to 
>> definitely consider conversion candidates, only to exclude those conversion 
>> candidates that are templated methods where the From type is the same as the 
>> To type, which to me mean they are possibly redundant?
>
> Excluding them is basically saying "because it may be a redundant conversion, 
> we should not consider it as the best via function." which doesn't seem 
> correct to me.
>
> I think the straightforward approach would be to check if we're in the 
> `ConstraintCheck` instantiation context, and if so check if any template 
> parameter is constrained by the same concept. However, I'm worried about the 
> overhead. So I'd prefer to skip this add-conv-candicates-for-copy-elision 
> path 
> (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaInit.cpp#L4012-L4053)
>  during the concept check. The compiler guarantees copy elision under certain 
> conditions (C++17) but I could not think of any situation that users want to 
> or could check copy elision inside the concept. So I think we're safe.

Thanks for your suggestion, I didn't know about the context member in Sema. I 
agree I think this is a much better approach than my original. While looping 
the code is in the `RequirementInstantiation` context, so that's the one I've 
keyed off here. Please let me know if this is what you had in mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

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

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 457579.
luken-google marked an inline comment as done.
luken-google added a comment.

Rebase and lazily check condition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,20 @@
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 }
+
+namespace Issue50891 {
+
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+
+} // namespace Issue50891
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -4020,8 +4020,14 @@
   //
   // Note: SecondStepOfCopyInit is only ever true in this case when
   // evaluating whether to produce a C++98 compatibility warning.
+  //
+  // The last check avoids an infinite template expansion loop in
+  // requirements checking by skipping the conversion functions check.
   if (S.getLangOpts().CPlusPlus17 && Args.size() == 1 &&
-  !SecondStepOfCopyInit) {
+  !SecondStepOfCopyInit &&
+  (S.CodeSynthesisContexts.empty() ||
+   S.CodeSynthesisContexts.back().Kind !=
+   Sema::CodeSynthesisContext::RequirementInstantiation)) {
 Expr *Initializer = Args[0];
 auto *SourceRD = Initializer->getType()->getAsCXXRecordDecl();
 if (SourceRD && S.isCompleteType(DeclLoc, Initializer->getType())) {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -211,16 +211,16 @@
 - Correctly set expression evaluation context as 'immediate function context' 
in
   consteval functions.
   This fixes `GH51182 `
-
 - Fixes an assert crash caused by looking up missing vtable information on 
``consteval``
   virtual functions. Fixes `GH55065 
`_.
-
 - Skip rebuilding lambda expressions in arguments of immediate invocations.
   This fixes `GH56183 `_,
   `GH51695 `_,
   `GH50455 `_,
   `GH54872 `_,
   `GH54587 `_.
+- Fixed a crash during template instantiation on a conversion operator during 
constraint
+  evaulation. Fixes `GH50891 
`_.
 
 C++2b Feature Support
 ^


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -256,3 +256,20 @@
 C auto **&j2 = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **&&j3 = g(); // expected-error {{deduced type 'int' does not satisfy 'C'}}
 }
+
+namespace Issue50891 {
+
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+
+} // namespace Issue50891
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -4020,8 +4020,14 @@
   //
   // Note: SecondStepOfCopyInit is only ever true in this case when
   // evaluating whether to produce a C++98 compatibility warning.
+  //
+  // The last check avoids an infinite template expansion loop in
+  // requirements checking by skipping the conversion functions check.
   if (S.getLangOpts().CPlusPlus17 && Args.size() == 1 &&
-  !SecondStepOfCopyInit) {
+  !SecondStepOfCopyInit &&
+  (S.CodeSynthesisContexts.empty() ||
+   S.CodeSynthesisContexts.back().Kind !=
+   Sema::CodeSynthesisContext::RequirementInstantiation)) {
 Expr *Initializer = Args[0];
 auto *SourceRD = Initializer->getType()->getAsCXXRecordDecl();
 if (SourceRD && S.isCompleteType(DeclLoc, Initializer->getType())) {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -211,16 +211,16 @

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

In D133052#3765753 , @ychen wrote:

> Oh, one more thing, we probably need to handle nested levels too, for 
> example, `foo(a);` might be triggered by a template which may be in turn 
> triggered by the concept check. So only checking 
> `S.CodeSynthesisContexts.back()` seems not enough. We need to check if we're 
> in concept check context regardless of instantiation levels.

I'll defer to your expertise on this one, but wanted to raise two concerns 
about this approach before proceeding:

a) The documentation around CodeSynthesisContexts asserts it should be treated 
as a stack (see 
https://github.com/llvm/llvm-project/blob/a5880b5f9c4a7ee543fbc38d528d2830fc27753e/clang/include/clang/Sema/Sema.h#L9131-L9132)

b) Given this is a heuristic fix, does it make sense to keep it as narrow as 
possible, to avoid unintended consequences? I share your confidence we will 
encounter other infinite template expansion bugs, but if we're not going to 
solve the general problem I would think that each individual fix should be 
conservative in its coverage.

Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

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

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

Friendly ping on this, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

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

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

@ilya-biryukov can you please take a look? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

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

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 472029.
luken-google added a comment.

Check template constraint satisfaction logic


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -764,3 +764,18 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace Issue50891 {
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+} // namespace Issue50891
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -7675,6 +7675,26 @@
 return;
   }
 
+  if (FunctionTemplate->hasAssociatedConstraints()) {
+TemplateDeductionInfo ConstraintInfo(FunctionTemplate->getLocation());
+llvm::SmallVector AC;
+FunctionTemplate->getAssociatedConstraints(AC);
+for (const Expr *ConstraintExpr : AC) {
+  Sema::InstantiatingTemplate Inst(
+  *this, ConstraintExpr->getBeginLoc(),
+  Sema::InstantiatingTemplate::ConstraintSubstitution{},
+  FunctionTemplate, ConstraintInfo, ConstraintExpr->getSourceRange());
+  if (Inst.isInvalid() || Inst.isAlreadyInstantiating()) {
+OverloadCandidate &Candidate = CandidateSet.addCandidate();
+Candidate.FoundDecl = FoundDecl;
+Candidate.Function = FunctionTemplate->getTemplatedDecl();
+Candidate.Viable = false;
+Candidate.FailureKind = ovl_fail_constraints_not_satisfied;
+return;
+  }
+}
+  }
+
   TemplateDeductionInfo Info(CandidateSet.getLocation());
   CXXConversionDecl *Specialization = nullptr;
   if (TemplateDeductionResult Result
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -588,9 +588,10 @@
   ([temp.func.order]p6.2.1 is not implemented, matching GCC).
 - Implemented `P0857R0 
`_,
   which specifies constrained lambdas and constrained template 
*template-parameter*\s.
-
 - Do not hide templated base members introduced via using-decl in derived class
   (useful specially for constrained members). Fixes `GH50886 
`_.
+- Fixed a crash during template instantiation on a conversion operator during 
constraint
+  evaulation. Fixes `GH50891 
`_.
 
 C++2b Feature Support
 ^


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -764,3 +764,18 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace Issue50891 {
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+} // namespace Issue50891
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -7675,6 +7675,26 @@
 return;
   }
 
+  if (FunctionTemplate->hasAssociatedConstraints()) {
+TemplateDeductionInfo ConstraintInfo(FunctionTemplate->getLocation());
+llvm::SmallVector AC;
+FunctionTemplate->getAssociatedConstraints(AC);
+for (const Expr *ConstraintExpr : AC) {
+  Sema::InstantiatingTemplate Inst(
+  *this, ConstraintExpr->getBeginLoc(),
+  Sema::InstantiatingTemplate::ConstraintSubstitution{},
+  FunctionTemplate, ConstraintInfo, ConstraintExpr->getSourceRange());
+  if (Inst.isInvalid() || Inst.isAlreadyInstantiating()) {
+OverloadCandidate &Candidate = CandidateSet.addCandidate();
+Candidate.FoundDecl = FoundDecl;
+Candidate.Function = FunctionTemplate->getTemplatedDecl();
+Candidate.Viable = false;
+Candidate.FailureKind = ovl_fail_constraints_not_satisfied;
+return;
+  }
+}
+  }
+
   TemplateDeductionInfo Info(CandidateSet.getLocation());
   CXXConversionDecl *Specialization = nullptr;
   if (TemplateDeductionResult Result
Index: clang/docs/ReleaseNotes.rst
===

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

Because of the change to `InsertNode` to not assert on already cached concepts 
this patch now works. If I try to catch the recursive expansion on a higher 
level the test in `concepts.cpp` for 2-level concept expansion fails. There are 
some finite recursive use cases for template expansion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

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

[PATCH] D136975: [Concepts] Correctly handle failure when checking concepts recursively

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

Looks good to me!


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

https://reviews.llvm.org/D136975

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

[PATCH] D132503: Add cxx scope if needed for requires clause.

[Luke Nihlen via Phabricator via cfe-commits]

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

Fixes issue #55216.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132503

Files:
  clang/lib/Parse/ParseTemplate.cpp
  clang/test/Parser/cxx-concepts-requires-clause.cpp


Index: clang/test/Parser/cxx-concepts-requires-clause.cpp
===
--- clang/test/Parser/cxx-concepts-requires-clause.cpp
+++ clang/test/Parser/cxx-concepts-requires-clause.cpp
@@ -12,6 +12,7 @@
   struct AA;
   enum E : int;
   static int x;
+  static constexpr int z = 16;
 
   template  requires true
   void Mfoo();
@@ -24,6 +25,8 @@
 
   template  requires true
   using MQ = M;
+
+  constexpr int bazz() requires (z == 16);
 };
 
 template  requires (!0)
@@ -56,6 +59,9 @@
 template  requires true
 using Q = A;
 
+template requires (!0)
+constexpr int A::bazz() requires (z == 16) { return z; }
+
 struct C {
   template  requires true
   void Mfoo();
Index: clang/lib/Parse/ParseTemplate.cpp
===
--- clang/lib/Parse/ParseTemplate.cpp
+++ clang/lib/Parse/ParseTemplate.cpp
@@ -289,8 +289,15 @@
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
-if (Tok.is(tok::kw_requires))
+if (Tok.is(tok::kw_requires)) {
+  DeclaratorScopeObj DeclScopeObj(*this, DeclaratorInfo.getCXXScopeSpec());
+  if (DeclaratorInfo.getCXXScopeSpec().isValid()) {
+if (Actions.ShouldEnterDeclaratorScope(getCurScope(), 
DeclaratorInfo.getCXXScopeSpec())) {
+  DeclScopeObj.EnterDeclaratorScope();
+}
+  }
   ParseTrailingRequiresClause(DeclaratorInfo);
+}
 
 MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
   }


Index: clang/test/Parser/cxx-concepts-requires-clause.cpp
===
--- clang/test/Parser/cxx-concepts-requires-clause.cpp
+++ clang/test/Parser/cxx-concepts-requires-clause.cpp
@@ -12,6 +12,7 @@
   struct AA;
   enum E : int;
   static int x;
+  static constexpr int z = 16;
 
   template  requires true
   void Mfoo();
@@ -24,6 +25,8 @@
 
   template  requires true
   using MQ = M;
+
+  constexpr int bazz() requires (z == 16);
 };
 
 template  requires (!0)
@@ -56,6 +59,9 @@
 template  requires true
 using Q = A;
 
+template requires (!0)
+constexpr int A::bazz() requires (z == 16) { return z; }
+
 struct C {
   template  requires true
   void Mfoo();
Index: clang/lib/Parse/ParseTemplate.cpp
===
--- clang/lib/Parse/ParseTemplate.cpp
+++ clang/lib/Parse/ParseTemplate.cpp
@@ -289,8 +289,15 @@
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
-if (Tok.is(tok::kw_requires))
+if (Tok.is(tok::kw_requires)) {
+  DeclaratorScopeObj DeclScopeObj(*this, DeclaratorInfo.getCXXScopeSpec());
+  if (DeclaratorInfo.getCXXScopeSpec().isValid()) {
+if (Actions.ShouldEnterDeclaratorScope(getCurScope(), DeclaratorInfo.getCXXScopeSpec())) {
+  DeclScopeObj.EnterDeclaratorScope();
+}
+  }
   ParseTrailingRequiresClause(DeclaratorInfo);
+}
 
 MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132503: Add cxx scope if needed for requires clause.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 455217.
luken-google added a comment.

Fix clang-format errors


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132503

Files:
  clang/lib/Parse/ParseTemplate.cpp
  clang/test/Parser/cxx-concepts-requires-clause.cpp


Index: clang/test/Parser/cxx-concepts-requires-clause.cpp
===
--- clang/test/Parser/cxx-concepts-requires-clause.cpp
+++ clang/test/Parser/cxx-concepts-requires-clause.cpp
@@ -12,6 +12,7 @@
   struct AA;
   enum E : int;
   static int x;
+  static constexpr int z = 16;
 
   template  requires true
   void Mfoo();
@@ -24,6 +25,8 @@
 
   template  requires true
   using MQ = M;
+
+  constexpr int bazz() requires (z == 16);
 };
 
 template  requires (!0)
@@ -56,6 +59,9 @@
 template  requires true
 using Q = A;
 
+template requires (!0)
+constexpr int A::bazz() requires (z == 16) { return z; }
+
 struct C {
   template  requires true
   void Mfoo();
Index: clang/lib/Parse/ParseTemplate.cpp
===
--- clang/lib/Parse/ParseTemplate.cpp
+++ clang/lib/Parse/ParseTemplate.cpp
@@ -289,8 +289,16 @@
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
-if (Tok.is(tok::kw_requires))
+if (Tok.is(tok::kw_requires)) {
+  DeclaratorScopeObj DeclScopeObj(*this, DeclaratorInfo.getCXXScopeSpec());
+  if (DeclaratorInfo.getCXXScopeSpec().isValid()) {
+if (Actions.ShouldEnterDeclaratorScope(
+getCurScope(), DeclaratorInfo.getCXXScopeSpec())) {
+  DeclScopeObj.EnterDeclaratorScope();
+}
+  }
   ParseTrailingRequiresClause(DeclaratorInfo);
+}
 
 MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
   }


Index: clang/test/Parser/cxx-concepts-requires-clause.cpp
===
--- clang/test/Parser/cxx-concepts-requires-clause.cpp
+++ clang/test/Parser/cxx-concepts-requires-clause.cpp
@@ -12,6 +12,7 @@
   struct AA;
   enum E : int;
   static int x;
+  static constexpr int z = 16;
 
   template  requires true
   void Mfoo();
@@ -24,6 +25,8 @@
 
   template  requires true
   using MQ = M;
+
+  constexpr int bazz() requires (z == 16);
 };
 
 template  requires (!0)
@@ -56,6 +59,9 @@
 template  requires true
 using Q = A;
 
+template requires (!0)
+constexpr int A::bazz() requires (z == 16) { return z; }
+
 struct C {
   template  requires true
   void Mfoo();
Index: clang/lib/Parse/ParseTemplate.cpp
===
--- clang/lib/Parse/ParseTemplate.cpp
+++ clang/lib/Parse/ParseTemplate.cpp
@@ -289,8 +289,16 @@
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
-if (Tok.is(tok::kw_requires))
+if (Tok.is(tok::kw_requires)) {
+  DeclaratorScopeObj DeclScopeObj(*this, DeclaratorInfo.getCXXScopeSpec());
+  if (DeclaratorInfo.getCXXScopeSpec().isValid()) {
+if (Actions.ShouldEnterDeclaratorScope(
+getCurScope(), DeclaratorInfo.getCXXScopeSpec())) {
+  DeclScopeObj.EnterDeclaratorScope();
+}
+  }
   ParseTrailingRequiresClause(DeclaratorInfo);
+}
 
 MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132503: Add cxx scope if needed for requires clause.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

Note that the following tests:

  Builtins-i386-linux :: muldc3_test.c
  SanitizerCommon-asan-i386-Linux :: Linux/crypt_r.cpp
  SanitizerCommon-asan-i386-Linux :: Posix/crypt.cpp
  SanitizerCommon-lsan-i386-Linux :: Linux/crypt_r.cpp
  SanitizerCommon-lsan-i386-Linux :: Posix/crypt.cpp
  SanitizerCommon-ubsan-i386-Linux :: Linux/crypt_r.cpp
  SanitizerCommon-ubsan-i386-Linux :: Posix/crypt.cpp

Are failing at HEAD and at this base diff without this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132503

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

[PATCH] D132670: [clang] Add cxx scope if needed for requires clause.

[Luke Nihlen via Phabricator via cfe-commits]

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

Fixes issue #55216.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132670

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Parse/ParseTemplate.cpp
  clang/test/Parser/cxx-concepts-requires-clause.cpp


Index: clang/test/Parser/cxx-concepts-requires-clause.cpp
===
--- clang/test/Parser/cxx-concepts-requires-clause.cpp
+++ clang/test/Parser/cxx-concepts-requires-clause.cpp
@@ -12,6 +12,7 @@
   struct AA;
   enum E : int;
   static int x;
+  static constexpr int z = 16;
 
   template  requires true
   void Mfoo();
@@ -24,6 +25,8 @@
 
   template  requires true
   using MQ = M;
+
+  constexpr int bazz() requires (z == 16);
 };
 
 template  requires (!0)
@@ -56,6 +59,9 @@
 template  requires true
 using Q = A;
 
+template requires (!0)
+constexpr int A::bazz() requires (z == 16) { return z; }
+
 struct C {
   template  requires true
   void Mfoo();
Index: clang/lib/Parse/ParseTemplate.cpp
===
--- clang/lib/Parse/ParseTemplate.cpp
+++ clang/lib/Parse/ParseTemplate.cpp
@@ -289,8 +289,14 @@
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
-if (Tok.is(tok::kw_requires))
+if (Tok.is(tok::kw_requires)) {
+  CXXScopeSpec ScopeSpec = DeclaratorInfo.getCXXScopeSpec();
+  DeclaratorScopeObj DeclScopeObj(*this, ScopeSpec);
+  if (ScopeSpec.isValid() &&
+  Actions.ShouldEnterDeclaratorScope(getCurScope(), ScopeSpec))
+DeclScopeObj.EnterDeclaratorScope();
   ParseTrailingRequiresClause(DeclaratorInfo);
+}
 
 MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -167,8 +167,8 @@
   (C++14 [dcl.constexpr]p6 (CWG DR647/CWG DR1358))
 - Correctly defer dependent immediate function invocations until template 
instantiation.
   This fixes `GH55601 `_.
-
-
+- Class member variables are now in scope when parsing requires clauses. Fixes
+  `GH55216 `_.
 
 C++2b Feature Support
 ^


Index: clang/test/Parser/cxx-concepts-requires-clause.cpp
===
--- clang/test/Parser/cxx-concepts-requires-clause.cpp
+++ clang/test/Parser/cxx-concepts-requires-clause.cpp
@@ -12,6 +12,7 @@
   struct AA;
   enum E : int;
   static int x;
+  static constexpr int z = 16;
 
   template  requires true
   void Mfoo();
@@ -24,6 +25,8 @@
 
   template  requires true
   using MQ = M;
+
+  constexpr int bazz() requires (z == 16);
 };
 
 template  requires (!0)
@@ -56,6 +59,9 @@
 template  requires true
 using Q = A;
 
+template requires (!0)
+constexpr int A::bazz() requires (z == 16) { return z; }
+
 struct C {
   template  requires true
   void Mfoo();
Index: clang/lib/Parse/ParseTemplate.cpp
===
--- clang/lib/Parse/ParseTemplate.cpp
+++ clang/lib/Parse/ParseTemplate.cpp
@@ -289,8 +289,14 @@
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
-if (Tok.is(tok::kw_requires))
+if (Tok.is(tok::kw_requires)) {
+  CXXScopeSpec ScopeSpec = DeclaratorInfo.getCXXScopeSpec();
+  DeclaratorScopeObj DeclScopeObj(*this, ScopeSpec);
+  if (ScopeSpec.isValid() &&
+  Actions.ShouldEnterDeclaratorScope(getCurScope(), ScopeSpec))
+DeclScopeObj.EnterDeclaratorScope();
   ParseTrailingRequiresClause(DeclaratorInfo);
+}
 
 MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -167,8 +167,8 @@
   (C++14 [dcl.constexpr]p6 (CWG DR647/CWG DR1358))
 - Correctly defer dependent immediate function invocations until template instantiation.
   This fixes `GH55601 `_.
-
-
+- Class member variables are now in scope when parsing requires clauses. Fixes
+  `GH55216 `_.
 
 C++2b Feature Support
 ^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132503: Add cxx scope if needed for requires clause.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 455620.
luken-google added a comment.

Responding to feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132503

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Parse/ParseTemplate.cpp
  clang/test/Parser/cxx-concepts-requires-clause.cpp


Index: clang/test/Parser/cxx-concepts-requires-clause.cpp
===
--- clang/test/Parser/cxx-concepts-requires-clause.cpp
+++ clang/test/Parser/cxx-concepts-requires-clause.cpp
@@ -12,6 +12,7 @@
   struct AA;
   enum E : int;
   static int x;
+  static constexpr int z = 16;
 
   template  requires true
   void Mfoo();
@@ -24,6 +25,8 @@
 
   template  requires true
   using MQ = M;
+
+  constexpr int bazz() requires (z == 16);
 };
 
 template  requires (!0)
@@ -56,6 +59,9 @@
 template  requires true
 using Q = A;
 
+template requires (!0)
+constexpr int A::bazz() requires (z == 16) { return z; }
+
 struct C {
   template  requires true
   void Mfoo();
Index: clang/lib/Parse/ParseTemplate.cpp
===
--- clang/lib/Parse/ParseTemplate.cpp
+++ clang/lib/Parse/ParseTemplate.cpp
@@ -289,8 +289,14 @@
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
-if (Tok.is(tok::kw_requires))
+if (Tok.is(tok::kw_requires)) {
+  CXXScopeSpec ScopeSpec = DeclaratorInfo.getCXXScopeSpec();
+  DeclaratorScopeObj DeclScopeObj(*this, ScopeSpec);
+  if (ScopeSpec.isValid() &&
+  Actions.ShouldEnterDeclaratorScope(getCurScope(), ScopeSpec))
+DeclScopeObj.EnterDeclaratorScope();
   ParseTrailingRequiresClause(DeclaratorInfo);
+}
 
 MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -167,8 +167,8 @@
   (C++14 [dcl.constexpr]p6 (CWG DR647/CWG DR1358))
 - Correctly defer dependent immediate function invocations until template 
instantiation.
   This fixes `GH55601 `_.
-
-
+- Class member variables are now in scope when parsing requires clauses. Fixes
+  `GH55216 `_.
 
 C++2b Feature Support
 ^


Index: clang/test/Parser/cxx-concepts-requires-clause.cpp
===
--- clang/test/Parser/cxx-concepts-requires-clause.cpp
+++ clang/test/Parser/cxx-concepts-requires-clause.cpp
@@ -12,6 +12,7 @@
   struct AA;
   enum E : int;
   static int x;
+  static constexpr int z = 16;
 
   template  requires true
   void Mfoo();
@@ -24,6 +25,8 @@
 
   template  requires true
   using MQ = M;
+
+  constexpr int bazz() requires (z == 16);
 };
 
 template  requires (!0)
@@ -56,6 +59,9 @@
 template  requires true
 using Q = A;
 
+template requires (!0)
+constexpr int A::bazz() requires (z == 16) { return z; }
+
 struct C {
   template  requires true
   void Mfoo();
Index: clang/lib/Parse/ParseTemplate.cpp
===
--- clang/lib/Parse/ParseTemplate.cpp
+++ clang/lib/Parse/ParseTemplate.cpp
@@ -289,8 +289,14 @@
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
-if (Tok.is(tok::kw_requires))
+if (Tok.is(tok::kw_requires)) {
+  CXXScopeSpec ScopeSpec = DeclaratorInfo.getCXXScopeSpec();
+  DeclaratorScopeObj DeclScopeObj(*this, ScopeSpec);
+  if (ScopeSpec.isValid() &&
+  Actions.ShouldEnterDeclaratorScope(getCurScope(), ScopeSpec))
+DeclScopeObj.EnterDeclaratorScope();
   ParseTrailingRequiresClause(DeclaratorInfo);
+}
 
 MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -167,8 +167,8 @@
   (C++14 [dcl.constexpr]p6 (CWG DR647/CWG DR1358))
 - Correctly defer dependent immediate function invocations until template instantiation.
   This fixes `GH55601 `_.
-
-
+- Class member variables are now in scope when parsing requires clauses. Fixes
+  `GH55216 `_.
 
 C++2b Feature Support
 ^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132503: Add cxx scope if needed for requires clause.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google marked 2 inline comments as done.
luken-google added a comment.

Thanks for the feedback and the code review. I've uploaded a new revision with 
the requested changes, PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132503

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

[PATCH] D132670: [clang] Add cxx scope if needed for requires clause.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google abandoned this revision.
luken-google added a comment.

Uploaded by mistake, kindly ignore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132670

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

[PATCH] D132503: [clang] Add cxx scope if needed for requires clause.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 455622.
luken-google retitled this revision from "Add cxx scope if needed for requires 
clause." to "[clang] Add cxx scope if needed for requires clause.".
luken-google added a comment.

Make ScopeSpec a reference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132503

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Parse/ParseTemplate.cpp
  clang/test/Parser/cxx-concepts-requires-clause.cpp


Index: clang/test/Parser/cxx-concepts-requires-clause.cpp
===
--- clang/test/Parser/cxx-concepts-requires-clause.cpp
+++ clang/test/Parser/cxx-concepts-requires-clause.cpp
@@ -12,6 +12,7 @@
   struct AA;
   enum E : int;
   static int x;
+  static constexpr int z = 16;
 
   template  requires true
   void Mfoo();
@@ -24,6 +25,8 @@
 
   template  requires true
   using MQ = M;
+
+  constexpr int bazz() requires (z == 16);
 };
 
 template  requires (!0)
@@ -56,6 +59,9 @@
 template  requires true
 using Q = A;
 
+template requires (!0)
+constexpr int A::bazz() requires (z == 16) { return z; }
+
 struct C {
   template  requires true
   void Mfoo();
Index: clang/lib/Parse/ParseTemplate.cpp
===
--- clang/lib/Parse/ParseTemplate.cpp
+++ clang/lib/Parse/ParseTemplate.cpp
@@ -289,8 +289,14 @@
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
-if (Tok.is(tok::kw_requires))
+if (Tok.is(tok::kw_requires)) {
+  CXXScopeSpec &ScopeSpec = DeclaratorInfo.getCXXScopeSpec();
+  DeclaratorScopeObj DeclScopeObj(*this, ScopeSpec);
+  if (ScopeSpec.isValid() &&
+  Actions.ShouldEnterDeclaratorScope(getCurScope(), ScopeSpec))
+DeclScopeObj.EnterDeclaratorScope();
   ParseTrailingRequiresClause(DeclaratorInfo);
+}
 
 MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -167,8 +167,8 @@
   (C++14 [dcl.constexpr]p6 (CWG DR647/CWG DR1358))
 - Correctly defer dependent immediate function invocations until template 
instantiation.
   This fixes `GH55601 `_.
-
-
+- Class member variables are now in scope when parsing requires clauses. Fixes
+  `GH55216 `_.
 
 C++2b Feature Support
 ^


Index: clang/test/Parser/cxx-concepts-requires-clause.cpp
===
--- clang/test/Parser/cxx-concepts-requires-clause.cpp
+++ clang/test/Parser/cxx-concepts-requires-clause.cpp
@@ -12,6 +12,7 @@
   struct AA;
   enum E : int;
   static int x;
+  static constexpr int z = 16;
 
   template  requires true
   void Mfoo();
@@ -24,6 +25,8 @@
 
   template  requires true
   using MQ = M;
+
+  constexpr int bazz() requires (z == 16);
 };
 
 template  requires (!0)
@@ -56,6 +59,9 @@
 template  requires true
 using Q = A;
 
+template requires (!0)
+constexpr int A::bazz() requires (z == 16) { return z; }
+
 struct C {
   template  requires true
   void Mfoo();
Index: clang/lib/Parse/ParseTemplate.cpp
===
--- clang/lib/Parse/ParseTemplate.cpp
+++ clang/lib/Parse/ParseTemplate.cpp
@@ -289,8 +289,14 @@
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
-if (Tok.is(tok::kw_requires))
+if (Tok.is(tok::kw_requires)) {
+  CXXScopeSpec &ScopeSpec = DeclaratorInfo.getCXXScopeSpec();
+  DeclaratorScopeObj DeclScopeObj(*this, ScopeSpec);
+  if (ScopeSpec.isValid() &&
+  Actions.ShouldEnterDeclaratorScope(getCurScope(), ScopeSpec))
+DeclScopeObj.EnterDeclaratorScope();
   ParseTrailingRequiresClause(DeclaratorInfo);
+}
 
 MaybeParseGNUAttributes(DeclaratorInfo, &LateParsedAttrs);
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -167,8 +167,8 @@
   (C++14 [dcl.constexpr]p6 (CWG DR647/CWG DR1358))
 - Correctly defer dependent immediate function invocations until template instantiation.
   This fixes `GH55601 `_.
-
-
+- Class member variables are now in scope when parsing requires clauses. Fixes
+  `GH55216 `_.
 
 C++2b Feature Support
 ^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132503: [clang] Add cxx scope if needed for requires clause.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added inline comments.



Comment at: clang/lib/Parse/ParseTemplate.cpp:293
+if (Tok.is(tok::kw_requires)) {
+  CXXScopeSpec ScopeSpec = DeclaratorInfo.getCXXScopeSpec();
+  DeclaratorScopeObj DeclScopeObj(*this, ScopeSpec);

erichkeane wrote:
> Not sure about this type, should this be a reference?
Mm, good point. Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132503

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

[PATCH] D132503: [clang] Add cxx scope if needed for requires clause.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

Yes please, I'm hoping to earn committer rights with a series of C++20 patches 
:).
Name is Luke Nihlen, email is lu...@google.com, github ID is luken-google@.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132503

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

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 460756.
luken-google added a comment.

Refactor to check during template overload evaluation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -262,3 +262,18 @@
 template  struct S {};
 void f(C auto);
 } // namespace GH55567
+
+namespace Issue50891 {
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+} // namespace Issue50891
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -7565,6 +7565,24 @@
 return;
   }
 
+  // We won't go through a user-defined type conversion function to convert a
+  // derived to base as such conversions are given Conversion Rank. They only
+  // go through a copy constructor. 13.3.3.1.2-p4 [over.ics.user]
+  // Peforming this check here avoids possible infinite template expansion
+  // in the following call to DeduceTemplateArguments().
+  QualType FromCanon =
+  Context.getCanonicalType(From->getType().getUnqualifiedType());
+  QualType ToCanon = Context.getCanonicalType(ToType).getUnqualifiedType();
+  if (FromCanon == ToCanon ||
+  IsDerivedFrom(CandidateSet.getLocation(), FromCanon, ToCanon)) {
+OverloadCandidate &Candidate = CandidateSet.addCandidate();
+Candidate.FoundDecl = FoundDecl;
+Candidate.Function = FunctionTemplate->getTemplatedDecl();
+Candidate.Viable = false;
+Candidate.FailureKind = ovl_fail_trivial_conversion;
+return;
+  }
+
   TemplateDeductionInfo Info(CandidateSet.getLocation());
   CXXConversionDecl *Specialization = nullptr;
   if (TemplateDeductionResult Result
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -4020,6 +4020,9 @@
   //
   // Note: SecondStepOfCopyInit is only ever true in this case when
   // evaluating whether to produce a C++98 compatibility warning.
+  //
+  // The last check avoids an infinite template expansion loop in
+  // requirements checking by skipping the conversion functions check.
   if (S.getLangOpts().CPlusPlus17 && Args.size() == 1 &&
   !SecondStepOfCopyInit) {
 Expr *Initializer = Args[0];
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -267,17 +267,17 @@
   `GH55216 `_.
 - Correctly set expression evaluation context as 'immediate function context' 
in
   consteval functions.
-  This fixes `GH51182 `_.
-
+  This fixes `GH51182 `
 - Fixes an assert crash caused by looking up missing vtable information on 
``consteval``
   virtual functions. Fixes `GH55065 
`_.
-
 - Skip rebuilding lambda expressions in arguments of immediate invocations.
   This fixes `GH56183 `_,
   `GH51695 `_,
   `GH50455 `_,
   `GH54872 `_,
   `GH54587 `_.
+- Fixed a crash during template instantiation on a conversion operator during 
constraint
+  evaulation. Fixes `GH50891 
`_.
 
 C++2b Feature Support
 ^


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -262,3 +262,18 @@
 template  struct S {};
 void f(C auto);
 } // namespace GH55567
+
+namespace Issue50891 {
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+} // namespace Issue50891
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -7565,6 +7565,24 @@
 return;
   }
 
+  // We won't go through a

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 460757.
luken-google added a comment.

Remove extraneous comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -262,3 +262,18 @@
 template  struct S {};
 void f(C auto);
 } // namespace GH55567
+
+namespace Issue50891 {
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+} // namespace Issue50891
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -7565,6 +7565,24 @@
 return;
   }
 
+  // We won't go through a user-defined type conversion function to convert a
+  // derived to base as such conversions are given Conversion Rank. They only
+  // go through a copy constructor. 13.3.3.1.2-p4 [over.ics.user]
+  // Peforming this check here avoids possible infinite template expansion
+  // in the following call to DeduceTemplateArguments().
+  QualType FromCanon =
+  Context.getCanonicalType(From->getType().getUnqualifiedType());
+  QualType ToCanon = Context.getCanonicalType(ToType).getUnqualifiedType();
+  if (FromCanon == ToCanon ||
+  IsDerivedFrom(CandidateSet.getLocation(), FromCanon, ToCanon)) {
+OverloadCandidate &Candidate = CandidateSet.addCandidate();
+Candidate.FoundDecl = FoundDecl;
+Candidate.Function = FunctionTemplate->getTemplatedDecl();
+Candidate.Viable = false;
+Candidate.FailureKind = ovl_fail_trivial_conversion;
+return;
+  }
+
   TemplateDeductionInfo Info(CandidateSet.getLocation());
   CXXConversionDecl *Specialization = nullptr;
   if (TemplateDeductionResult Result
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -267,17 +267,17 @@
   `GH55216 `_.
 - Correctly set expression evaluation context as 'immediate function context' 
in
   consteval functions.
-  This fixes `GH51182 `_.
-
+  This fixes `GH51182 `
 - Fixes an assert crash caused by looking up missing vtable information on 
``consteval``
   virtual functions. Fixes `GH55065 
`_.
-
 - Skip rebuilding lambda expressions in arguments of immediate invocations.
   This fixes `GH56183 `_,
   `GH51695 `_,
   `GH50455 `_,
   `GH54872 `_,
   `GH54587 `_.
+- Fixed a crash during template instantiation on a conversion operator during 
constraint
+  evaulation. Fixes `GH50891 
`_.
 
 C++2b Feature Support
 ^


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -262,3 +262,18 @@
 template  struct S {};
 void f(C auto);
 } // namespace GH55567
+
+namespace Issue50891 {
+template 
+concept Numeric =
+requires(T a) {
+foo(a);
+};
+
+struct Deferred {
+friend void foo(Deferred);
+template  operator TO();
+};
+
+static_assert(Numeric); // should not crash clang
+} // namespace Issue50891
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -7565,6 +7565,24 @@
 return;
   }
 
+  // We won't go through a user-defined type conversion function to convert a
+  // derived to base as such conversions are given Conversion Rank. They only
+  // go through a copy constructor. 13.3.3.1.2-p4 [over.ics.user]
+  // Peforming this check here avoids possible infinite template expansion
+  // in the following call to DeduceTemplateArguments().
+  QualType FromCanon =
+  Context.getCanonicalType(From->getType().getUnqualifiedType());
+  QualType ToCanon = Context.getCanonicalType(ToType).getUnqualifiedType();
+  if (FromCanon == ToCanon ||
+  IsDerivedFrom(CandidateSet.getLocation(), FromCanon, ToCanon)) {
+OverloadCandidate &Candidate = CandidateSet.a

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

@ilya-biryukov something like this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

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

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google abandoned this revision.
luken-google added a comment.

Abandoned in favor of Erich's patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052

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

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

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

Fixes GH48182.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138210

Files:
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template 
parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template requires invalid // expected-error{{use of undeclared 
identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid also here ; // expected-error{{use of undeclared 
identifier 'invalid'}}
+
+int foo() {
+bool b;
+b = invalid not just in declarations; // expected-error{{expected ';' 
after expression}}
+   // expected-error@-1{{use of 
undeclared identifier 'invalid'}}
+   // expected-error@-2{{expected 
';' after expression}}
+   // expected-error@-3{{use of 
undeclared identifier 'just'}}
+   // expected-error@-4{{unknown 
type name 'in'}}
+return b;
+}
+} // namespace GH48182
\ No newline at end of file
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -8923,17 +8923,31 @@
 return nullptr;
   }
 
-  if (TemplateParameterLists.front()->size() == 0) {
+  TemplateParameterList *Params = TemplateParameterLists.front();
+
+  if (Params->size() == 0) {
 Diag(NameLoc, diag::err_concept_no_parameters);
 return nullptr;
   }
 
+  for (TemplateParameterList::const_iterator ParamIt = Params->begin(),
+ ParamEnd = Params->end();
+   ParamIt != ParamEnd; ++ParamIt) {
+Decl const *Param = *ParamIt;
+if (Param->isParameterPack()) {
+  if (++ParamIt == ParamEnd)
+break;
+  Diag(Param->getLocation(),
+   diag::err_template_param_pack_must_be_last_template_parameter);
+  return nullptr;
+}
+  }
+
   if (DiagnoseUnexpandedParameterPack(ConstraintExpr))
 return nullptr;
 
-  ConceptDecl *NewDecl = ConceptDecl::Create(Context, DC, NameLoc, Name,
- TemplateParameterLists.front(),
- ConstraintExpr);
+  ConceptDecl *NewDecl =
+  ConceptDecl::Create(Context, DC, NameLoc, Name, Params, ConstraintExpr);
 
   if (NewDecl->hasAssociatedConstraints()) {
 // C++2a [temp.concept]p4:


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template requires invalid // expected-error{{use of undeclared identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid also here ; // expected-error{{use of undeclared identifier 'invalid'}}
+
+int foo() {
+bool b;
+b = invalid not just in declarations; // expected-error{{expected ';' after expression}}
+   // expected-error@-1{{use of undeclared identifier 'invalid'}}
+   // expected-error@-2{{expected ';' after expression}}
+   // expected-error@-3{{use of undeclared identifier 'just'}}
+   // expected-error@-4{{unknown type name 'in'}}
+return b;
+}
+} // namespace GH48182
\ No newline at end of file
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -8923,17 +8923,31 @@
 return nullptr;
   }
 
-  if (TemplateParameterLists.front()->size() == 0) {
+  TemplateParameterList *Params = TemplateParameterLists.front();
+
+  if (Params->size() == 0) {
 Diag(NameLoc, diag::err_concept_no_parameters);
 return nullptr;
   }
 
+  for (TemplateParameterList::const_iterator ParamIt = Params->begin(),
+ ParamEnd = Params->end();
+   ParamIt != ParamEnd; ++ParamIt) {
+Decl const *Param = *Param

[PATCH] D141690: [clang] fix consteval ctor code generation assert

[Luke Nihlen via Phabricator via cfe-commits]

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

Fixes GH#53983.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141690

Files:
  clang/lib/CodeGen/CGCall.cpp


Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1334,7 +1334,8 @@
 void CodeGenFunction::EmitAggregateStore(llvm::Value *Val, Address Dest,
  bool DestIsVolatile) {
   // Prefer scalar stores to first-class aggregate stores.
-  if (llvm::StructType *STy = dyn_cast(Val->getType())) {
+  llvm::StructType *STy = dyn_cast(Val->getType());
+  if (STy && 
STy->isLayoutIdentical(cast(Dest.getElementType( {
 for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
   Address EltPtr = Builder.CreateStructGEP(Dest, i);
   llvm::Value *Elt = Builder.CreateExtractValue(Val, i);


Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1334,7 +1334,8 @@
 void CodeGenFunction::EmitAggregateStore(llvm::Value *Val, Address Dest,
  bool DestIsVolatile) {
   // Prefer scalar stores to first-class aggregate stores.
-  if (llvm::StructType *STy = dyn_cast(Val->getType())) {
+  llvm::StructType *STy = dyn_cast(Val->getType());
+  if (STy && STy->isLayoutIdentical(cast(Dest.getElementType( {
 for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
   Address EltPtr = Builder.CreateStructGEP(Dest, i);
   llvm::Value *Elt = Builder.CreateExtractValue(Val, i);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141690: [clang] fix consteval ctor code generation assert

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

See: https://github.com/llvm/llvm-project/issues/53983

I can add a release note and some testing, but wanted feedback that this was 
the right approach. The problem is when generating code for the constructor for 
the `bar` element inside of `MyStruct`, `Dest` has an elementType of `Base`, 
but `Builder.CreateStructGEP` assumes the `{i8, i8}` layout implied by `Base`. 
which causes an assert trying to emit code for the second element. So, I've 
added logic to make sure the layout of `STy` and `Dest` are identical, which 
fixes the assert but doesn't break any of the existing testing around aggregate 
stores.

AFAICT the AST and lowering all looks correct, and this is an issue in code 
generation. But this is a new area of clang for me so I'd welcome suggestions 
about if there is a better fix or if the fix should be in a different place. 
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141690

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

[PATCH] D141690: [clang] fix consteval ctor code generation assert

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 489057.
luken-google added a comment.

Adds test and release note, refactors to avoid possible null pointer deref.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141690

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenCXX/cxx20-consteval-crash.cpp


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -emit-llvm 
-o - | FileCheck %s
-// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -fexceptions 
-fcxx-exceptions %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 
-fexceptions -fcxx-exceptions %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
@@ -92,3 +92,41 @@
 }
 } // namespace Issue55065
 
+namespace Issue53983 {
+// This code would previously cause a crash in code generation
+// for the `bar` element in the `MyStruct` constructor.
+struct Base {
+constexpr Base() : a(), b() {}
+char a : 1;
+char b;
+};
+
+struct Foo : private Base {
+constexpr Foo(bool b) {
+if (b) throw "test";
+}
+};
+
+struct Bar : private Base {
+consteval Bar(bool) {}
+};
+
+struct MyStruct {
+Foo foo = true;
+Bar bar = false;
+};
+MyStruct structTest;
+
+// clang should issue a constructor for MyStruct that calls Foo and Bar,
+// and a constructor for Bar that calls Base.
+// CHECK: define{{.*}} void @_ZN10Issue539838MyStructC2Ev(
+// CHECK: entry:
+// CHECK: call void @_ZN10Issue539833FooC2Eb(
+// CHECK: call void @_ZN10Issue539833BarC2Eb(
+// CHECK: ret void
+
+// CHECK: define{{.*}} void @_ZN10Issue539833BarC2Eb(
+// CHECK: entry:
+// CHECK: call void @_ZN10Issue539834BaseC2Ev(
+// CHECK: ret void
+} // namespace Issue53983
\ No newline at end of file
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1334,7 +1334,8 @@
 void CodeGenFunction::EmitAggregateStore(llvm::Value *Val, Address Dest,
  bool DestIsVolatile) {
   // Prefer scalar stores to first-class aggregate stores.
-  if (llvm::StructType *STy = dyn_cast(Val->getType())) {
+  llvm::StructType *STy = dyn_cast(Val->getType());
+  if (STy && STy->canLosslesslyBitCastTo(Dest.getElementType())) {
 for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
   Address EltPtr = Builder.CreateStructGEP(Dest, i);
   llvm::Value *Elt = Builder.CreateExtractValue(Val, i);
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -739,6 +739,8 @@
   which specifies constrained lambdas and constrained template 
*template-parameter*\s.
 - Required parameter pack to be provided at the end of the concept parameter 
list. This
   fixes `Issue 48182 `_.
+- Fixed a assert triggered in some cases of a ``consteval`` implicit cast 
constructor. Fixes
+  `Issue 53983 `_.
 
 - Do not hide templated base members introduced via using-decl in derived class
   (useful specially for constrained members). Fixes `GH50886 
`_.


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -fexceptions -fcxx-exceptions %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 -fexceptions -fcxx-exceptions %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
@@ -92,3 +92,41 @@
 }
 } // namespace Issue55065
 
+namespace Issue53983 {
+// This code would previously cause a crash in code generation
+// for the `bar` element in the `MyStruct` constructor.
+struct Base {
+constexpr Base() : a(), b() {}
+char a : 1;
+char b;
+};
+
+struct Foo : private Base {
+constexpr Foo(bool b) {
+if (b) throw "test";
+}
+};
+
+struct Bar : private Base {
+consteval Bar(bool) {}
+};
+
+struct MyStruct {
+Foo foo = true;
+Bar bar = false;

[PATCH] D141690: [clang] fix consteval ctor code generation assert

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 489058.
luken-google added a comment.

Add newline at end of test file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141690

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenCXX/cxx20-consteval-crash.cpp


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -emit-llvm 
-o - | FileCheck %s
-// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -fexceptions 
-fcxx-exceptions %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 
-fexceptions -fcxx-exceptions %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
@@ -92,3 +92,41 @@
 }
 } // namespace Issue55065
 
+namespace Issue53983 {
+// This code would previously cause a crash in code generation
+// for the `bar` element in the `MyStruct` constructor.
+struct Base {
+constexpr Base() : a(), b() {}
+char a : 1;
+char b;
+};
+
+struct Foo : private Base {
+constexpr Foo(bool b) {
+if (b) throw "test";
+}
+};
+
+struct Bar : private Base {
+consteval Bar(bool) {}
+};
+
+struct MyStruct {
+Foo foo = true;
+Bar bar = false;
+};
+MyStruct structTest;
+
+// clang should issue a constructor for MyStruct that calls Foo and Bar,
+// and a constructor for Bar that calls Base.
+// CHECK: define{{.*}} void @_ZN10Issue539838MyStructC2Ev(
+// CHECK: entry:
+// CHECK: call void @_ZN10Issue539833FooC2Eb(
+// CHECK: call void @_ZN10Issue539833BarC2Eb(
+// CHECK: ret void
+
+// CHECK: define{{.*}} void @_ZN10Issue539833BarC2Eb(
+// CHECK: entry:
+// CHECK: call void @_ZN10Issue539834BaseC2Ev(
+// CHECK: ret void
+} // namespace Issue53983
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1334,7 +1334,8 @@
 void CodeGenFunction::EmitAggregateStore(llvm::Value *Val, Address Dest,
  bool DestIsVolatile) {
   // Prefer scalar stores to first-class aggregate stores.
-  if (llvm::StructType *STy = dyn_cast(Val->getType())) {
+  llvm::StructType *STy = dyn_cast(Val->getType());
+  if (STy && STy->canLosslesslyBitCastTo(Dest.getElementType())) {
 for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
   Address EltPtr = Builder.CreateStructGEP(Dest, i);
   llvm::Value *Elt = Builder.CreateExtractValue(Val, i);
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -739,6 +739,8 @@
   which specifies constrained lambdas and constrained template 
*template-parameter*\s.
 - Required parameter pack to be provided at the end of the concept parameter 
list. This
   fixes `Issue 48182 `_.
+- Fixed a assert triggered in some cases of a ``consteval`` implicit cast 
constructor. Fixes
+  `Issue 53983 `_.
 
 - Do not hide templated base members introduced via using-decl in derived class
   (useful specially for constrained members). Fixes `GH50886 
`_.


Index: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
===
--- clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
+++ clang/test/CodeGenCXX/cxx20-consteval-crash.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 %s -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 %s -o -
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 -fexceptions -fcxx-exceptions %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-obj -debug-info-kind=constructor -std=c++20 -fexceptions -fcxx-exceptions %s -o -
 
 namespace PR50787 {
 // This code would previously cause a crash.
@@ -92,3 +92,41 @@
 }
 } // namespace Issue55065
 
+namespace Issue53983 {
+// This code would previously cause a crash in code generation
+// for the `bar` element in the `MyStruct` constructor.
+struct Base {
+constexpr Base() : a(), b() {}
+char a : 1;
+char b;
+};
+
+struct Foo : private Base {
+constexpr Foo(bool b) {
+if (b) throw "test";
+}
+};
+
+struct Bar : private Base {
+consteval Bar(bool) {}
+};
+
+struct MyStruct {
+Foo foo = true;
+Bar bar = false;
+};
+MyStruct structTest;
+
+// clang should issue a constructor for My

[PATCH] D141690: [clang] fix consteval ctor code generation assert

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:1338
+  llvm::StructType *STy = dyn_cast(Val->getType());
+  if (STy && 
STy->isLayoutIdentical(cast(Dest.getElementType( {
 for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {

ilya-biryukov wrote:
> Is it guaranteed that `Dest.getElementType()` is never something other than 
> `StructType`?
Yikes, no. And checking the cast as another condition broke a bunch of other 
cases. This works much better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141690

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

[PATCH] D127683: Adds a warning for aligned new in MSVC before C++17

[Luke Nihlen via Phabricator via cfe-commits]

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

MSVC requires C++17 to support aligned new and defining the macro
__cpp_aligned_new. Clang does not, which could result in incompatibility
between clang-cl and msvc-built binaries. Issue a warning when this
use case is detected.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127683

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/Frontend/InitPreprocessor.cpp


Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -591,7 +591,8 @@
 /// Initialize the predefined C++ language feature test macros defined in
 /// ISO/IEC JTC1/SC22/WG21 (C++) SD-6: "SG10 Feature Test Recommendations".
 static void InitializeCPlusPlusFeatureTestMacros(const LangOptions &LangOpts,
- MacroBuilder &Builder) {
+ MacroBuilder &Builder,
+ DiagnosticsEngine &Diags) {
   // C++98 features.
   if (LangOpts.RTTI)
 Builder.defineMacro("__cpp_rtti", "199711L");
@@ -666,8 +667,13 @@
 Builder.defineMacro("__cpp_guaranteed_copy_elision", "201606L");
 Builder.defineMacro("__cpp_nontype_template_parameter_auto", "201606L");
   }
-  if (LangOpts.AlignedAllocation && !LangOpts.AlignedAllocationUnavailable)
+  if (LangOpts.AlignedAllocation && !LangOpts.AlignedAllocationUnavailable) {
+// LLVM supports __cpp_aligned_new in C++ version older than C++17, but 
MSVC
+// does not. Warn about the difference in support.
+if (LangOpts.MSVCCompat && LangOpts.CPlusPlus && !LangOpts.CPlusPlus17)
+  Diags.Report(diag::warn_fe_msvc_aligned_new_before_cpp17);
 Builder.defineMacro("__cpp_aligned_new", "201606L");
+  }
   if (LangOpts.RelaxedTemplateTemplateArgs)
 Builder.defineMacro("__cpp_template_template_args", "201611L");
 
@@ -728,7 +734,8 @@
const LangOptions &LangOpts,
const FrontendOptions &FEOpts,
const PreprocessorOptions &PPOpts,
-   MacroBuilder &Builder) {
+   MacroBuilder &Builder,
+   DiagnosticsEngine &Diags) {
   // Compiler version introspection macros.
   Builder.defineMacro("__llvm__");  // LLVM Backend
   Builder.defineMacro("__clang__"); // Clang Frontend
@@ -856,7 +863,7 @@
   Twine(TI.useSignedCharForObjCBool() ? "0" : "1"));
 
   if (LangOpts.CPlusPlus)
-InitializeCPlusPlusFeatureTestMacros(LangOpts, Builder);
+InitializeCPlusPlusFeatureTestMacros(LangOpts, Builder, Diags);
 
   // darwin_constant_cfstrings controls this. This is also dependent
   // on other things like the runtime I believe.  This is set even for C code.
@@ -1326,10 +1333,10 @@
 if ((LangOpts.CUDA || LangOpts.OpenMPIsDevice || LangOpts.SYCLIsDevice) &&
 PP.getAuxTargetInfo())
   InitializePredefinedMacros(*PP.getAuxTargetInfo(), LangOpts, FEOpts,
- PP.getPreprocessorOpts(), Builder);
+ PP.getPreprocessorOpts(), Builder, 
PP.getDiagnostics());
 
 InitializePredefinedMacros(PP.getTargetInfo(), LangOpts, FEOpts,
-   PP.getPreprocessorOpts(), Builder);
+   PP.getPreprocessorOpts(), Builder, 
PP.getDiagnostics());
 
 // Install definitions to make Objective-C++ ARC work well with various
 // C++ Standard Library implementations.
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -109,6 +109,9 @@
 "unable to open output file '%0': '%1'">;
 def warn_fe_macro_contains_embedded_newline : Warning<
 "macro '%0' contains embedded newline; text after the newline is ignored">;
+def warn_fe_msvc_aligned_new_before_cpp17 : Warning<
+"possible incompatibility with MSVC, it does not support alignedNew before 
C++17">,
+InGroup;
 def warn_fe_cc_print_header_failure : Warning<
 "unable to open CC_PRINT_HEADERS file: %0 (using stderr)">;
 def warn_fe_cc_log_diagnostics_failure : Warning<


Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -591,7 +591,8 @@
 /// Initialize the predefined C++ language feature test macros defined in
 /// ISO/IEC JTC1/SC22/WG21 (C++) SD-6: "SG10 Featu

[PATCH] D127748: Adds a warning for aligned new in MSVC before C++17

[Luke Nihlen via Phabricator via cfe-commits]

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

MSVC requires C++17 to support aligned new and defining the macro
__cpp_aligned_new. Clang does not, which could result in incompatibility
between clang-cl and msvc-built binaries. Issue a warning when this
use case is detected.

BUG=https://github.com/llvm/llvm-project/issues/55389


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127748

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/test/Frontend/aligned-new-before-cpp17.cpp


Index: clang/test/Frontend/aligned-new-before-cpp17.cpp
===
--- /dev/null
+++ clang/test/Frontend/aligned-new-before-cpp17.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fms-compatibility -faligned-allocation -std=c++03 %s
+// CHECK: warning: possible incompatibility with MSVC, it does not support 
alignedNew before C++17
+// RUN: %clang_cc1 -fms-compatibility -faligned-allocation -std=c++11 %s
+// CHECK: warning: possible incompatibility with MSVC, it does not support 
alignedNew before C++17
+// RUN: %clang_cc1 -fms-compatibility -faligned-allocation -std=c++14 %s
+// CHECK: warning: possible incompatibility with MSVC, it does not support 
alignedNew before C++17
+// RUN: %clang_cc1 -fms-compatibility -faligned-allocation -std=c++17 %s
+// expected-no-diagnostics
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -591,7 +591,8 @@
 /// Initialize the predefined C++ language feature test macros defined in
 /// ISO/IEC JTC1/SC22/WG21 (C++) SD-6: "SG10 Feature Test Recommendations".
 static void InitializeCPlusPlusFeatureTestMacros(const LangOptions &LangOpts,
- MacroBuilder &Builder) {
+ MacroBuilder &Builder,
+ DiagnosticsEngine &Diags) {
   // C++98 features.
   if (LangOpts.RTTI)
 Builder.defineMacro("__cpp_rtti", "199711L");
@@ -666,8 +667,13 @@
 Builder.defineMacro("__cpp_guaranteed_copy_elision", "201606L");
 Builder.defineMacro("__cpp_nontype_template_parameter_auto", "201606L");
   }
-  if (LangOpts.AlignedAllocation && !LangOpts.AlignedAllocationUnavailable)
+  if (LangOpts.AlignedAllocation && !LangOpts.AlignedAllocationUnavailable) {
+// LLVM supports __cpp_aligned_new in C++ version older than C++17, but 
MSVC
+// does not. Warn about the difference in support.
+if (LangOpts.MSVCCompat && LangOpts.CPlusPlus && !LangOpts.CPlusPlus17)
+  Diags.Report(diag::warn_fe_msvc_aligned_new_before_cpp17);
 Builder.defineMacro("__cpp_aligned_new", "201606L");
+  }
   if (LangOpts.RelaxedTemplateTemplateArgs)
 Builder.defineMacro("__cpp_template_template_args", "201611L");
 
@@ -728,7 +734,8 @@
const LangOptions &LangOpts,
const FrontendOptions &FEOpts,
const PreprocessorOptions &PPOpts,
-   MacroBuilder &Builder) {
+   MacroBuilder &Builder,
+   DiagnosticsEngine &Diags) {
   // Compiler version introspection macros.
   Builder.defineMacro("__llvm__");  // LLVM Backend
   Builder.defineMacro("__clang__"); // Clang Frontend
@@ -856,7 +863,7 @@
   Twine(TI.useSignedCharForObjCBool() ? "0" : "1"));
 
   if (LangOpts.CPlusPlus)
-InitializeCPlusPlusFeatureTestMacros(LangOpts, Builder);
+InitializeCPlusPlusFeatureTestMacros(LangOpts, Builder, Diags);
 
   // darwin_constant_cfstrings controls this. This is also dependent
   // on other things like the runtime I believe.  This is set even for C code.
@@ -1326,10 +1333,10 @@
 if ((LangOpts.CUDA || LangOpts.OpenMPIsDevice || LangOpts.SYCLIsDevice) &&
 PP.getAuxTargetInfo())
   InitializePredefinedMacros(*PP.getAuxTargetInfo(), LangOpts, FEOpts,
- PP.getPreprocessorOpts(), Builder);
+ PP.getPreprocessorOpts(), Builder, 
PP.getDiagnostics());
 
 InitializePredefinedMacros(PP.getTargetInfo(), LangOpts, FEOpts,
-   PP.getPreprocessorOpts(), Builder);
+   PP.getPreprocessorOpts(), Builder, 
PP.getDiagnostics());
 
 // Install definitions to make Objective-C++ ARC work well with various
 // C++ Standard Library implementations.
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKin

[PATCH] D127683: Adds a warning for aligned new in MSVC before C++17

[Luke Nihlen via Phabricator via cfe-commits]

luken-google abandoned this revision.
luken-google added a comment.

Mistakenly updated change here: https://reviews.llvm.org/D127748


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127683

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

[PATCH] D127748: Adds a warning for aligned new in MSVC before C++17

[Luke Nihlen via Phabricator via cfe-commits]

luken-google abandoned this revision.
luken-google added a comment.

Probably better to address this in the Driver. See also 
https://reviews.llvm.org/D127641. I'm going to close this for now and see if we 
want to revisit after that lands or as part of that change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127748

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

[PATCH] D126664: Expand definition deprecation warning to include constexpr statements.

[Luke Nihlen via Phabricator via cfe-commits]

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

Clang currently warns on definitions downgraded to declarations
with a const modifier, but not for a constexpr modifier. This patch
updates the warning logic to warn on both inputs, and adds a test to
check the additional case as well.

See also: https://bugs.chromium.org/p/chromium/issues/detail?id=1284718


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126664

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CXX/basic/basic.def/p2.cpp


Index: clang/test/CXX/basic/basic.def/p2.cpp
===
--- clang/test/CXX/basic/basic.def/p2.cpp
+++ clang/test/CXX/basic/basic.def/p2.cpp
@@ -5,4 +5,9 @@
 static constexpr int n = 0;
   };
   const int A::n; // expected-warning {{deprecated}}
+
+  struct B {
+static constexpr int m = 0;
+  };
+  constexpr int B::m; // expected-warning {{deprecated}}
 }
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -4508,15 +4508,15 @@
   }
 
   // C++ doesn't have tentative definitions, so go right ahead and check here.
-  if (getLangOpts().CPlusPlus &&
-  New->isThisDeclarationADefinition() == VarDecl::Definition) {
+  if (getLangOpts().CPlusPlus) {
 if (Old->isStaticDataMember() && Old->getCanonicalDecl()->isInline() &&
 Old->getCanonicalDecl()->isConstexpr()) {
   // This definition won't be a definition any more once it's been merged.
   Diag(New->getLocation(),
diag::warn_deprecated_redundant_constexpr_static_def);
-} else if (VarDecl *Def = Old->getDefinition()) {
-  if (checkVarDeclRedefinition(Def, New))
+} else if (New->isThisDeclarationADefinition() == VarDecl::Definition) {
+  VarDecl *Def = Old->getDefinition();
+  if (Def && checkVarDeclRedefinition(Def, New))
 return;
 }
   }


Index: clang/test/CXX/basic/basic.def/p2.cpp
===
--- clang/test/CXX/basic/basic.def/p2.cpp
+++ clang/test/CXX/basic/basic.def/p2.cpp
@@ -5,4 +5,9 @@
 static constexpr int n = 0;
   };
   const int A::n; // expected-warning {{deprecated}}
+
+  struct B {
+static constexpr int m = 0;
+  };
+  constexpr int B::m; // expected-warning {{deprecated}}
 }
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -4508,15 +4508,15 @@
   }
 
   // C++ doesn't have tentative definitions, so go right ahead and check here.
-  if (getLangOpts().CPlusPlus &&
-  New->isThisDeclarationADefinition() == VarDecl::Definition) {
+  if (getLangOpts().CPlusPlus) {
 if (Old->isStaticDataMember() && Old->getCanonicalDecl()->isInline() &&
 Old->getCanonicalDecl()->isConstexpr()) {
   // This definition won't be a definition any more once it's been merged.
   Diag(New->getLocation(),
diag::warn_deprecated_redundant_constexpr_static_def);
-} else if (VarDecl *Def = Old->getDefinition()) {
-  if (checkVarDeclRedefinition(Def, New))
+} else if (New->isThisDeclarationADefinition() == VarDecl::Definition) {
+  VarDecl *Def = Old->getDefinition();
+  if (Def && checkVarDeclRedefinition(Def, New))
 return;
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126664: Expand definition deprecation warning to include constexpr statements.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 433395.
luken-google added a comment.

Add release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126664

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -241,6 +241,9 @@
   suggest ``#else`` as an alternative. ``#elifdef`` and ``#elifndef`` are only
   suggested when in C2x or C++2b mode. Fixes
   `Issue 51598 `_.
+- The ``-Wdeprecated`` diagnostic will now warn on out-of-line ``constexpr``
+  declarations downgraded to definitions in C++1z, in addition to the
+  existing warning on out-of-line ``const`` declarations.
 
 Non-comprehensive list of changes in this release
 -


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -241,6 +241,9 @@
   suggest ``#else`` as an alternative. ``#elifdef`` and ``#elifndef`` are only
   suggested when in C2x or C++2b mode. Fixes
   `Issue 51598 `_.
+- The ``-Wdeprecated`` diagnostic will now warn on out-of-line ``constexpr``
+  declarations downgraded to definitions in C++1z, in addition to the
+  existing warning on out-of-line ``const`` declarations.
 
 Non-comprehensive list of changes in this release
 -
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126664: Expand definition deprecation warning to include constexpr statements.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

Thanks for the review! I do need someone to commit on my behalf. Please commit 
with name "Luke Nihlen" and email address "lu...@google.com".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126664

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

[PATCH] D126664: Expand definition deprecation warning to include constexpr statements.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 433404.
luken-google added a comment.

Squash commit to include original changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126664

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CXX/basic/basic.def/p2.cpp


Index: clang/test/CXX/basic/basic.def/p2.cpp
===
--- clang/test/CXX/basic/basic.def/p2.cpp
+++ clang/test/CXX/basic/basic.def/p2.cpp
@@ -5,4 +5,9 @@
 static constexpr int n = 0;
   };
   const int A::n; // expected-warning {{deprecated}}
+
+  struct B {
+static constexpr int m = 0;
+  };
+  constexpr int B::m; // expected-warning {{deprecated}}
 }
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -4508,15 +4508,15 @@
   }
 
   // C++ doesn't have tentative definitions, so go right ahead and check here.
-  if (getLangOpts().CPlusPlus &&
-  New->isThisDeclarationADefinition() == VarDecl::Definition) {
+  if (getLangOpts().CPlusPlus) {
 if (Old->isStaticDataMember() && Old->getCanonicalDecl()->isInline() &&
 Old->getCanonicalDecl()->isConstexpr()) {
   // This definition won't be a definition any more once it's been merged.
   Diag(New->getLocation(),
diag::warn_deprecated_redundant_constexpr_static_def);
-} else if (VarDecl *Def = Old->getDefinition()) {
-  if (checkVarDeclRedefinition(Def, New))
+} else if (New->isThisDeclarationADefinition() == VarDecl::Definition) {
+  VarDecl *Def = Old->getDefinition();
+  if (Def && checkVarDeclRedefinition(Def, New))
 return;
 }
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -241,6 +241,9 @@
   suggest ``#else`` as an alternative. ``#elifdef`` and ``#elifndef`` are only
   suggested when in C2x or C++2b mode. Fixes
   `Issue 51598 `_.
+- The ``-Wdeprecated`` diagnostic will now warn on out-of-line ``constexpr``
+  declarations downgraded to definitions in C++1z, in addition to the
+  existing warning on out-of-line ``const`` declarations.
 
 Non-comprehensive list of changes in this release
 -


Index: clang/test/CXX/basic/basic.def/p2.cpp
===
--- clang/test/CXX/basic/basic.def/p2.cpp
+++ clang/test/CXX/basic/basic.def/p2.cpp
@@ -5,4 +5,9 @@
 static constexpr int n = 0;
   };
   const int A::n; // expected-warning {{deprecated}}
+
+  struct B {
+static constexpr int m = 0;
+  };
+  constexpr int B::m; // expected-warning {{deprecated}}
 }
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -4508,15 +4508,15 @@
   }
 
   // C++ doesn't have tentative definitions, so go right ahead and check here.
-  if (getLangOpts().CPlusPlus &&
-  New->isThisDeclarationADefinition() == VarDecl::Definition) {
+  if (getLangOpts().CPlusPlus) {
 if (Old->isStaticDataMember() && Old->getCanonicalDecl()->isInline() &&
 Old->getCanonicalDecl()->isConstexpr()) {
   // This definition won't be a definition any more once it's been merged.
   Diag(New->getLocation(),
diag::warn_deprecated_redundant_constexpr_static_def);
-} else if (VarDecl *Def = Old->getDefinition()) {
-  if (checkVarDeclRedefinition(Def, New))
+} else if (New->isThisDeclarationADefinition() == VarDecl::Definition) {
+  VarDecl *Def = Old->getDefinition();
+  if (Def && checkVarDeclRedefinition(Def, New))
 return;
 }
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -241,6 +241,9 @@
   suggest ``#else`` as an alternative. ``#elifdef`` and ``#elifndef`` are only
   suggested when in C2x or C++2b mode. Fixes
   `Issue 51598 `_.
+- The ``-Wdeprecated`` diagnostic will now warn on out-of-line ``constexpr``
+  declarations downgraded to definitions in C++1z, in addition to the
+  existing warning on out-of-line ``const`` declarations.
 
 Non-comprehensive list of changes in this release
 -
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 476462.
luken-google added a comment.

Adding release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template 
parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template requires invalid // expected-error{{use of undeclared 
identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid also here ; // expected-error{{use of undeclared 
identifier 'invalid'}}
+
+int foo() {
+bool b;
+b = invalid not just in declarations; // expected-error{{expected ';' 
after expression}}
+   // expected-error@-1{{use of 
undeclared identifier 'invalid'}}
+   // expected-error@-2{{expected 
';' after expression}}
+   // expected-error@-3{{use of 
undeclared identifier 'just'}}
+   // expected-error@-4{{unknown 
type name 'in'}}
+return b;
+}
+} // namespace GH48182
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -8923,17 +8923,31 @@
 return nullptr;
   }
 
-  if (TemplateParameterLists.front()->size() == 0) {
+  TemplateParameterList *Params = TemplateParameterLists.front();
+
+  if (Params->size() == 0) {
 Diag(NameLoc, diag::err_concept_no_parameters);
 return nullptr;
   }
 
+  for (TemplateParameterList::const_iterator ParamIt = Params->begin(),
+ ParamEnd = Params->end();
+   ParamIt != ParamEnd; ++ParamIt) {
+Decl const *Param = *ParamIt;
+if (Param->isParameterPack()) {
+  if (++ParamIt == ParamEnd)
+break;
+  Diag(Param->getLocation(),
+   diag::err_template_param_pack_must_be_last_template_parameter);
+  return nullptr;
+}
+  }
+
   if (DiagnoseUnexpandedParameterPack(ConstraintExpr))
 return nullptr;
 
-  ConceptDecl *NewDecl = ConceptDecl::Create(Context, DC, NameLoc, Name,
- TemplateParameterLists.front(),
- ConstraintExpr);
+  ConceptDecl *NewDecl =
+  ConceptDecl::Create(Context, DC, NameLoc, Name, Params, ConstraintExpr);
 
   if (NewDecl->hasAssociatedConstraints()) {
 // C++2a [temp.concept]p4:
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -638,6 +638,8 @@
   ([temp.func.order]p6.2.1 is not implemented, matching GCC).
 - Implemented `P0857R0 
`_,
   which specifies constrained lambdas and constrained template 
*template-parameter*\s.
+- Required parameter pack to be provided at the end of the concept parameter 
list. This
+  fixes `Issue 48182 `_.
 
 - Do not hide templated base members introduced via using-decl in derived class
   (useful specially for constrained members). Fixes `GH50886 
`_.


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template requires invalid // expected-error{{use of undeclared identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid also here ; // expected-error{{use of undeclared identifier 'invalid'}}
+
+int foo() {
+bool b;
+b = invalid not just in declarations; // expected-error{{expected ';' after expression}}
+   // expected-error@-1{{use of undeclared identifier 'invalid'}}
+   // expected-error@-2{{expected ';' after expression}}
+   // expected-error@-3{{use of undeclared identifier 'just'}}
+   // expected-error@-4{

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google added a comment.

There is logic in the template expansion that returns an error state if the 
template parameter pack was not at the end of the template parameter list. It 
returns `true` but does not generate any diagnostics, but the outer code error 
machinery assumes that diagnostics have already been generated and so considers 
the expression invalid and moves on.

As this fix no longer accepts these ill-formed templates as valid, that code no 
longer fires. I have strengthened the check there to an assert. PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

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

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google updated this revision to Diff 478019.
luken-google added a comment.

Strengthen assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template 
parameter pack must be the last template parameter}}
+concept invalid = true;
+
+template requires invalid // expected-error{{use of undeclared 
identifier 'invalid'}}
+no errors are printed
+;
+
+static_assert(invalid also here ; // expected-error{{use of undeclared 
identifier 'invalid'}}
+
+int foo() {
+bool b;
+b = invalid not just in declarations; // expected-error{{expected ';' 
after expression}}
+   // expected-error@-1{{use of 
undeclared identifier 'invalid'}}
+   // expected-error@-2{{expected 
';' after expression}}
+   // expected-error@-3{{use of 
undeclared identifier 'just'}}
+   // expected-error@-4{{unknown 
type name 'in'}}
+return b;
+}
+} // namespace GH48182
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -5981,8 +5981,12 @@
   // A non-expanded parameter pack before the end of the parameter list
   // only occurs for an ill-formed template parameter list, unless we've
   // got a partial argument list for a function template, so just bail out.
-  if (Param + 1 != ParamEnd)
+  if (Param + 1 != ParamEnd) {
+assert(
+(Template->getMostRecentDecl()->getKind() != Decl::Kind::Concept) 
&&
+"Concept templates must have parameter packs at the end.");
 return true;
+  }
 
   SugaredConverted.push_back(
   TemplateArgument::CreatePackCopy(Context, SugaredArgumentPack));
@@ -8922,17 +8926,33 @@
 return nullptr;
   }
 
-  if (TemplateParameterLists.front()->size() == 0) {
+  TemplateParameterList *Params = TemplateParameterLists.front();
+
+  if (Params->size() == 0) {
 Diag(NameLoc, diag::err_concept_no_parameters);
 return nullptr;
   }
 
+  // Ensure that the parameter pack, if present, is the last parameter in the
+  // template.
+  for (TemplateParameterList::const_iterator ParamIt = Params->begin(),
+ ParamEnd = Params->end();
+   ParamIt != ParamEnd; ++ParamIt) {
+Decl const *Param = *ParamIt;
+if (Param->isParameterPack()) {
+  if (++ParamIt == ParamEnd)
+break;
+  Diag(Param->getLocation(),
+   diag::err_template_param_pack_must_be_last_template_parameter);
+  return nullptr;
+}
+  }
+
   if (DiagnoseUnexpandedParameterPack(ConstraintExpr))
 return nullptr;
 
-  ConceptDecl *NewDecl = ConceptDecl::Create(Context, DC, NameLoc, Name,
- TemplateParameterLists.front(),
- ConstraintExpr);
+  ConceptDecl *NewDecl =
+  ConceptDecl::Create(Context, DC, NameLoc, Name, Params, ConstraintExpr);
 
   if (NewDecl->hasAssociatedConstraints()) {
 // C++2a [temp.concept]p4:
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -648,6 +648,8 @@
   ([temp.func.order]p6.2.1 is not implemented, matching GCC).
 - Implemented `P0857R0 
`_,
   which specifies constrained lambdas and constrained template 
*template-parameter*\s.
+- Required parameter pack to be provided at the end of the concept parameter 
list. This
+  fixes `Issue 48182 `_.
 
 - Do not hide templated base members introduced via using-decl in derived class
   (useful specially for constrained members). Fixes `GH50886 
`_.


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -766,3 +766,24 @@
   __iterator_traits_member_pointer_or_arrow_or_void> f;
 }
 }// namespace InheritedFromPartialSpec
+
+namespace GH48182 {
+template // expected-error{{template parameter pack must be the last template parameter}}
+concept i

[PATCH] D141690: [clang] fix consteval ctor code generation assert

[Luke Nihlen via Phabricator via cfe-commits]

luken-google abandoned this revision.
luken-google added a comment.

seems to have fixed itself..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141690

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

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

[Luke Nihlen via Phabricator via cfe-commits]

luken-google closed this revision.
luken-google added a comment.

committed at afd800fc5679ccfe6f32b097586c658628500867 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138210

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