[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] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Erich Keane via Phabricator via cfe-commits]

erichkeane requested changes to this revision.
erichkeane added a comment.
This revision now requires changes to proceed.

I think the 'this results in a hard error, not failed lookup' is a necessity 
here based on discussions on the core reflector.  Also see: 
https://reviews.llvm.org/D133052


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.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D133052#3896663 , @luken-google 
wrote:

> 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.

Note that I'm still working on the recursive concept-check (see 
https://reviews.llvm.org/D136975), and will probably get this part fixed in the 
next day or two.

I don't know that we can really use the Sema::InstantiatingTemplate bit, as 
that doesn't really work in cases where we are forming a recovery expression, 
or the concept caused us to instantiate the same concepts separately somewhere 
else (like when evaluating the constexprness of a move constructor is the other 
good example).

Additionally, I think any such check needs to happen at the 'expression' level 
itself, rather than here at the function.

My biggest problem at the moment is making sure it fails overlaod resolution 
entirely, rather than just the single candidate (which this has the same 
problem I believe).


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.

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] 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  = 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  = 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.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D133052#3892650 , @erichkeane 
wrote:

> In D133052#3891768 , @usaxena95 
> wrote:
>
>> My suggestion would be to properly handle cycles in 
>> `CheckConstraintSatisfaction`. This problem goes beyond cycles introduced by 
>> conversion. See #53213 , 
>> #44304  and #45736 
>>  
>> https://godbolt.org/z/hzM6f3qnW. Most of the bugs are due to an assertion 
>> failure while inserting entry into SatisfactionCache.
>>
>> IIUC, failing on cycles would solve all of these issues.
>>
>> My suggestion would be detect cycles in `CheckConstraintSatisfaction`. We 
>> already have a way to check "whether we have seen this evaluation before ?" 
>> in `SatisfactionCache` using `FoldingSet` for `ConstraintSatisfaction`. We 
>> could use a similar set to track the constraints being evaluated. Stop 
>> evaluation when we detect a cycle. Attach appropriate details to the 
>> Satisfaction and fail the constraint.
>
> We cant use the SatisfactionCache 'just yet', we have to have a different 
> stack, but we should fail there. We should NOT fail the constraint, it needs 
> to be a hard-error based on discussion on the reflector. I'm intending to 
> commit a patch to that effect today/monday.
>
> In D133052#3892527 , @usaxena95 
> wrote:
>
>> Coincidentally 2 of the bugs were just fixed in b9a77b56d8 
>> .
>>  I do not completely agree with the approach there though. We should be 
>> fixing the root cause instead of circumventing the assert in `InsertNode` by 
>> insert-if-not-present. This is why we have 44304 and 50891 still persistent.
>
> So the problem that I solved was forming a recovery expression during 
> evaluation, this wasn't a 'circumventing' the assert, this was making sure we 
> dont try to double-cache a legitimate recursive-lookup.

Interestingly, just doing it in CheckConstraintSatisfaction 
(~SemaConcept.cpp:385) doesn't seem right, we end up catching the failure 'too 
early', since a FunctionDecl itself goes through there and has its constraints 
checked.  I suspect we actually need to check at the constraint-expr level 
instead (so down a few levels).


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.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D133052#3891768 , @usaxena95 wrote:

> My suggestion would be to properly handle cycles in 
> `CheckConstraintSatisfaction`. This problem goes beyond cycles introduced by 
> conversion. See #53213 , 
> #44304  and #45736 
>  
> https://godbolt.org/z/hzM6f3qnW. Most of the bugs are due to an assertion 
> failure while inserting entry into SatisfactionCache.
>
> IIUC, failing on cycles would solve all of these issues.
>
> My suggestion would be detect cycles in `CheckConstraintSatisfaction`. We 
> already have a way to check "whether we have seen this evaluation before ?" 
> in `SatisfactionCache` using `FoldingSet` for `ConstraintSatisfaction`. We 
> could use a similar set to track the constraints being evaluated. Stop 
> evaluation when we detect a cycle. Attach appropriate details to the 
> Satisfaction and fail the constraint.

We cant use the SatisfactionCache 'just yet', we have to have a different 
stack, but we should fail there. We should NOT fail the constraint, it needs to 
be a hard-error based on discussion on the reflector. I'm intending to commit a 
patch to that effect today/monday.

In D133052#3892527 , @usaxena95 wrote:

> Coincidentally 2 of the bugs were just fixed in b9a77b56d8 
> .
>  I do not completely agree with the approach there though. We should be 
> fixing the root cause instead of circumventing the assert in `InsertNode` by 
> insert-if-not-present. This is why we have 44304 and 50891 still persistent.

So the problem that I solved was forming a recovery expression during 
evaluation, this wasn't a 'circumventing' the assert, this was making sure we 
dont try to double-cache a legitimate recursive-lookup.


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.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added a comment.

Coincidentally 2 of the bugs were just fixed in b9a77b56d8 
.
 I do not completely agree with the approach there though. We should be fixing 
the root cause instead of circumventing the assert in `InsertNode` by 
insert-if-not-present. This is why we still have 44304 and 50891 still 
persistent.


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.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added a comment.

My suggestion would be to properly handle cycles in 
`CheckConstraintSatisfaction`. This problem goes beyond cycles introduced by 
conversion. See #53213  and 
#44304  
https://godbolt.org/z/v41ez6eW4. These two bugs are due to an assertion failure 
while inserting entry into SatisfactionCache.

My suggestion would be detect cycles in `CheckConstraintSatisfaction`. We 
already have a way to check "whether we have seen this before ?" in 
`SatisfactionCache` using `FoldingSet` for `ConstraintSatisfaction`. We could 
use a similar set to track the constraints being evaluated. Stop evaluation 
when we detect a cycle. Attach appropriate details to the Satisfaction and fail 
the constraint.


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.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added a comment.

Not sure if this has come up already but this would still crash on

  template 
  concept C = requires(T a) { foo(a); };
  struct D1 {
  template  D1(TO);
  };
  struct D2 {
  friend void foo(D1);
  };
  
  static_assert(C);

as we only consider conversion to itself (derived to base).


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.

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D133052#3792121 , @ilya-biryukov 
wrote:

> It seems wrong to change semantics of initialization when instantiating 
> concept requirements. It implies that semantic checking may behave 
> differently inside requires expressions, this is a red flag.
> Clang has a mechanism 
> 
>  to prevent recursion when considering user-defined conversion for copy 
> initialization.
>
> Currently the intention of the Clang code path that handles this case is to:
>
> 1. Deduce `TO` to be `Deferred`
> 2. Try to check `Numeric`,
> 3. Check conversions during overload resolution for `foo(a)`. Go to step 1, 
> 
> 4.  Check 
> 
>  that conversion operator converts the type to itself and mark the candidate 
> as failed.
>
> If move the check in step 4 to happen before step 3 (even if we need to 
> duplicate the check), we will get the desired semantics.
>
> Does that look reasonable?

I think the direction we sould go is not to keep this "semantics" (I'm not sure 
it should be regarded as semantics because the standard wording does not 
describe any language-level behavior, instead, just say the constructor should 
not be used in certain cases).  Because the bug report and user experience are 
that this behavior is surprising and no references in the language require it 
so it is hard to reason about. I guess that's the reason the original 
implementation describe it as a "workaround". I'd only consider this workaround 
(i.e, hoist an certain overload resolution rule to avoid recursion) if you're 
blocked by this and needs a quick fix.

Ideally, I think we should implement this copy elision elsewhere like CodGen to 
avoid this unexpected semantic (conversion functions become candidate).

> Clang will currently cut this out because the template instantiation depth is 
> too high, whereas GCC will provide a useful diagnostics that says concept 
> satisfaction computation is recursive.
> We probably want a more informative error message too. Probably worth 
> investigating separately from that particular change.

Agreed. That requires either walking or maintaining extra states in the 
template instantiation stack and detects cycle, which would make this patch 
unnecessary.


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 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 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  = 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  = CandidateSet.addCandidate();
+

[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  = 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.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

It seems wrong to change semantics of initialization when instantiating concept 
requirements. It implies that semantic checking may behave differently inside 
requires expressions, this is a red flag.
Clang has a mechanism 

 to prevent recursion when considering user-defined conversion for copy 
initialization.

Currently the intention of the Clang code path that handles this case is to:

1. Deduce `TO` to be `Deferred`
2. Try to check `Numeric`,
3. Check conversions during overload resolution for `foo(a)`. Go to step 1, 

4.  Check 

 that conversion operator converts the type to itself and mark the candidate as 
failed.

If move the check in step 4 to happen before step 3 (even if we need to 
duplicate the check), we will get the desired semantics.

Does that look reasonable?

I also agree there is a more general issue with recursive concept 
instantiations at play here, e.g. for code like

  template  concept A = requires (T a) { foo(a); };
  template  concept B = requires (T a) { bar(a); };
  
  struct X {};
  template  void bar(T);
  template  void foo(T);
  
  void test() {
  foo(X());
  }

Clang will currently cut this out because the template instantiation depth is 
too high, whereas GCC will provide a useful diagnostics that says concept 
satisfaction computation is recursive.
We probably want a more informative error message too. Probably worth 
investigating separately from that particular change.


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 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.

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 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 ** = g();  // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 C auto **& = 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 ** = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **& = 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.

[Yuanfang Chen via Phabricator via cfe-commits]

ychen requested changes to this revision.
ychen added a comment.
This revision now requires changes to proceed.

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.


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.

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:4012
 
+  // Avoid an infinite template expansion loop in requirements checking by
+  // skipping the conversion functions check.

Would be better to check this lazily (inside the if statement below).


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.

[Yuanfang Chen via Phabricator via cfe-commits]

ychen accepted this revision.
ychen added a comment.
This revision is now accepted and ready to land.

In D133052#3763983 , @luken-google 
wrote:

> 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.

LGTM. Thanks. That's what I have in mind. The patch needs a rebase though.

For future reference, I want to mention that the infinite recursion could 
happen in many other ways inside require expression. I'm not sure it is 
possible or worthwhile to check them all. I think we should do this check for 
the copy elision path because making conv-functions candidates are merely 
Clang's implementation detail about copy elision, the standard does not dictate 
*how* to perform copy elision. So avoiding infinite recursion in this path is 
better for user experiences.


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#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 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 ** = g();  // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 C auto **& = 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 ** = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **& = 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 

[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 ** = g();  // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 C auto **& = 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 ** = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **& = 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.

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

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.


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.

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

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.


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 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 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 ** = g();  // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 C auto **& = 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 ** = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **& = 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-commits

[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 ** = g();  // expected-error {{deduced type 'int' does not satisfy 
'C'}}
 C auto **& = 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 ** = g();  // expected-error {{deduced type 'int' does not satisfy 'C'}}
 C auto **& = 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