Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Crash with concepts in diagnoseWellFormedUnsatisfiedConstraintExpr #64723

Closed
zyn0217 opened this issue Aug 16, 2023 · 7 comments
Closed

Crash with concepts in diagnoseWellFormedUnsatisfiedConstraintExpr #64723

zyn0217 opened this issue Aug 16, 2023 · 7 comments
Assignees
@zyn0217
Labels
c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema" concepts C++20 concepts confirmed Verified by a second party crash-on-invalid

Comments

@zyn0217
Copy link
Contributor

zyn0217 commented Aug 16, 2023

The following code hits an assertion from diagnoseWellFormedUnsatisfiedConstraintExpr:

template <typename _Iterator> class normal_iterator {};

/// is_convertible
template <typename From, typename To> struct is_convertible {};

template <typename From, typename To>
inline constexpr bool is_convertible_v = is_convertible<From, To>::value;

template <typename From, typename To>
concept convertible_to = is_convertible_v<From, To>;

template <typename IteratorL, typename IteratorR>
  requires requires(IteratorL __lhs, IteratorR __rhs) {
    { __lhs == __rhs } -> convertible_to<bool>;
  }
constexpr bool operator==(normal_iterator<IteratorL> __lhs,
                          normal_iterator<IteratorR> __rhs) {
  return __lhs.base() == __rhs.base();
}

template <typename T> struct Vector {
  normal_iterator<T> begin() const { return {}; }
  normal_iterator<T> end() const { return {}; }
};

class Object;

void function(Vector<Object *> objects) { objects.begin() == objects.end(); }

https://clang.godbolt.org/z/evchKETWq

See clangd/clangd#1726 for analysis.
@zyn0217 zyn0217 added clang:frontend Language frontend issues, e.g. anything involving "Sema" crash-on-invalid labels Aug 16, 2023
@zyn0217 zyn0217 self-assigned this Aug 16, 2023
@llvmbot
Copy link
Member

llvmbot commented Aug 16, 2023

@llvm/issue-subscribers-clang-frontend

@zyn0217
Copy link
Contributor Author

zyn0217 commented Aug 16, 2023

This appears to be a duplicate of #64172, as my local patch would also resolve it.

@zyn0217
Copy link
Contributor Author

zyn0217 commented Aug 16, 2023

Proposed fix: https://reviews.llvm.org/D158061

@zyn0217 zyn0217 added this to the LLVM 17.0.X Release milestone Aug 19, 2023
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Aug 19, 2023
@tru
Copy link
Collaborator

tru commented Aug 21, 2023

@AaronBallman is this something we should fix in 17.x? I see you added as a reviewer on the Phab diff.

@AaronBallman AaronBallman added c++20 concepts C++20 concepts confirmed Verified by a second party labels Aug 21, 2023
@llvmbot
Copy link
Member

llvmbot commented Aug 21, 2023

@llvm/issue-subscribers-c-20

@AaronBallman
Copy link
Collaborator

We've been asserting on that code since Clang 10.0: https://clang.godbolt.org/z/Y6drTe37z so it's not a regression. I don't think we need to block 17.x on a fix for this (esp because @erichkeane is still out for a bit longer, so we won't hear from the concepts code owner before putting out the next rc).

@tru
Copy link
Collaborator

tru commented Aug 21, 2023

I will drop this from the 17.x milestone in that case.

@tru tru moved this from Needs Triage to Done in LLVM Release Status Aug 21, 2023
@EugeneZelenko EugeneZelenko removed this from the LLVM 17.0.X Release milestone Aug 21, 2023
@zyn0217 zyn0217 closed this as completed in 2fd01d7 Sep 1, 2023
zyn0217 added a commit to zyn0217/llvm-project that referenced this issue Sep 4, 2023
@zyn0217
[clang] Construct ExprRequirement with SubstitutionDiagnostic on Subs…
c6b778e 
…tFailure

We're expecting a SubstitutionDiagnostic in diagnoseUnsatisfiedRequirement
if the status of ExprRequirement is SubstFailure. Previously, the Requirement
was created with Expr on SubstFailure by mistake, which could lead to the
assertion failure in the subsequent diagnosis.

Fixes clangd/clangd#1726
Fixes llvm#64723
Fixes llvm#64172

In addition, this patch also fixes an invalid test from D129499.

Reviewed By: erichkeane

Differential Revision: https://reviews.llvm.org/D158061
tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Sep 5, 2023
@zyn0217 @tru
[clang] Construct ExprRequirement with SubstitutionDiagnostic on Subs…
c2c9c0f 
…tFailure

We're expecting a SubstitutionDiagnostic in diagnoseUnsatisfiedRequirement
if the status of ExprRequirement is SubstFailure. Previously, the Requirement
was created with Expr on SubstFailure by mistake, which could lead to the
assertion failure in the subsequent diagnosis.

Fixes clangd/clangd#1726
Fixes llvm/llvm-project#64723
Fixes llvm/llvm-project#64172

In addition, this patch also fixes an invalid test from D129499.

Reviewed By: erichkeane

Differential Revision: https://reviews.llvm.org/D158061
avillega pushed a commit to avillega/llvm-project that referenced this issue Sep 11, 2023
@zyn0217
[clang] Construct ExprRequirement with SubstitutionDiagnostic on Subs…
a6b0271 
…tFailure

We're expecting a SubstitutionDiagnostic in diagnoseUnsatisfiedRequirement
if the status of ExprRequirement is SubstFailure. Previously, the Requirement
was created with Expr on SubstFailure by mistake, which could lead to the
assertion failure in the subsequent diagnosis.

Fixes clangd/clangd#1726
Fixes llvm#64723
Fixes llvm#64172

In addition, this patch also fixes an invalid test from D129499.

Reviewed By: erichkeane

Differential Revision: https://reviews.llvm.org/D158061
qihangkong pushed a commit to rvgpu/llvm that referenced this issue Apr 18, 2024
@zyn0217 @ZijunZhaoCCK
[clang] Construct ExprRequirement with SubstitutionDiagnostic on Subs…
12d124b 
…tFailure

We're expecting a SubstitutionDiagnostic in diagnoseUnsatisfiedRequirement
if the status of ExprRequirement is SubstFailure. Previously, the Requirement
was created with Expr on SubstFailure by mistake, which could lead to the
assertion failure in the subsequent diagnosis.

Fixes clangd/clangd#1726
Fixes llvm/llvm-project#64723
Fixes llvm/llvm-project#64172

In addition, this patch also fixes an invalid test from D129499.

Reviewed By: erichkeane

Differential Revision: https://reviews.llvm.org/D158061
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zyn0217 zyn0217

Labels
c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema" concepts C++20 concepts confirmed Verified by a second party crash-on-invalid
Projects
LLVM Release Status
Archived in project
Milestone
No milestone
Development

No branches or pull requests
5 participants
@tru @AaronBallman @EugeneZelenko @llvmbot @zyn0217