[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
Sirraide wrote: See also: #92439 https://github.com/llvm/llvm-project/pull/92283 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
Caslyn wrote: Hi @sdkrystian & @erichkeane - my apologies! Yes, indeed this code calls its own increment operator and it should be `++*this` - I failed to see that. Thanks for the feedback here and helping us fix this bug in our code. https://github.com/llvm/llvm-project/pull/92283 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
erichkeane wrote: > Hi @sdkrystian - this appears to break in a circumstance where a custom > iterator class defines the postfix operator: > > ``` > FAILED: host_x64/obj/src/lib/zbitl/tests/zbitl-unittests.mem-config-test.cc.o > ../../prebuilt/third_party/clang/custom/bin/clang++ -MD -MF > host_x64/obj/src/lib/zbitl/tests/zbitl-unittests.mem-config-test.cc.o.d > -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS > -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES > -DRAPIDJSON_HAS_STDSTRING -DRAPIDJSON_HAS_CXX11_RANGE_FOR > -DRAPIDJSON_HAS_CXX11_RVALUE_REFS -DRAPIDJSON_HAS_CXX11_TYPETRAITS > -DRAPIDJSON_HAS_CXX11_N... > In file included from ../../src/lib/zbitl/tests/mem-config-test.cc:6: > ../../src/lib/zbitl/include/lib/zbitl/items/mem-config.h:61:7: error: > expression is not assignable >61 | ++this; > | ^ > 1 error generated. > ``` > > I went ahead and attached a reproducer. Could you please revert this change > and investigate? > > [clang-crashreports.tar.gz](https://github.com/llvm/llvm-project/files/15337912/clang-crashreports.tar.gz) If the iterator is trying to call its own increment operator, that probably should be `++*this`. At the moment it is trying to do something that I'd be shocked if it isn't UB, which is incrementing 'this' itself, which is a pointer. https://github.com/llvm/llvm-project/pull/92283 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
sdkrystian wrote: @Caslyn [[expr.prim.this] p3](http://eel.is/c++draft/expr.prim.this#3) states: > [...] the expression this is a prvalue of type “pointer to _cv-qualifier-seq_ > `X`” wherever `X` is the current class [...] And [[over.match.oper] p1](http://eel.is/c++draft/over.match.oper#1) states: > If no operand of an operator in an expression has a type that is a class or > an enumeration, the operator is assumed to be a built-in operator and > interpreted according to > [[expr.compound]](http://eel.is/c++draft/expr.compound). Since the type of `this` is a pointer, `++this` uses the built-in prefix increment operator. The operand of the built-in prefix increment operator must be an lvalue (and `this` is a prvalue). https://github.com/llvm/llvm-project/pull/92283 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
Caslyn wrote: Hi @sdkrystian - this appears to break in a circumstance where a custom iterator class defines the postfix operator: ``` FAILED: host_x64/obj/src/lib/zbitl/tests/zbitl-unittests.mem-config-test.cc.o ../../prebuilt/third_party/clang/custom/bin/clang++ -MD -MF host_x64/obj/src/lib/zbitl/tests/zbitl-unittests.mem-config-test.cc.o.d -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -DRAPIDJSON_HAS_STDSTRING -DRAPIDJSON_HAS_CXX11_RANGE_FOR -DRAPIDJSON_HAS_CXX11_RVALUE_REFS -DRAPIDJSON_HAS_CXX11_TYPETRAITS -DRAPIDJSON_HAS_CXX11_N... In file included from ../../src/lib/zbitl/tests/mem-config-test.cc:6: ../../src/lib/zbitl/include/lib/zbitl/items/mem-config.h:61:7: error: expression is not assignable 61 | ++this; | ^ 1 error generated. ``` I went ahead and attached a reproducer. Could you please revert this change and investigate? [clang-crashreports.tar.gz](https://github.com/llvm/llvm-project/files/15337912/clang-crashreports.tar.gz) https://github.com/llvm/llvm-project/pull/92283 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
https://github.com/sdkrystian closed https://github.com/llvm/llvm-project/pull/92283 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
https://github.com/erichkeane approved this pull request. https://github.com/llvm/llvm-project/pull/92283 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
sdkrystian wrote: All the new changes are in 5ce0e969f3f94e9694545fe71b14fd8eb086f33e https://github.com/llvm/llvm-project/pull/92283 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
erichkeane wrote: Can you point out the 'diff' inline what the change from the previous commit is? https://github.com/llvm/llvm-project/pull/92283 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Krystian Stasiowski (sdkrystian)
Changes
This patch reapplies #90500, addressing a bug which caused binary
operators with dependent operands to be incorrectly rebuilt by `TreeTransform`.
---
Patch is 46.60 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/92283.diff
17 Files Affected:
- (modified) clang/docs/ReleaseNotes.rst (+3)
- (modified) clang/include/clang/AST/Type.h (+4-1)
- (modified) clang/lib/Sema/SemaExpr.cpp (+177-186)
- (modified) clang/lib/Sema/TreeTransform.h (+7-10)
- (modified) clang/test/AST/ast-dump-expr-json.cpp (+2-2)
- (modified) clang/test/AST/ast-dump-expr.cpp (+1-1)
- (modified) clang/test/AST/ast-dump-lambda.cpp (+1-1)
- (added) clang/test/CXX/expr/expr.unary/expr.unary.general/p1.cpp (+65)
- (modified) clang/test/CXX/over/over.built/ast.cpp (+128-30)
- (modified) clang/test/CXX/over/over.built/p10.cpp (+1-1)
- (modified) clang/test/CXX/over/over.built/p11.cpp (+1-1)
- (added) clang/test/CXX/over/over.oper/over.oper.general/p1.cpp (+173)
- (modified) clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp
(+10-15)
- (modified) clang/test/Frontend/noderef_templates.cpp (+2-2)
- (modified) clang/test/SemaCXX/cxx2b-deducing-this.cpp (+2-4)
- (modified) clang/test/SemaTemplate/class-template-spec.cpp (+6-6)
- (modified) clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp (+3-3)
``diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 49ab222bec405..a2e44efe41347 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -55,6 +55,9 @@ C++ Specific Potentially Breaking Changes
- Clang now rejects pointer to member from parenthesized expression in
unevaluated context such as ``decltype(&(foo::bar))``. (#GH40906).
+- Clang now performs semantic analysis for unary operators with dependent
operands
+ that are known to be of non-class non-enumeration type prior to
instantiation.
+
ABI Changes in This Version
---
- Fixed Microsoft name mangling of implicitly defined variables used for thread
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index e6643469e0b33..da3834f19ca04 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -8044,7 +8044,10 @@ inline bool Type::isUndeducedType() const {
/// Determines whether this is a type for which one can define
/// an overloaded operator.
inline bool Type::isOverloadableType() const {
- return isDependentType() || isRecordType() || isEnumeralType();
+ if (!CanonicalType->isDependentType())
+return isRecordType() || isEnumeralType();
+ return !isArrayType() && !isFunctionType() && !isAnyPointerType() &&
+ !isMemberPointerType();
}
/// Determines whether this type is written as a typedef-name.
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index ec84798e4ce60..50569c1cd5367 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -672,12 +672,12 @@ ExprResult Sema::DefaultLvalueConversion(Expr *E) {
// We don't want to throw lvalue-to-rvalue casts on top of
// expressions of certain types in C++.
- if (getLangOpts().CPlusPlus &&
- (E->getType() == Context.OverloadTy ||
- // FIXME: This is a hack! We want the lvalue-to-rvalue conversion
applied
- // to pointer types even if the pointee type is dependent.
- (T->isDependentType() && !T->isPointerType()) || T->isRecordType()))
-return E;
+ if (getLangOpts().CPlusPlus) {
+if (T == Context.OverloadTy || T->isRecordType() ||
+(T->isDependentType() && !T->isAnyPointerType() &&
+ !T->isMemberPointerType()))
+ return E;
+ }
// The C standard is actually really unclear on this point, and
// DR106 tells us what the result should be but not why. It's
@@ -10827,7 +10827,7 @@ static bool checkArithmeticIncompletePointerType(Sema
, SourceLocation Loc,
if (const AtomicType *ResAtomicType = ResType->getAs())
ResType = ResAtomicType->getValueType();
- assert(ResType->isAnyPointerType() && !ResType->isDependentType());
+ assert(ResType->isAnyPointerType());
QualType PointeeTy = ResType->getPointeeType();
return S.RequireCompleteSizedType(
Loc, PointeeTy,
@@ -13957,9 +13957,6 @@ static QualType CheckIncrementDecrementOperand(Sema ,
Expr *Op,
ExprObjectKind ,
SourceLocation OpLoc, bool
IsInc,
bool IsPrefix) {
- if (Op->isTypeDependent())
-return S.Context.DependentTy;
-
QualType ResType = Op->getType();
// Atomic types can be used for increment / decrement where the non-atomic
// versions can, so ignore the _Atomic() specifier for the purpose of
@@ -14410,9 +14407,6 @@ static void RecordModifiableNonNullParam(Sema , const
[clang] Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" (PR #92283)
https://github.com/sdkrystian created
https://github.com/llvm/llvm-project/pull/92283
This patch reapplies #90500, addressing a bug which caused binary operators
with dependent operands to be incorrectly rebuilt by `TreeTransform`.
>From 365d97508883eb5a4f9b898f8277d16e1f6d3862 Mon Sep 17 00:00:00 2001
From: Krystian Stasiowski
Date: Tue, 14 May 2024 13:23:45 -0400
Subject: [PATCH 1/2] Reapply "[Clang][Sema] Earlier type checking for builtin
unary operators (#90500)"
---
clang/docs/ReleaseNotes.rst | 3 +
clang/include/clang/AST/Type.h| 5 +-
clang/lib/Sema/SemaExpr.cpp | 351 +-
clang/test/AST/ast-dump-expr-json.cpp | 4 +-
clang/test/AST/ast-dump-expr.cpp | 2 +-
clang/test/AST/ast-dump-lambda.cpp| 2 +-
.../expr/expr.unary/expr.unary.general/p1.cpp | 65
clang/test/CXX/over/over.built/ast.cpp| 158 ++--
clang/test/CXX/over/over.built/p10.cpp| 2 +-
clang/test/CXX/over/over.built/p11.cpp| 2 +-
.../temp.res/temp.dep/temp.dep.type/p4.cpp| 25 +-
clang/test/Frontend/noderef_templates.cpp | 4 +-
clang/test/SemaCXX/cxx2b-deducing-this.cpp| 6 +-
.../test/SemaTemplate/class-template-spec.cpp | 12 +-
.../ASTMatchers/ASTMatchersNarrowingTest.cpp | 6 +-
15 files changed, 402 insertions(+), 245 deletions(-)
create mode 100644 clang/test/CXX/expr/expr.unary/expr.unary.general/p1.cpp
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 49ab222bec405..a2e44efe41347 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -55,6 +55,9 @@ C++ Specific Potentially Breaking Changes
- Clang now rejects pointer to member from parenthesized expression in
unevaluated context such as ``decltype(&(foo::bar))``. (#GH40906).
+- Clang now performs semantic analysis for unary operators with dependent
operands
+ that are known to be of non-class non-enumeration type prior to
instantiation.
+
ABI Changes in This Version
---
- Fixed Microsoft name mangling of implicitly defined variables used for thread
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index e6643469e0b33..da3834f19ca04 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -8044,7 +8044,10 @@ inline bool Type::isUndeducedType() const {
/// Determines whether this is a type for which one can define
/// an overloaded operator.
inline bool Type::isOverloadableType() const {
- return isDependentType() || isRecordType() || isEnumeralType();
+ if (!CanonicalType->isDependentType())
+return isRecordType() || isEnumeralType();
+ return !isArrayType() && !isFunctionType() && !isAnyPointerType() &&
+ !isMemberPointerType();
}
/// Determines whether this type is written as a typedef-name.
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index ec84798e4ce60..18fd5ba700ad3 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -672,12 +672,12 @@ ExprResult Sema::DefaultLvalueConversion(Expr *E) {
// We don't want to throw lvalue-to-rvalue casts on top of
// expressions of certain types in C++.
- if (getLangOpts().CPlusPlus &&
- (E->getType() == Context.OverloadTy ||
- // FIXME: This is a hack! We want the lvalue-to-rvalue conversion
applied
- // to pointer types even if the pointee type is dependent.
- (T->isDependentType() && !T->isPointerType()) || T->isRecordType()))
-return E;
+ if (getLangOpts().CPlusPlus) {
+if (T == Context.OverloadTy || T->isRecordType() ||
+(T->isDependentType() && !T->isAnyPointerType() &&
+ !T->isMemberPointerType()))
+ return E;
+ }
// The C standard is actually really unclear on this point, and
// DR106 tells us what the result should be but not why. It's
@@ -10827,7 +10827,7 @@ static bool checkArithmeticIncompletePointerType(Sema
, SourceLocation Loc,
if (const AtomicType *ResAtomicType = ResType->getAs())
ResType = ResAtomicType->getValueType();
- assert(ResType->isAnyPointerType() && !ResType->isDependentType());
+ assert(ResType->isAnyPointerType());
QualType PointeeTy = ResType->getPointeeType();
return S.RequireCompleteSizedType(
Loc, PointeeTy,
@@ -13957,9 +13957,6 @@ static QualType CheckIncrementDecrementOperand(Sema ,
Expr *Op,
ExprObjectKind ,
SourceLocation OpLoc, bool
IsInc,
bool IsPrefix) {
- if (Op->isTypeDependent())
-return S.Context.DependentTy;
-
QualType ResType = Op->getType();
// Atomic types can be used for increment / decrement where the non-atomic
// versions can, so ignore the _Atomic() specifier for the purpose of
@@ -14410,9 +14407,6 @@ static void RecordModifiableNonNullParam(Sema ,
