[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr
kimgr added a comment. Having looked at this some more, I wonder if this patch is covering for another problem. Running even the simplest example fails on an assertion in `Sema::BuildCXXTypeConstructExpr`: https://godbolt.org/z/fMPcsh4f3. It looks like that function is creating the majority of `CXXFunctionalCastExpr`s, so the way the patch shaves off `CXXFunctionalCastExpr` might be compensating for something that should never happen. I don't know much about this, so I can't say for sure. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119095/new/ https://reviews.llvm.org/D119095 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr
kimgr added a comment.
I can also confirm that `main` and my branch (containing
https://reviews.llvm.org/D119477 and https://reviews.llvm.org/D119476) both
assert on the same line:
$ cat bug-53244.cc
struct A {
consteval A() {}
A(const A&) = delete;
consteval void f() {}
};
int main() {
A{A{}}.f();
}
$ bin/clang -std=c++20 ../../bug-53244.cc
clang-14: /home/kimgr/code/llvm-project/clang/lib/Sema/SemaExprCXX.cpp:1453:
clang::ExprResult clang::Sema::BuildCXXTypeConstructExpr(clang::TypeSourceInfo
*, clang::SourceLocation, clang::MultiExprArg, clang::SourceLocation, bool):
Assertion `(!ListInitialization || (Exprs.size() == 1 &&
isa(Exprs[0]))) && "List initialization must have initializer
list as expression."' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/
and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0. Program arguments:
/home/kimgr/code/llvm-project/out/main/bin/clang-14 -cc1 -triple
x86_64-unknown-linux-gnu -emit-obj -mrelax-all --mrelax-relocations
-disable-free -clear-ast-before-backend -main-file-name bug-53244.cc
-mrelocation-model static -mframe-pointer=all -fmath-errno -ffp-contract=on
-fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu x86-64
-tune-cpu generic -mllvm -treat-scalable-fixed-error-as-warning
-debugger-tuning=gdb
-fcoverage-compilation-dir=/home/kimgr/code/llvm-project/out/main -resource-dir
/home/kimgr/code/llvm-project/out/main/lib/clang/15.0.0 -internal-isystem
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9 -internal-isystem
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/x86_64-linux-gnu/c++/9
-internal-isystem
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/backward
-internal-isystem
/home/kimgr/code/llvm-project/out/main/lib/clang/15.0.0/include
-internal-isystem /usr/local/include -internal-isystem
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../x86_64-linux-gnu/include
-internal-externc-isystem /usr/include/x86_64-linux-gnu
-internal-externc-isystem /include -internal-externc-isystem /usr/include
-std=c++20 -fdeprecated-macro
-fdebug-compilation-dir=/home/kimgr/code/llvm-project/out/main -ferror-limit 19
-fgnuc-version=4.2.1 -fno-implicit-modules -fcxx-exceptions -fexceptions
-fcolor-diagnostics -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o
/tmp/bug-53244-192e65.o -x c++ ../../bug-53244.cc
1. parser at end of file
2. ../../bug-53244.cc:7:12: parsing function body 'main'
#0 0x0698e43a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int)
/home/kimgr/code/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:11
#1 0x0698e5eb PrintStackTraceSignalHandler(void*)
/home/kimgr/code/llvm-project/llvm/lib/Support/Unix/Signals.inc:632:1
#2 0x0698cc9a llvm::sys::RunSignalHandlers()
/home/kimgr/code/llvm-project/llvm/lib/Support/Signals.cpp:97:5
#3 0x0698ed15 SignalHandler(int)
/home/kimgr/code/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
#4 0x7f8769c493c0 __restore_rt
(/lib/x86_64-linux-gnu/libpthread.so.0+0x153c0)
#5 0x7f87696dd18b raise
/build/glibc-eX1tMB/glibc-2.31/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
#6 0x7f87696bc859 abort
/build/glibc-eX1tMB/glibc-2.31/stdlib/abort.c:81:7
#7 0x7f87696bc729 get_sysdep_segment_value
/build/glibc-eX1tMB/glibc-2.31/intl/loadmsgcat.c:509:8
#8 0x7f87696bc729 _nl_load_domain
/build/glibc-eX1tMB/glibc-2.31/intl/loadmsgcat.c:970:34
#9 0x7f87696cdf36 (/lib/x86_64-linux-gnu/libc.so.6+0x36f36)
#10 0x0af7d719
clang::Sema::BuildCXXTypeConstructExpr(clang::TypeSourceInfo*,
clang::SourceLocation, llvm::MutableArrayRef,
clang::SourceLocation, bool)
/home/kimgr/code/llvm-project/clang/lib/Sema/SemaExprCXX.cpp:1454:39
#11 0x0ade829d
clang::TreeTransform,
llvm::PointerIntPairInfo >
>*>)::ComplexRemove>::RebuildCXXFunctionalCastExpr(clang::TypeSourceInfo*,
clang::SourceLocation, clang::Expr*, clang::SourceLocation, bool)
/home/kimgr/code/llvm-project/clang/lib/Sema/TreeTransform.h:3019:22
#12 0x0adac3d5
clang::TreeTransform,
llvm::PointerIntPairInfo >
>*>)::ComplexRemove>::TransformCXXFunctionalCastExpr(clang::CXXFunctionalCastExpr*)
/home/kimgr/code/llvm-project/clang/lib/Sema/TreeTransform.h:11728:23
#13 0x0ada4a8f
clang::TreeTransform,
llvm::PointerIntPairInfo >
>*>)::ComplexRemove>::TransformExpr(clang::Expr*)
/home/kimgr/code/llvm-project/out/main/tools/clang/include/clang/AST/StmtNodes.inc:929:1
#14 0x0adcd51c
clang::TreeTransform,
llvm::PointerIntPairInfo >
>*>)::ComplexRemove>::TransformExprs(clang::Expr* const*, unsigned int, bool,
llvm::SmallVectorImpl&, bool*)
/home/kimgr/code/llvm-project/clang/lib/Sema/TreeTransform.h:4047:29
#15 0x0adae285
clang::TreeTransform,
llvm::PointerIntPairInfo >
[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr
kimgr added a comment. First sanity check: applying this patch on my branch causes no test failures either in the `tools/clang/unittests/Tooling/ToolingTests` or `ninja check-clang-semacxx`. Hopefully I can think this through more deeply soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119095/new/ https://reviews.llvm.org/D119095 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr
kimgr added a comment. Thanks for the heads-up! I'm a little busy this week, but I need to think about potential interference between these two changes. Off the top of my head it looks like they should get along fine, but there may be subtleties. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119095/new/ https://reviews.llvm.org/D119095 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr
aaron.ballman added a comment. I haven't had the chance to review this yet (in C standards meetings again this week), but I did notice that https://reviews.llvm.org/D119477 seems to be touching on a related (or possibly the same) issue. Can you coordinate with the other patch author to make sure nobody is writing patches (or reviewing them) that conflict? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119095/new/ https://reviews.llvm.org/D119095 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr
erichkeane added a comment. I think this makes sense to me, I'd like to see it captured in an AST test though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119095/new/ https://reviews.llvm.org/D119095 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr
Izaron added a comment.
There are some godbolt links in the github issues.
Expression `A{}.f();` generates the call of the move constructor from nowhere
as well - https://godbolt.org/z/MceYedKzj
With the patch this node (that lives inside an another `ConstantExpr` node):
MemberExpr 0x5e48b3d8 '' .f 0x5e48aa98
`-MaterializeTemporaryExpr 0x5e48b3c0 'struct A' xvalue
`-CXXFunctionalCastExpr 0x5e48b398 'struct A' functional cast to struct
A
`-ConstantExpr 0x5e48b200 'struct A'
`-CXXTemporaryObjectExpr 0x5e48b1d0 'struct A' 'void (void)' list
transforms to this:
MemberExpr 0x5e48b3d8 '' .f 0x5e48aa98
`-MaterializeTemporaryExpr 0x5e48b3c0 'struct A' xvalue
`-CXXTemporaryObjectExpr 0x5e48b1d0 'struct A' 'void (void)' list
I reviewed other github issues about consteval. This patch doesn't fix other
known issues as I checked.
//P.S. If this review is eventually approved, kindly please merge the commit on
my behalf =) As I don't have merge access. My name is `Evgeny Shulgin` and
email is `izaronpl...@gmail.com`. Sorry for inconvenience!//
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119095/new/
https://reviews.llvm.org/D119095
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr
Izaron created this revision.
Izaron added reviewers: aaron.ballman, cor3ntin.
Izaron requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
When removing nested ConstantExprs, the tree transformer
doesn't remove redundant CXXFunctionalCasts that were created
"above" consteval constructors. After a while it generates a call
to a constructor, therefore violating the C++17 mandatory copy
elision rule.
Fixes https://github.com/llvm/llvm-project/issues/53244
and https://github.com/llvm/llvm-project/issues/53245
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D119095
Files:
clang/lib/Sema/SemaExpr.cpp
clang/test/SemaCXX/cxx2a-consteval.cpp
Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -613,6 +613,45 @@
} // namespace unevaluated
+namespace mandatory_copy_elision {
+
+struct A {
+ consteval A() {}
+ consteval A(const A &);
+ consteval A(A &&);
+ consteval void f() {}
+};
+
+struct B {
+ consteval B() {}
+ consteval B(const B &) = delete;
+ consteval B(B &&) = delete;
+ consteval void f() {}
+};
+
+struct C {
+ consteval C() {}
+ consteval void f() {}
+
+private:
+ consteval C(const C &){};
+ consteval C(C &&){};
+};
+
+void test() {
+ { A{}.f(); }
+ { A{A{}}.f(); }
+ { A{A{A{A{A{A{A{A{.f(); }
+ { B{}.f(); }
+ { B{B{}}.f(); }
+ { B{B{B{B{B{B{B{B{.f(); }
+ { C{}.f(); }
+ { C{C{}}.f(); }
+ { C{C{C{C{C{C{C{C{.f(); }
+}
+
+} // namespace mandatory_copy_elision
+
namespace PR50779 {
struct derp {
int b = 0;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16796,6 +16796,14 @@
DRSet.erase(cast(E->getCallee()->IgnoreImplicit()));
return Base::TransformCXXOperatorCallExpr(E);
}
+/// Delete extra no-op functional casts to avoid calling a constructor
+ExprResult TransformCXXFunctionalCastExpr(CXXFunctionalCastExpr *E) {
+ auto *CE = dyn_cast(E->getSubExpr());
+ if (E->getCastKind() != CK_NoOp || !CE || !CE->isImmediateInvocation())
+return Base::TransformCXXFunctionalCastExpr(E);
+ RemoveImmediateInvocation(CE);
+ return Base::TransformExpr(CE->getSubExpr());
+}
/// Base::TransformInitializer skip ConstantExpr so we need to visit them
/// here.
ExprResult TransformInitializer(Expr *Init, bool NotCopyInit) {
Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -613,6 +613,45 @@
} // namespace unevaluated
+namespace mandatory_copy_elision {
+
+struct A {
+ consteval A() {}
+ consteval A(const A &);
+ consteval A(A &&);
+ consteval void f() {}
+};
+
+struct B {
+ consteval B() {}
+ consteval B(const B &) = delete;
+ consteval B(B &&) = delete;
+ consteval void f() {}
+};
+
+struct C {
+ consteval C() {}
+ consteval void f() {}
+
+private:
+ consteval C(const C &){};
+ consteval C(C &&){};
+};
+
+void test() {
+ { A{}.f(); }
+ { A{A{}}.f(); }
+ { A{A{A{A{A{A{A{A{.f(); }
+ { B{}.f(); }
+ { B{B{}}.f(); }
+ { B{B{B{B{B{B{B{B{.f(); }
+ { C{}.f(); }
+ { C{C{}}.f(); }
+ { C{C{C{C{C{C{C{C{.f(); }
+}
+
+} // namespace mandatory_copy_elision
+
namespace PR50779 {
struct derp {
int b = 0;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16796,6 +16796,14 @@
DRSet.erase(cast(E->getCallee()->IgnoreImplicit()));
return Base::TransformCXXOperatorCallExpr(E);
}
+/// Delete extra no-op functional casts to avoid calling a constructor
+ExprResult TransformCXXFunctionalCastExpr(CXXFunctionalCastExpr *E) {
+ auto *CE = dyn_cast(E->getSubExpr());
+ if (E->getCastKind() != CK_NoOp || !CE || !CE->isImmediateInvocation())
+return Base::TransformCXXFunctionalCastExpr(E);
+ RemoveImmediateInvocation(CE);
+ return Base::TransformExpr(CE->getSubExpr());
+}
/// Base::TransformInitializer skip ConstantExpr so we need to visit them
/// here.
ExprResult TransformInitializer(Expr *Init, bool NotCopyInit) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
