[Clang] Fix __is_trivially_equality_comparable returning true with ineligebile defaulted overloads #93113
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
philnik777
force-pushed
the
fix_is_trivially_equality_comparable
branch
from
53b52a0 to
31e3346
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Nikolas Klauser (philnik777) ChangesThis changes Fixes #89293 Full diff: https://github.com/llvm/llvm-project/pull/93113.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0c4a343b70009..cb662f520c4c3 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -606,6 +606,9 @@ Bug Fixes in This Version
- ``__is_array`` and ``__is_bounded_array`` no longer return ``true`` for
zero-sized arrays. Fixes (#GH54705).
+- ``__is_trivially_equality_comparable`` no longer returns true for types which
+ have a constrained defaulted comparison operator (#GH89293).
+
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 9a5c6e8d562c3..628c7a0d2df83 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1126,9 +1126,6 @@ class QualType {
/// Return true if this is a trivially relocatable type.
bool isTriviallyRelocatableType(const ASTContext &Context) const;
- /// Return true if this is a trivially equality comparable type.
- bool isTriviallyEqualityComparableType(const ASTContext &Context) const;
-
/// Returns true if it is a class and it might be dynamic.
bool mayBeDynamicClass() const;
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 3b90b8229dd18..62ca402460f94 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2768,66 +2768,6 @@ bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
}
}
-static bool
-HasNonDeletedDefaultedEqualityComparison(const CXXRecordDecl *Decl) {
- if (Decl->isUnion())
- return false;
- if (Decl->isLambda())
- return Decl->isCapturelessLambda();
-
- auto IsDefaultedOperatorEqualEqual = [&](const FunctionDecl *Function) {
- return Function->getOverloadedOperator() ==
- OverloadedOperatorKind::OO_EqualEqual &&
- Function->isDefaulted() && Function->getNumParams() > 0 &&
- (Function->getParamDecl(0)->getType()->isReferenceType() ||
- Decl->isTriviallyCopyable());
- };
-
- if (llvm::none_of(Decl->methods(), IsDefaultedOperatorEqualEqual) &&
- llvm::none_of(Decl->friends(), [&](const FriendDecl *Friend) {
- if (NamedDecl *ND = Friend->getFriendDecl()) {
- return ND->isFunctionOrFunctionTemplate() &&
- IsDefaultedOperatorEqualEqual(ND->getAsFunction());
- }
- return false;
- }))
- return false;
-
- return llvm::all_of(Decl->bases(),
- [](const CXXBaseSpecifier &BS) {
- if (const auto *RD = BS.getType()->getAsCXXRecordDecl())
- return HasNonDeletedDefaultedEqualityComparison(RD);
- return true;
- }) &&
- llvm::all_of(Decl->fields(), [](const FieldDecl *FD) {
- auto Type = FD->getType();
- if (Type->isArrayType())
- Type = Type->getBaseElementTypeUnsafe()->getCanonicalTypeUnqualified();
-
- if (Type->isReferenceType() || Type->isEnumeralType())
- return false;
- if (const auto *RD = Type->getAsCXXRecordDecl())
- return HasNonDeletedDefaultedEqualityComparison(RD);
- return true;
- });
-}
-
-bool QualType::isTriviallyEqualityComparableType(
- const ASTContext &Context) const {
- QualType CanonicalType = getCanonicalType();
- if (CanonicalType->isIncompleteType() || CanonicalType->isDependentType() ||
- CanonicalType->isEnumeralType() || CanonicalType->isArrayType())
- return false;
-
- if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {
- if (!HasNonDeletedDefaultedEqualityComparison(RD))
- return false;
- }
-
- return Context.hasUniqueObjectRepresentations(
- CanonicalType, /*CheckIfTriviallyCopyable=*/false);
-}
-
bool QualType::isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const {
return !Context.getLangOpts().ObjCAutoRefCount &&
Context.getLangOpts().ObjCWeak &&
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index f543e006060d6..ccf678e666ecb 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -5199,6 +5199,75 @@ static bool HasNoThrowOperator(const RecordType *RT, OverloadedOperatorKind Op,
return false;
}
+static bool
+HasNonDeletedDefaultedEqualityComparison(Sema& S, const CXXRecordDecl *Decl) {
+ if (Decl->isUnion())
+ return false;
+ if (Decl->isLambda())
+ return Decl->isCapturelessLambda();
+
+ {
+ EnterExpressionEvaluationContext UnevaluatedContext(
+ S, Sema::ExpressionEvaluationContext::Unevaluated);
+ Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
+ Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());
+
+ // const ClassT& obj;
+ OpaqueValueExpr Operand(
+ {}, Decl->getTypeForDecl()->getCanonicalTypeUnqualified().withConst(),
+ ExprValueKind::VK_LValue);
+ UnresolvedSet<16> Functions;
+ // obj == obj;
+ S.LookupBinOp(S.TUScope, {}, BinaryOperatorKind::BO_EQ, Functions);
+
+ auto Result = S.CreateOverloadedBinOp({}, BinaryOperatorKind::BO_EQ,
+ Functions, &Operand, &Operand);
+ if (Result.isInvalid() || SFINAE.hasErrorOccurred())
+ return false;
+ auto Callee = Result.getAs<CXXOperatorCallExpr>()->getDirectCallee();
+ auto ParamT = Callee->getParamDecl(0)->getType();
+ if (!Callee->isDefaulted() ||
+ (!ParamT->isReferenceType() && !Decl->isTriviallyCopyable()))
+ return false;
+ }
+
+ return llvm::all_of(Decl->bases(),
+ [&](const CXXBaseSpecifier &BS) {
+ if (const auto *RD = BS.getType()->getAsCXXRecordDecl())
+ return HasNonDeletedDefaultedEqualityComparison(S,
+ RD);
+ return true;
+ }) &&
+ llvm::all_of(Decl->fields(), [&](const FieldDecl *FD) {
+ auto Type = FD->getType();
+ if (Type->isArrayType())
+ Type = Type->getBaseElementTypeUnsafe()
+ ->getCanonicalTypeUnqualified();
+
+ if (Type->isReferenceType() || Type->isEnumeralType())
+ return false;
+ if (const auto *RD = Type->getAsCXXRecordDecl())
+ return HasNonDeletedDefaultedEqualityComparison(S, RD);
+ return true;
+ });
+}
+
+static bool isTriviallyEqualityComparableType(
+ Sema& S, QualType Type) {
+ QualType CanonicalType = Type.getCanonicalType();
+ if (CanonicalType->isIncompleteType() || CanonicalType->isDependentType() ||
+ CanonicalType->isEnumeralType() || CanonicalType->isArrayType())
+ return false;
+
+ if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {
+ if (!HasNonDeletedDefaultedEqualityComparison(S, RD))
+ return false;
+ }
+
+ return S.getASTContext().hasUniqueObjectRepresentations(
+ CanonicalType, /*CheckIfTriviallyCopyable=*/false);
+}
+
static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT,
SourceLocation KeyLoc,
TypeSourceInfo *TInfo) {
@@ -5629,7 +5698,7 @@ static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT,
Self.Diag(KeyLoc, diag::err_builtin_pass_in_regs_non_class) << T;
return false;
case UTT_IsTriviallyEqualityComparable:
- return T.isTriviallyEqualityComparableType(C);
+ return isTriviallyEqualityComparableType(Self, T);
}
}
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index d40605f56f1ed..d06495e9cfb66 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -3885,8 +3885,36 @@ struct NotTriviallyEqualityComparableNonTriviallyEqualityComparableArrs2 {
bool operator==(const NotTriviallyEqualityComparableNonTriviallyEqualityComparableArrs2&) const = default;
};
+
static_assert(!__is_trivially_equality_comparable(NotTriviallyEqualityComparableNonTriviallyEqualityComparableArrs2));
+template<bool B>
+struct MaybeTriviallyEqualityComparable {
+ int i;
+ bool operator==(const MaybeTriviallyEqualityComparable&) const requires B = default;
+ bool operator==(const MaybeTriviallyEqualityComparable& rhs) const { return (i % 3) == (rhs.i % 3); }
+};
+static_assert(__is_trivially_equality_comparable(MaybeTriviallyEqualityComparable<true>));
+static_assert(!__is_trivially_equality_comparable(MaybeTriviallyEqualityComparable<false>));
+
+struct NotTriviallyEqualityComparableMoreConstrainedExternalOp {
+ int i;
+ bool operator==(const NotTriviallyEqualityComparableMoreConstrainedExternalOp&) const = default;
+};
+
+bool operator==(const NotTriviallyEqualityComparableMoreConstrainedExternalOp&,
+ const NotTriviallyEqualityComparableMoreConstrainedExternalOp&) __attribute__((enable_if(true, ""))) {}
+
+static_assert(!__is_trivially_equality_comparable(NotTriviallyEqualityComparableMoreConstrainedExternalOp));
+
+struct TriviallyEqualityComparableExternalDefaultedOp {
+ int i;
+ friend bool operator==(TriviallyEqualityComparableExternalDefaultedOp, TriviallyEqualityComparableExternalDefaultedOp);
+};
+bool operator==(TriviallyEqualityComparableExternalDefaultedOp, TriviallyEqualityComparableExternalDefaultedOp) = default;
+
+static_assert(__is_trivially_equality_comparable(TriviallyEqualityComparableExternalDefaultedOp));
+
namespace hidden_friend {
struct TriviallyEqualityComparable {
|
|
Could you explain a bit about why we check |
|
This patch seems to break the following example (works in trunk, breaks after this patch). Could you please add a test case for this? |
philnik777
force-pushed
the
fix_is_trivially_equality_comparable
branch
2 times, most recently
from
cabff59 to
d1507bf
Compare
|
gentle ping |
|
LGTM now, for what it's worth. |
| @@ -1126,9 +1126,6 @@ class QualType { | |||
| /// Return true if this is a trivially relocatable type. | |||
| bool isTriviallyRelocatableType(const ASTContext &Context) const; | |||
|
|
|||
| /// Return true if this is a trivially equality comparable type. | |||
| bool isTriviallyEqualityComparableType(const ASTContext &Context) const; | |||
There was a problem hiding this comment.
It's unfortunate that the other traits are implemented directly on Type but this one is in SemaExprCXX.cpp and isn't possible to compute without access to Sema. ISTM we should be able to answer this question on the type rather than requiring semantic analysis (considering potential uses like in clang-tidy where there is no Sema available).
Perhaps another way to approach this is with the DefinitionData for classes -- as we add members to the class, we build up information about whether something is trivially copyable, etc and store that on the bits defined in CXXRecordDeclDefinitionBits.def. Maybe we could do the same for equality comparable types, then we can leave this interface in Type and do special handling if the type is a CXXRecordDecl. WDYT?
There was a problem hiding this comment.
The main problem with this is that you don't have to have a member. Take this test case for example:
struct NotTriviallyEqualityComparableMoreConstrainedExternalOp {
int i;
bool operator==(const NotTriviallyEqualityComparableMoreConstrainedExternalOp&) const = default;
};
bool operator==(const NotTriviallyEqualityComparableMoreConstrainedExternalOp&,
const NotTriviallyEqualityComparableMoreConstrainedExternalOp&) __attribute__((enable_if(true, ""))) {}
static_assert(!__is_trivially_equality_comparable(NotTriviallyEqualityComparableMoreConstrainedExternalOp));I'm sure this can also be written with some requires clause. Without the builtin telling the library there is no way to figure out that a free function instead of the member is called.
There was a problem hiding this comment.
Well shoot! That's... frustrating. What about splitting the logic between Type and Sema? e.g., Type tracks whether the type in isolation is trivially equality comparable, and Sema can use that to early return if a type is not trivially equality comparable in isolation and do additional semantic checking otherwise to provide the value for the trait.
There was a problem hiding this comment.
We're not actually storing any infomation for __is_trivially_equaltiy_comparable, so I'm not sure what the benefit would be? If we ever do that we can of course split things up to say "this definitely isn't trivially equaility comparable".
There was a problem hiding this comment.
Yeah, it's not that we already have this information, it's a different approach to your current patch so that the functionality continues to live on Type (the benefit is that external consumers like downstreams or clang-tidy could continue to use the same interface they always have but get slightly more accurate results from it). If that is more effort than it's worth, I don't insist -- I'm not aware of any breakage from your current changes.
There was a problem hiding this comment.
Hmm. I'm not sure the is much use to "this is definitely not trivially equality comparable". Right now I think I'd rather just keep it in SemaExprCXX. If there comes up a use-case or we want to save some of the information we can still move some info into Type.
There was a problem hiding this comment.
Okay, I'm sold. Thanks for entertaining the idea!
There was a problem hiding this comment.
Always happy to do that!
…eligebile defaulted overloads
philnik777
force-pushed
the
fix_is_trivially_equality_comparable
branch
from
d1507bf to
3998e9a
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/176/builds/463 Here is the relevant piece of the build log for the reference: |
|
This causes clang to crash when building chromium: I'm running creduce to get a smaller repro for it. |
|
Reverting for now as this also breaks LLVM CI builder. |
|
@ZequanWu Could you give me a reproducer? Ther CI failure looks pretty unrelated to me, since it complains about LeakSanitizer. The next build was also green. |
|
I have a partially reduced repro: I have to rename the attached file to |
|
@ZequanWu I can't successfully build your reproducer with clang trunk. Would it be possible to provide the full test case or a fully reduced one? |
No, I mean it isn't a well-formed program. There are semicolons missing after class definitions. |
That's caused by creduce but it shouldn't matter (clang shouldn't crash due to syntax errors). I attached the original crash source here as creduce is still running. The command to repro is same as above. |
Yes, but it's a lot easier to reduce a file without errors, at least for me. (And FWIW I'd also question reverting a bug fix that could result into bad code gen because of a crash-on-invalid it introduced) |
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
…eligebile defaulted overloads (llvm#93113) This changes `__is_trivially_equality_comparable` to do overload resolution instead, which fixes a couple of false-positives (and a false-negative as a drive-by). Fixes llvm#89293
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
… with ineligebile defaulted overloads" (llvm#97002) Reverts llvm#93113
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes
__is_trivially_equality_comparableto do overload resolution instead, which fixes a couple of false-positives (and a false-negative as a drive-by).Fixes #89293