This is an archive of the discontinued LLVM Phabricator instance.

[clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values
ClosedPublic

Authored by ayzhao on Jul 11 2022, 6:24 PM.

Details

Summary

This patch implements P0960R3, which allows initialization of aggregates
via parentheses.

As an example:

struct S { int i, j; };
S s1(1, 1);

int arr1[2](1, 2);

This patch also implements P1975R0, which fixes the wording of P0960R3
for single-argument parenthesized lists so that statements like the
following are allowed:

S s2(1);
S s3 = static_cast<S>(1);
S s4 = (S)1;

int (&&arr2)[] = static_cast<int[]>(1);
int (&&arr3)[2] = static_cast<int[2]>(1);

This patch was originally authored by @0x59616e and completed by
@ayzhao.

Fixes #54040, Fixes #54041

Co-authored-by: Sheng <ox59616e@gmail.com>

Full write up : https://discourse.llvm.org/t/c-20-rfc-suggestion-desired-regarding-the-implementation-of-p0960r3/63744

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
shafik added inline comments.Oct 24 2022, 1:52 PM
clang/include/clang/AST/ExprCXX.h
4779

Should we prefer makeArrayRef?

clang/lib/CodeGen/CGExprAgg.cpp
1618
clang/lib/Sema/SemaInit.cpp
5381

I feel like this function could use a lot of comments, I found it hard to follow the flow of logic and I think I get it mostly now but comments would have helped a lot.

5391

Or some other clarifying comment.

5428
ayzhao marked 11 inline comments as done.Oct 24 2022, 4:00 PM

Thanks for working on it! It looks really good. Please remember to update the feature test macro (__cpp_aggregate_paren_init).

Done

Also, I think there's no test coverage for the ASTReader/Writer changes? I would like to see some as well.

I added a precompiled header test. Currently, the ASTReader/Writer change is broken with the following error:

                                                                                                                         "hunter-bidens-laptop." 15:58 24-Oct-22
$ bin/llvm-lit -vv --debug ../clang/test/PCH/cxx_paren_init.cpp
[553/553] Creating executable symlink bin/clang
llvm-lit: /llvm-project/llvm/utils/lit/lit/discovery.py:62: note: loading suite config '/llvm-project/build/tools/clang/test/lit.site.cfg.py'
llvm-lit: /llvm-project/llvm/utils/lit/lit/LitConfig.py:116: note: load_config from '/llvm-project/clang/test/lit.cfg.py'
llvm-lit: /llvm-project/llvm/utils/lit/lit/llvm/config.py:456: note: using clang: /llvm-project/build/bin/clang
llvm-lit: /llvm-project/llvm/utils/lit/lit/TestingConfig.py:129: note: ... loaded config '/llvm-project/clang/test/lit.cfg.py'
llvm-lit: /llvm-project/llvm/utils/lit/lit/TestingConfig.py:129: note: ... loaded config '/llvm-project/build/tools/clang/test/lit.site.cfg.py'
llvm-lit: /llvm-project/llvm/utils/lit/lit/discovery.py:136: note: resolved input '../clang/test/PCH/cxx_paren_init.cpp' to 'Clang'::('PCH', 'cxx_paren_init.cpp')
-- Testing: 1 tests, 1 workers --
FAIL: Clang :: PCH/cxx_paren_init.cpp (1 of 1)
******************** TEST 'Clang :: PCH/cxx_paren_init.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /llvm-project/build/bin/clang -cc1 -internal-isystem /llvm-project/build/lib/clang/16.0.0/include -nostdsysteminc -x c++ -std=c++20 -emit-pc
h -o /llvm-project/build/tools/clang/test/PCH/Output/cxx_paren_init.cpp.tmp /llvm-project/clang/test/PCH/cxx_paren_init.h
: 'RUN: at line 2';   /llvm-project/build/bin/clang -cc1 -internal-isystem /llvm-project/build/lib/clang/16.0.0/include -nostdsysteminc -x c++ -std=c++20 -include
-pch /llvm-project/build/tools/clang/test/PCH/Output/cxx_paren_init.cpp.tmp -fsyntax-only -S -o -
--
Exit Code: 139

Command Output (stderr):
--
+ : 'RUN: at line 1'
+ /llvm-project/build/bin/clang -cc1 -internal-isystem /llvm-project/build/lib/clang/16.0.0/include -nostdsysteminc -x c++ -std=c++20 -emit-pch -o /dev/shm/ayzhao
_llvm/llvm-project/build/tools/clang/test/PCH/Output/cxx_paren_init.cpp.tmp /llvm-project/clang/test/PCH/cxx_paren_init.h
+ : 'RUN: at line 2'
+ /llvm-project/build/bin/clang -cc1 -internal-isystem /llvm-project/build/lib/clang/16.0.0/include -nostdsysteminc -x c++ -std=c++20 -include-pch /dev/shm/ayzhao
_llvm/llvm-project/build/tools/clang/test/PCH/Output/cxx_paren_init.cpp.tmp -fsyntax-only -S -o -
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: /llvm-project/build/bin/clang -cc1 -internal-isystem /llvm-project/build/lib/clang/16.0.0/include -nostdsysteminc -x c++ -std=c++20 -in
clude-pch /llvm-project/build/tools/clang/test/PCH/Output/cxx_paren_init.cpp.tmp -fsyntax-only -S -o -
1.      <eof> parser at end of file
2.      /llvm-project/clang/test/PCH/cxx_paren_init.h:4:3: LLVM IR generation of declaration 'foo'
3.      /llvm-project/clang/test/PCH/cxx_paren_init.h:4:3: Generating code for declaration 'foo'
 #0 0x000055ac3d88eeea llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /src/llvm_tmpfs/llvm-project/llvm/lib/Support/Unix/Signals.inc:569:11
 #1 0x000055ac3d88f09b PrintStackTraceSignalHandler(void*) /src/llvm_tmpfs/llvm-project/llvm/lib/Support/Unix/Signals.inc:636:1
 #2 0x000055ac3d88d6e6 llvm::sys::RunSignalHandlers() /src/llvm_tmpfs/llvm-project/llvm/lib/Support/Signals.cpp:104:5
 #3 0x000055ac3d88f7c5 SignalHandler(int) /src/llvm_tmpfs/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
 #4 0x00007fbe3c83daf0 (/lib/x86_64-linux-gnu/libc.so.6+0x3daf0)
 #5 0x000055ac3dd9e32c clang::Stmt::getStmtClass() const /src/llvm_tmpfs/llvm-project/clang/include/clang/AST/Stmt.h:1166:44
 #6 0x000055ac3e40c735 clang::ImplicitValueInitExpr::classof(clang::Stmt const*) /src/llvm_tmpfs/llvm-project/clang/include/clang/AST/Expr.h:5527:30
 #7 0x000055ac3e40c715 llvm::isa_impl<clang::ImplicitValueInitExpr, clang::Expr, void>::doit(clang::Expr const&) /src/llvm_tmpfs/llvm-project/llvm/include/llvm/Support/Casting.h:64:46
 #8 0x000055ac3e40c6f3 llvm::isa_impl_cl<clang::ImplicitValueInitExpr, clang::Expr const*>::doit(clang::Expr const*) /src/llvm_tmpfs/llvm-project/llvm/include/llvm/Support/Casting.h:110:5
 #9 0x000055ac3e40c678 llvm::isa_impl_wrap<clang::ImplicitValueInitExpr, clang::Expr const*, clang::Expr const*>::doit(clang::Expr const* const&) /src/llvm_tmpfs/llvm-project/llvm/include/llvm/Support/Casting.h:137:5
#10 0x000055ac3e40c652 llvm::isa_impl_wrap<clang::ImplicitValueInitExpr, clang::Expr const* const, clang::Expr const*>::doit(clang::Expr const* const&) /src/llvm_tmpfs/llvm-project/llvm/include/llvm/Support/Casting.h:127:5
#11 0x000055ac3e40c625 llvm::CastIsPossible<clang::ImplicitValueInitExpr, clang::Expr const*, void>::isPossible(clang::Expr const* const&) /src/llvm_tmpfs/llvm-project/llvm/include/llvm/Support/Casting.h:255:5
#12 0x000055ac3e40d3d2 llvm::CastInfo<clang::ImplicitValueInitExpr, clang::Expr* const, void>::isPossible(clang::Expr* const&) /src/llvm_tmpfs/llvm-project/llvm/include/llvm/Support/Casting.h:510:5
#13 0x000055ac3e40d385 bool llvm::isa<clang::ImplicitValueInitExpr, clang::Expr*>(clang::Expr* const&) /src/llvm_tmpfs/llvm-project/llvm/include/llvm/Support/Casting.h:549:3
#14 0x000055ac3e4088ac (anonymous namespace)::AggExprEmitter::EmitInitializationToLValue(clang::Expr*, clang::CodeGen::LValue) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CGExprAgg.cpp:1536:44
#15 0x000055ac3e402d1e (anonymous namespace)::AggExprEmitter::VisitCXXParenListInitExpr(clang::CXXParenListInitExpr*) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CGExprAgg.cpp:1628:5
#16 0x000055ac3e3fe04d clang::StmtVisitorBase<std::add_pointer, (anonymous namespace)::AggExprEmitter, void>::Visit(clang::Stmt*) /src/llvm_tmpfs/llvm-project/build/tools/clang/include/clang/AST/StmtNodes.inc:877:1
#17 0x000055ac3e3fba35 (anonymous namespace)::AggExprEmitter::Visit(clang::Expr*) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CGExprAgg.cpp:108:3
#18 0x000055ac3e40a9ed (anonymous namespace)::AggExprEmitter::VisitCastExpr(clang::CastExpr*) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CGExprAgg.cpp:864:5
#19 0x000055ac3e40992d clang::StmtVisitorBase<std::add_pointer, (anonymous namespace)::AggExprEmitter, void>::VisitExplicitCastExpr(clang::ExplicitCastExpr*) /src/llvm_tmpfs/llvm-project/build/tools/clang/include/clang/AST/StmtNodes.inc:973:1
#20 0x000055ac3e403c1d clang::StmtVisitorBase<std::add_pointer, (anonymous namespace)::AggExprEmitter, void>::VisitCXXFunctionalCastExpr(clang::CXXFunctionalCastExpr*) /src/llvm_tmpfs/llvm-project/build/tools/clang/include/clang/AST/StmtNodes.inc:989:1
#21 0x000055ac3e3fe17f clang::StmtVisitorBase<std::add_pointer, (anonymous namespace)::AggExprEmitter, void>::Visit(clang::Stmt*) /src/llvm_tmpfs/llvm-project/build/tools/clang/include/clang/AST/StmtNodes.inc:989:1
#22 0x000055ac3e3fba35 (anonymous namespace)::AggExprEmitter::Visit(clang::Expr*) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CGExprAgg.cpp:108:3
#23 0x000055ac3e3fb655 clang::CodeGen::CodeGenFunction::EmitAggExpr(clang::Expr const*, clang::CodeGen::AggValueSlot) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CGExprAgg.cpp:2046:1
#24 0x000055ac3dfab89b clang::CodeGen::CodeGenFunction::EmitReturnStmt(clang::ReturnStmt const&) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CGStmt.cpp:0:7
#25 0x000055ac3dfa8491 clang::CodeGen::CodeGenFunction::EmitStmt(clang::Stmt const*, llvm::ArrayRef<clang::Attr const*>) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CGStmt.cpp:153:75
#26 0x000055ac3dfb1e5f clang::CodeGen::CodeGenFunction::EmitCompoundStmtWithoutScope(clang::CompoundStmt const&, bool, clang::CodeGen::AggValueSlot) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CGStmt.cpp:496:3
#27 0x000055ac3df8295d clang::CodeGen::CodeGenFunction::EmitFunctionBody(clang::Stmt const*) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CodeGenFunction.cpp:1239:5
#28 0x000055ac3df83732 clang::CodeGen::CodeGenFunction::GenerateCode(clang::GlobalDecl, llvm::Function*, clang::CodeGen::CGFunctionInfo const&) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CodeGenFunction.cpp:1448:3
#29 0x000055ac3de1cd02 clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(clang::GlobalDecl, llvm::GlobalValue*) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:5321:3
#30 0x000055ac3de13639 clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, llvm::GlobalValue*) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:3596:12
#31 0x000055ac3de18b3a clang::CodeGen::CodeGenModule::EmitGlobal(clang::GlobalDecl) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:3337:5
#32 0x000055ac3de11d50 clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:6180:5
#33 0x000055ac3ef4e7b0 (anonymous namespace)::CodeGeneratorImpl::HandleTopLevelDecl(clang::DeclGroupRef) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/ModuleBuilder.cpp:188:73
#34 0x000055ac3ef470e0 clang::BackendConsumer::HandleTopLevelDecl(clang::DeclGroupRef) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:232:12
#35 0x000055ac3ef4724b clang::BackendConsumer::HandleInterestingDecl(clang::DeclGroupRef) /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:260:5
#36 0x000055ac3f013cc3 clang::ASTReader::PassInterestingDeclToConsumer(clang::Decl*) /src/llvm_tmpfs/llvm-project/clang/lib/Serialization/ASTReader.cpp:7740:1
#37 0x000055ac3f0ebeba clang::ASTReader::PassInterestingDeclsToConsumer() /src/llvm_tmpfs/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:3965:3
#38 0x000055ac3f013e27 clang::ASTReader::StartTranslationUnit(clang::ASTConsumer*) /src/llvm_tmpfs/llvm-project/clang/lib/Serialization/ASTReader.cpp:0:5
#39 0x000055ac417efb74 clang::ParseAST(clang::Sema&, bool, bool) /src/llvm_tmpfs/llvm-project/clang/lib/Parse/ParseAST.cpp:151:20
#40 0x000055ac3ed606ac clang::ASTFrontendAction::ExecuteAction() /src/llvm_tmpfs/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1164:1
#41 0x000055ac3ef432f4 clang::CodeGenAction::ExecuteAction() /src/llvm_tmpfs/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:1145:5
#42 0x000055ac3ed600ac clang::FrontendAction::Execute() /src/llvm_tmpfs/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1059:7
#43 0x000055ac3ec8f16c clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /src/llvm_tmpfs/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:1043:23
#44 0x000055ac3ef2c007 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /src/llvm_tmpfs/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:8
#45 0x000055ac3b82a5e0 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /src/llvm_tmpfs/llvm-project/clang/tools/driver/cc1_main.cpp:250:13
#46 0x000055ac3b81c49c ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) /src/llvm_tmpfs/llvm-project/clang/tools/driver/driver.cpp:316:5
#47 0x000055ac3b81b325 clang_main(int, char**) /src/llvm_tmpfs/llvm-project/clang/tools/driver/driver.cpp:388:5
#48 0x000055ac3b84a8b2 main /src/llvm_tmpfs/llvm-project/build/tools/clang/tools/driver/clang-driver.cpp:11:35
#49 0x00007fbe3c82920a __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#50 0x00007fbe3c8292bc call_init ./csu/../csu/libc-start.c:128:20
#51 0x00007fbe3c8292bc __libc_start_main ./csu/../csu/libc-start.c:376:5
#52 0x000055ac3b81aba1 _start (/llvm-project/build/bin/clang+0x54d9ba1)
/llvm-project/build/tools/clang/test/PCH/Output/cxx_paren_init.cpp.script: line 2: 1747518 Segmentation fault      /llvm-project/build/bin/clang -cc1 -internal-is
ystem /llvm-project/build/lib/clang/16.0.0/include -nostdsysteminc -x c++ -std=c++20 -include-pch /llvm-project/build/tools/clang/test/PCH/Output/cxx_paren_init.c
pp.tmp -fsyntax-only -S -o -

--

********************
********************
Failed Tests (1):
  Clang :: PCH/cxx_paren_init.cpp


Testing Time: 48.40s
  Failed: 1

I'm currently looking into this - I suspect it has to do with CXXParenListInitExpr storing all it's subexpressions in an llvm::TrailingObjects base class, but I have yet to confirm one way or another.

ayzhao updated this revision to Diff 470311.Oct 24 2022, 4:03 PM

some comment fixes, add a test for serialization which doesn't work yet

ayzhao updated this revision to Diff 470314.Oct 24 2022, 4:14 PM

remove whitespace change and disable test

ilya-biryukov added inline comments.
clang/lib/Serialization/ASTReaderStmt.cpp
2175

FYI: I think this is where the crash comes from.
We should allocate the trailing objects first.
E.g. see how PragmaCommentDecl::CreateDeserialized does this.

ayzhao added inline comments.Oct 25 2022, 3:58 PM
clang/lib/Serialization/ASTReaderStmt.cpp
2175

This sounds like it could be the solution - thanks for looking at it!

Currently, I'm working on the refactor that shafik@ suggested, which was to inherit from InitListExpr. Hopefully, that refactor will fix this issue as InitListExpr stores it's subexpressions in a class member instead of using llvm::TrailingObjects.

ilya-biryukov added inline comments.Oct 26 2022, 2:24 AM
clang/lib/Serialization/ASTReaderStmt.cpp
2175

Are we trying to share code between two implementations? If so, I suggest to consider alternatives, e.g. creating a new base class and inheriting both InitListExpr and CXXParentInitListExpr to share the common code.

Inheriting CXXParentInitListExpr from InitListExpr breaks Liskov substitution principle and will likely to lead to bugs that are hard to chase. InitListExpr is widely used and means`{}. CXXParenInitListExpr` is not {} and we do not know in advance which code is going to work for both and which code is only valid for {}. Reviewing all callsites that use InitListExpr does not seem plausible. Note that in addition to Clang, there are also uses in ast-matchers and in clang-tidy checks (quite a few of those are downstream).

ayzhao updated this revision to Diff 471329.Oct 27 2022, 4:50 PM
ayzhao marked 11 inline comments as done.

Fix PCH test and address some code review comments

Thanks for working on it! It looks really good. Please remember to update the feature test macro (__cpp_aggregate_paren_init).

Done

Also, I think there's no test coverage for the ASTReader/Writer changes? I would like to see some as well.

I added a precompiled header test. Currently, the ASTReader/Writer change is broken with the following error:

...

I'm currently looking into this - I suspect it has to do with CXXParenListInitExpr storing all it's subexpressions in an llvm::TrailingObjects base class, but I have yet to confirm one way or another.

The PCH test is fixed now, so we now have coverage for the ASTReader/Writer changes.

clang/lib/AST/Expr.cpp
3782

See the response from @ilya-biryukov for why I decided against inheriting from InitListExpr.

clang/lib/AST/ExprClassification.cpp
446

IMO; no.

I believe the comment for InitListExpr refers to statements like:

int a;
int &b{a};

This doesn't apply to CXXParenListInitExprClass because the corresponding statement with parentheses wouldn't be parsed as a CXXParenListInitExpr in the first place.

clang/lib/AST/StmtPrinter.cpp
2469

See the response from @ilya-biryukov for why I decided against inheriting from InitListExpr.

clang/lib/Sema/TreeTransform.h
13960

This is an extra change, but clang-format would always make it.

clang/lib/Serialization/ASTReaderStmt.cpp
2175

So I figured out why this was failing. When I created the CXXParenListInitExpr object on the heap in ASTReader::ReadStmtFromStream(...) below, I just called new without allocating any space for the TrailingObjects. This is fixed now, and the PCH test passes.

Inheriting CXXParentInitListExpr from InitListExpr breaks Liskov substitution principle and will likely to lead to bugs that are hard to chase. InitListExpr is widely used and means`{}. CXXParenInitListExpr` is not {} and we do not know in advance which code is going to work for both and which code is only valid for {}. Reviewing all callsites that use InitListExpr does not seem plausible. Note that in addition to Clang, there are also uses in ast-matchers and in clang-tidy checks (quite a few of those are downstream).

+1, I am convinced that inheriting from InitListExpr is the wrong thing to do here. Additionally, when I tried to inherit from InitListExpr, I found that there were a lot of things in that class that were specific to InitListExpr that I had to wrap with an if statement to make it work with CXXParenListInitExpr, and that the amount of code that I had to add considerably decreases readability.

ayzhao updated this revision to Diff 471338.Oct 27 2022, 5:27 PM

we don't need CHECK-DAG here

ayzhao updated this revision to Diff 471694.Oct 28 2022, 5:59 PM
ayzhao marked 2 inline comments as done.

add C++20 extension diagnostics and some other code review feedback

Friendly ping for reviewers as all comments should be addressed by now.

It starting to look great.
Should we add an extension warning? Note that I'm not suggesting to support that in earlier modes, just an off-by-default warning that says "this is a c++20 feature". but we are a bit inconsistent with those.

Done

can you rename test/SemaCXX/P0960R3.cpp to something like test/SemaCXX/cxx20-paren-list-init.cpp (and same for other tests?) it's more consistent with other existing tests.

Done

clang/lib/Sema/SemaInit.cpp
5381

I added some comments, did a little refactoring, and renamed some variables. PTAL and LMKWYT.

I added a few more comments, mostly nitpicks

clang/lib/AST/ExprConstant.cpp
9967–9968
9971

Maybe that message could be a bit more clear.

"Union should have exactly 1 initializer in in C++ paren list" or something like that.

clang/lib/CodeGen/CGExprAgg.cpp
1595

There are some overlaps with VisitInitListExpr, I wonder if we should try to factor out some of the code, especially the base class initialization

1600

Maybe it would be cleaner to use a structured binding here.

1616

Should we assert that the base is not virtual?

clang/lib/Sema/SemaInit.cpp
5400
clang/lib/Sema/TreeTransform.h
13968

ArgumentChanged is not used, and is defaulted by the function, so you should be able to remove it

ayzhao updated this revision to Diff 472190.Oct 31 2022, 5:33 PM
ayzhao marked 6 inline comments as done.

address some comments and implement disallowing flexible array members

Status update:

While investigating @cor3ntin's comment about refactoring VisitInitListExpr and VisitCXXParenListInitExpr as they share common code, I discovered that flexible array members would cause this patch to explode. Specifically, the following code:

struct S {
    int b;
    int a[];
};

S s2(1, {1, 2});

was causing this assertion to fail.

I took a look at GCC, and it turns out that GCC doesn't allow flexible array members at all in paren list initialization of aggregates, even in contexts where the corresponding designated initializer expression succeeds: https://godbolt.org/z/E73433erb

For now, I updated this implementation to follow GCC's behavior and disallow flexible array members, as IIRC in C++ they are a GCC extension and we should therefore follow GCC behavior. However, I am open to other thoughts.

ayzhao updated this revision to Diff 472405.Nov 1 2022, 2:45 PM

clang-format + rebase

ayzhao updated this revision to Diff 472441.Nov 1 2022, 4:11 PM
ayzhao marked an inline comment as done.

Merge CXXParenListInitExpr with VisitInitListExpr, also friendly ping as all comments are addressed

I think this is basically good to go, but I'll let other people give it a look too

clang/lib/AST/ExprConstant.cpp
9980–9983
clang/lib/CodeGen/CGExprAgg.cpp
92

Could you give E a better name?

478

Ditto

ayzhao updated this revision to Diff 472702.Nov 2 2022, 11:08 AM
ayzhao marked 3 inline comments as done.

addressed code review comments

ayzhao updated this revision to Diff 473746.Nov 7 2022, 10:51 AM

rebase + update commit message

ayzhao retitled this revision from [clang][C++20] P0960R3: Allow initializing aggregates from a parenthesized list of values to [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values.Nov 7 2022, 10:53 AM
ayzhao edited the summary of this revision. (Show Details)

Friendly ping, does anyone else have any more comments?

ayzhao updated this revision to Diff 473772.Nov 7 2022, 1:35 PM

whoops, uploaded the wrong commit. should be fixed now

ayzhao updated this revision to Diff 473779.Nov 7 2022, 1:44 PM

remove extra parens

ayzhao edited the summary of this revision. (Show Details)Nov 7 2022, 1:45 PM
ayzhao updated this revision to Diff 473789.Nov 7 2022, 2:08 PM

s/pro20/post20/g

ayzhao edited the summary of this revision. (Show Details)Nov 7 2022, 4:06 PM
danakj added a subscriber: danakj.Nov 7 2022, 5:28 PM

There is a C++ standard meeting this week so some folks won't be able to review it. Feel free to re-ping next week if nobody else replies.
Because it's a fairly big change and I've been known to miss glaring things, I don't feel comfortable to approve it. But it looks great to me.
Either way we need to make sure this ships in Clang 16.

I am in the committee meeting this week but if you ping next week I should have time.

Sorry for not reviewing this sooner. This looks very good overall, but I still have some NITs and a few major comments. For the major points see:

  • the comment about the filler and the syntactic/semantic form for the newly added expression,
  • the comment about relying on getFailedCandidateSet().size().
clang/lib/AST/ExprCXX.cpp
1769

NIT: add newline

clang/lib/AST/ExprConstant.cpp
9980

NIT: maybe add braces as the other branches have them? This seems to be in the style guide:

readability is also harmed if an if/else chain does not use braced bodies for either all or none of its members

10931

Could we add an assertion that the array size and the InitExprs.size() are the same?
E.g. it can differ in the case of InitListExpr (probably due to the filler optimization?), it would be nice to add a sanity check here.

clang/lib/AST/JSONNodeDumper.cpp
852

NIT: maybe use the same formatting as other switch cases for constistency?

clang/lib/CodeGen/CGExprAgg.cpp
478

NIT: maybe add a comment that ExprToVisit is either InitListExpr or CXXParenInitListExpr?

577
  • Should we have a filler for CXXParenInitListExpr too? It seems like an important optimization and could have large effect on compile times if we don't
  • Same question for semantic and syntactic-form (similar to InitListExpr): should we have it here? I am not sure if it's semantically required (probably not?), but that's definitely something that clang-tidy and other source tools will rely on.

We should probably share the code there, I suggest moving it to a shared base class and using it where appropriate instead of the derived classes.

clang/lib/Sema/SemaInit.cpp
6097

It seems shaky to rely on the size of getFailedCandidateSet.
What are we trying to achieve here? Is there a more direct way to specify it?

6116

NIT: maybe move the full comment into the body of the function?
It describes in detail what the body of the function does and it would be easier to understand whether the code fits the intention in the standard.
Having the first part of the comment here would still be useful, probably.

clang/lib/Serialization/ASTReaderStmt.cpp
2174

Could we assign the CXXParenInitListExpr.NumExprs on construction and add the assertion sanity-checking the numbers here:

unsigned NumExprs = Record.readInt();
(void)NumExprs; // to avoid unused warnings in NDEBUG builds.
assert(NumExprs == E->NumExprs);

(Looked up how DesignatedInitExpr does a similar thing, probably good for consistency in addition to added safety)

ayzhao updated this revision to Diff 476625.Nov 18 2022, 4:01 PM
ayzhao marked 9 inline comments as done.

address code review comments

clang/lib/AST/JSONNodeDumper.cpp
852

Unfortunately clang-format insists that these be on separate lines.

clang/lib/CodeGen/CGExprAgg.cpp
577

Should we have a filler for CXXParenInitListExpr too? It seems like an important optimization and could have large effect on compile times if we don't

This feels like premature optimization - presumably, wouldn't this only be an issue with extraordinarily large (say, O(1000)) arrays?

Same question for semantic and syntactic-form (similar to InitListExpr): should we have it here? I am not sure if it's semantically required (probably not?), but that's definitely something that clang-tidy and other source tools will rely on

IIRC this doesn't apply to paren list aggregate expressions, as the syntatic form would be the enclosing ParenListExpr.

clang/lib/Sema/SemaInit.cpp
6097

Done.

As mentioned in the above comment, we try to parse this as a CXXParenListInit if the only failed overload constructor candidates are the default constructor, the default copy constructor, and the default move constructor).

6116

Done. Most of this comment is spread out throughout the body of the function anyways.

ychen added a subscriber: ychen.Nov 27 2022, 10:12 PM

Thanks for addressing the comments and sorry for a wait before the comments.
Please see the comment about syntactic form, I might be holding it wrong, but it looks like we actually loose the syntactic form completely in this case.

clang/lib/AST/JSONNodeDumper.cpp
852

Ah, that's unfortunate. I normally just revert the effect of clang-format for those lines, but up to you.
Also fine to keep as is or even format the other lines according to the style guide (it's just 3 more lines, so should not be a big deal).

clang/lib/CodeGen/CGExprAgg.cpp
577

This feels like premature optimization - presumably, wouldn't this only be an issue with extraordinarily large (say, O(1000)) arrays?

Yes, this should only happen with large arrays. Normally I would agree, but it's surprising that changing {} to () in the compiler would lead to performance degradation.
In that sense, this premature optimization is already implemented, we are rather degrading performance for a different syntax to do the same thing.

Although we could also land without it, but in that case could you open a GH issue and add a FIXME to track the implementation of this particular optimization?
This should increase the chances of users finding the root cause of the issue if they happen to hit it.

IIRC this doesn't apply to paren list aggregate expressions, as the syntatic form would be the enclosing ParenListExpr.

Do we even have the enclosing ParenListExpr? If I dump the AST with clang -fsyntax-only -Xclang=-ast-dump ... for the following code:

struct pair { int a; int b = 2; };
int main() {
  pair(1); pair p(1);
  pair b{1}; pair{1};
}

I get

`-FunctionDecl 0x557d79717e98 <line:2:1, line:5:1> line:2:5 main 'int ()'
  `-CompoundStmt 0x557d797369d0 <col:12, line:5:1>
    |-CXXFunctionalCastExpr 0x557d79718528 <line:3:3, col:9> 'pair':'pair' functional cast to pair <NoOp>
    | `-CXXParenListInitExpr 0x557d79718500 <col:3> 'pair':'pair'
    |   |-IntegerLiteral 0x557d79718010 <col:8> 'int' 1
    |   `-IntegerLiteral 0x557d79717e18 <line:1:30> 'int' 2
    |-DeclStmt 0x557d79718650 <line:3:12, col:21>
    | `-VarDecl 0x557d79718568 <col:12, col:17> col:17 p 'pair':'pair' parenlistinit
    |   `-CXXParenListInitExpr 0x557d79718610 <col:17> 'pair':'pair'
    |     |-IntegerLiteral 0x557d797185d0 <col:19> 'int' 1
    |     `-IntegerLiteral 0x557d79717e18 <line:1:30> 'int' 2
    |-DeclStmt 0x557d797187d8 <line:4:3, col:12>
    | `-VarDecl 0x557d79718680 <col:3, col:11> col:8 b 'pair':'pair' listinit
    |   `-InitListExpr 0x557d79718750 <col:9, col:11> 'pair':'pair'
    |     |-IntegerLiteral 0x557d797186e8 <col:10> 'int' 1
    |     `-CXXDefaultInitExpr 0x557d797187a0 <col:11> 'int'
    `-CXXFunctionalCastExpr 0x557d797369a8 <col:14, col:20> 'pair':'pair' functional cast to pair <NoOp>
      `-InitListExpr 0x557d79718868 <col:18, col:20> 'pair':'pair'
        |-IntegerLiteral 0x557d79718800 <col:19> 'int' 1
        `-CXXDefaultInitExpr 0x557d797188b8 <col:20> 'int'

It feels like the ParentListExpr is replaced during semantic analysis and there is no way to get it back. I also tried running clang-query and trying to match parenListExpr() and go 0 results. Is it just missing in the AST dump and I run clang-query incorrectly or do we actually not have the syntactic form of this expression after all?

ilya-biryukov added inline comments.Nov 28 2022, 8:05 AM
clang/lib/CodeGen/CGExprAgg.cpp
577

Another important thing to address from the dump: notice how braced initialization creates CXXDefaultInitExpr and CXXParenListInitExpr copies the default argument expression directly. It's really important to use the former form, here's the example that currently breaks:

#include <iostream>

struct func_init { int some_int; const char* fn = __builtin_FUNCTION(); };

int main() {
  func_init a(10);
  std::cout << "From paren init:" << a.fn << std::endl;

  func_init b{10};
  std::cout << "From braced init: " << b.fn << std::endl;
}

The program above is expected to report main for both cases, but instead we get:

From paren init:
From braced init: main
ayzhao updated this revision to Diff 479758.Dec 2 2022, 3:15 PM
ayzhao marked 2 inline comments as done.

implement CXXDefaultInitExpr and ImplicitValueInitExpr, array fillers, and semantic vs syntatic forms.

clang/lib/CodeGen/CGExprAgg.cpp
577

The following have now been implemented:

  • CXXDefaultInitExpr and ImplicitValueInitExpr, which includes adding a test for __builtin_FUNCTION()
  • Array fillers
  • Semantic forms vs syntactic forms

Thanks!

I have two major comments and also inline NITs. Not sure if we should block on those, just wanted to hear your opinions:

  • InitListExpr and CXXParenInitListExpr have some code in common. Code duplication is substantial and I think sharing the common implementation is warranted. E.g. if some code would want to change something with ArrayFiller or make a use of it, the work will probably need to be duplicated. To reiterate what I mentioned earlier, I believe deriving CXXParenInitListExpr from InitListExpr is not a very good option, but we might explore other possibilities: a base class or factoring out common code in a separate struct. I believe this could be done with a follow-up change, though, as the amount of changes required does not seem too big, I would be happy with first landing this patch and then cleaning up with a follow-up change.
  • Did we explore the possibility of modelling this differently and somehow avoiding making CXXParenInitListExpr an expression. This seems to borrow from InitListExpr, but maybe for the wrong reasons. Braced initializers can actually occur in expressions positions, e.g. int a = {123}. However, CXXParenInistListExpr cannot occur in expression positions outside initializations, e.g. having CXXFunctionalCastExpr for aggregate_struct(123) feels somewhat wrong to me and seems to lead to confusing error messages too. Maybe we should model it as a new expression that contains both the type and the argument list? I.e. more similar to CXXConstructExpr?
clang/lib/Sema/SemaInit.cpp
5564

NIT: s/expressionse/expressions

9683

NIT: add break

clang/lib/Sema/TreeTransform.h
13965

reinterpret_cast seems redundant here or am I missing something?

clang/lib/Serialization/ASTReaderStmt.cpp
2172

NIT: ExpectedNumExprs

2181

NIT: maybe add a comment that dependence does not depend on filler or syntactic form. This might be non-obvious for someone who did not look into the implementation.

Alternatively, you could run this at the end of this function, it should not be confusing to anyone that certain properties get computed *after* the expression was fully deserialized.

clang/test/CXX/class/class.compare/class.spaceship/p1.cpp
106 ↗(On Diff #479758)

Do you have any idea why did this diagnostic change?

clang/test/CXX/drs/dr2xx.cpp
161 ↗(On Diff #479758)

This might be related to other diagnostic change.
Why do we see the new behavior here? Do you think we should keep the old behavior maybe?
I feel that both errors are communicating the same thing and probably keeping the current state is acceptable, just want to better understand how we got there.

It definitely has something to do with the initialization code, but I struggle to understand what exactly changed

clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
131 ↗(On Diff #479758)

Given how pervasive this error is right now, I feel that we want to add a name of the struct to this message.
This case is also a good example of how this diagnostic can be really low quality with templates: it's unclear which exact base class causes a problem here from the compiler output,.

Maybe open a GH issue for that? It seems like an independent task that will also affect braced initializers and may need test file updates.

clang/test/PCH/cxx_paren_init.cpp
30 ↗(On Diff #479758)

NIT: add a newline

clang/test/PCH/cxx_paren_init.h
6 ↗(On Diff #479758)

NIT: add a newline

ayzhao marked 4 inline comments as done.Dec 5 2022, 11:46 AM

Thanks!

I have two major comments and also inline NITs. Not sure if we should block on those, just wanted to hear your opinions:

  • InitListExpr and CXXParenInitListExpr have some code in common. Code duplication is substantial and I think sharing the common implementation is warranted. E.g. if some code would want to change something with ArrayFiller or make a use of it, the work will probably need to be duplicated. To reiterate what I mentioned earlier, I believe deriving CXXParenInitListExpr from InitListExpr is not a very good option, but we might explore other possibilities: a base class or factoring out common code in a separate struct. I believe this could be done with a follow-up change, though, as the amount of changes required does not seem too big, I would be happy with first landing this patch and then cleaning up with a follow-up change.
  • Did we explore the possibility of modelling this differently and somehow avoiding making CXXParenInitListExpr an expression. This seems to borrow from InitListExpr, but maybe for the wrong reasons. Braced initializers can actually occur in expressions positions, e.g. int a = {123}. However, CXXParenInistListExpr cannot occur in expression positions outside initializations, e.g. having CXXFunctionalCastExpr for aggregate_struct(123) feels somewhat wrong to me and seems to lead to confusing error messages too. Maybe we should model it as a new expression that contains both the type and the argument list? I.e. more similar to CXXConstructExpr?

As discussed offline, let's keep the current implementation as is for now.

ayzhao updated this revision to Diff 480184.Dec 5 2022, 11:49 AM

use NumUserSpecifiedExprs instead of syntatic/semantic forms, also address some comments

ayzhao added inline comments.Dec 5 2022, 11:58 AM
clang/lib/CodeGen/CGExprAgg.cpp
577

Instead of implementing separate syntactic and semantic forms, I think it might be simpler and cleaner to store an unsigned NumUserSpecifiedExprs.

The reason for implementing it this way is that the purpose of having separate syntactic and semantic forms is to determine what the form in the code was originally as written and what the form is after semantic analysis. For InitListExpr, we need separate objects because there can be a significant difference in the two forms (e.g. designated initializers, etc.). However, CXXParenListInitExpr is much simpler - the first k expressions are user-specified, while the remaining are default/compiler-generated. Therefore, we can eliminate the complexity required for maintaining two forms (say, in TreeTransform) by just storing k.

N.B. this also resolves a build failure with an earlier diff where the libc++ bots were failing.

There is also precedence for this kind of structure - function calls with default arguments use similar logic instead of creating two Expr objects.

ayzhao updated this revision to Diff 480189.Dec 5 2022, 11:59 AM
ayzhao marked 2 inline comments as done.

add missing EOF newline

ayzhao marked an inline comment as done.Dec 5 2022, 1:33 PM
ayzhao updated this revision to Diff 480596.Dec 6 2022, 1:20 PM
ayzhao marked 3 inline comments as done.

restore original diagnostics

clang/test/CXX/class/class.compare/class.spaceship/p1.cpp
106 ↗(On Diff #479758)

Fixed - I needed to add a line to SemaCast.cpp

clang/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
131 ↗(On Diff #479758)
ayzhao added inline comments.Dec 7 2022, 5:32 PM
clang/lib/Sema/SemaInit.cpp
5489

So the libc++ test compile failures are due to this line.

One example of a failing unit test is range.take.while/ctor.view.pass. Clang calls this function twice in TreeTransform.h - once with VerifyOnly set to true, once with it set to false.

For some reason, when this function tries to value-initialize the member MoveOnly mo in View, Seq.Failed() returns false after TryValueInitialization(...), but the resulting ExprResult is nullptr, causing the segfault we see when we push nullptr to InitExprs and pass InitExprs to the constructor of CXXParenListInitExpr. One way to be fix this is to move the line ExprResult ER = Seq.Perform(...) out of the if (!VerifyOnly) block and check for ER.isInvalid() instead of Seq.Failed(), but that results in test failures due to excess diagnostic messages in Seq.Perform(...)

I'm still looking into this, but if anyone has any ideas, they would be very welcome.

To repro the buildbot failures, just build clang with this patch, and then in a separate build directory, build the target check-cxx using the previously built clang.

ayzhao updated this revision to Diff 481508.Dec 8 2022, 7:52 PM

rebase + (hopefully) fix libc++ c++2b test failures

clang/lib/Sema/SemaInit.cpp
5489

I was able to get the above workaround to pass the test by clearing the diagnostics after calling Seq.Perform(...).

IMO, this should be OK for now, but I'm open to better ideas if anyone has any.

ayzhao added inline comments.Dec 8 2022, 7:56 PM
clang/lib/AST/JSONNodeDumper.cpp
852

Yeah, I would definitely prefer to keep the current style. The problem though is that the buildbots run clang format and will fail if the patch isn't formatted correctly.

ilya-biryukov added inline comments.Dec 9 2022, 8:09 AM
clang/include/clang/AST/ExprCXX.h
4824

NIT: maybe just use return ArrayFiller? The code is much simpler and since functions are very close it's pretty much impossible that anyone would refactor one of those without looking at the other.

clang/lib/AST/ExprConstant.cpp
9977

NIT: !Args.empty() ? Args[0] : &VIE
Args.size() also works, but arguably captures the intent of the code a bit less clearly.

clang/lib/CodeGen/CGExprAgg.cpp
577

That looks fine indeed, I agree that CXXParenInitListExpr is simpler.

clang/lib/Sema/SemaInit.cpp
5390

NIT: reminder to remove this and other debug prints

5435

NIT: maybe rename to SubSeq or something similar?
Having both Seq and Sequence in scope with the same type is error-prone and hard to comprehend.

5489

Clearing all the diagnostics is a nuclear options and definitely seems off here. We should not call Perform() when VerifyOnly is true to avoid producing the diagnostics in the first place.

It's fine for the call with VerifyOnly = true to return no errors and later produce diagnostics with VerifyOnly = false, I believe this is what InitListChecker is already doing.
I have been playing around with the old version of the code, but couldn't fix it fully. I do have a small example that breaks, we should add it to the test and it should also be easier to understand what's going on:

struct MoveOnly
{
  MoveOnly(int data = 1);
  MoveOnly(const MoveOnly&) = delete;
  MoveOnly(MoveOnly&&) = default;
};

struct View {
  int a;
  MoveOnly mo;
};

void test() {
  View{0};
  View(0); // should work, but crashes and produces invalid diagnostics.
}

In general, my approach would be to try mimicing what InitListChecker is doing as much as possible, trimming all the unnecessary complexity that braced-init-lists entail.
Hope it's helpful.

ayzhao updated this revision to Diff 481700.Dec 9 2022, 11:07 AM
ayzhao marked 6 inline comments as done.

address comments + rebase

ayzhao added inline comments.Dec 9 2022, 11:11 AM
clang/lib/Sema/SemaInit.cpp
5489

So it looks like all I had to do was remove the call to TryValueInitialization(...) and just check for Seq.Failed(). This is also what we do in InitListChecker.

The check-cxx target appears to work for me locally, so fingers crossed that the build passes.

I do have a small example that breaks, we should add it to the test and it should also be easier to understand what's going on:

Done.

This LG, but not accepting as I believe libc++ is still broken if built with modules, right?
I have run check-cxx locally and it seems to work, but I suspect that's not using modules by default. I have clicked through the links in phrabricator and the errors seem to come from the latest uploaded diff.
Let me know if I'm missing something.

clang/lib/Sema/SemaInit.cpp
5489

Good catch! I have also noticed that TryValueInitialization is missing in InitListChecker, but somehow thought it's just doing the same thing without reusing the code.

ilya-biryukov added a comment.EditedDec 12 2022, 9:22 AM

Since the errors only shows up in modular builds, I would look closely for bugs inside ASTReader/ASTWriter.
Also, it seems that ArrayFiller is not taken in to account in computeDependence and maybe it should be. I am not 100% sure, though: if ArrayFiller is only used for non-dependent code, it should not case this bug. It also does not explain the variation between modular and non-modular builds.

ayzhao updated this revision to Diff 482261.Dec 12 2022, 2:01 PM

rebase + some compiler warning fixes

Since the errors only shows up in modular builds, I would look closely for bugs inside ASTReader/ASTWriter.
Also, it seems that ArrayFiller is not taken in to account in computeDependence and maybe it should be. I am not 100% sure, though: if ArrayFiller is only used for non-dependent code, it should not case this bug. It also does not explain the variation between modular and non-modular builds.

These test failures are a little weird because they seem to alternate between being able to reliably reproduce vs not being able to reproduce at all.

One thing that I did notice is that the tests fail with RelWithDebInfo builds, but pass with all of the other builds (Debug, Release, and MinSizeRel). Right now though, I'm unable to reproduce the libc++ failure even with RelWithDebInfo.

Incidentally, I just rebased this patch, and it seems like the dr2xx.cpp test is also failing in a similar manner (fails on RelWithDebInfo but passes on other builds).

ayzhao updated this revision to Diff 482566.Dec 13 2022, 11:14 AM

fix memory access issues identified by UBSan

Since the errors only shows up in modular builds, I would look closely for bugs inside ASTReader/ASTWriter.

These test failures are a little weird because they seem to alternate between being able to reliably reproduce vs not being able to reproduce at all.

One thing that I did notice is that the tests fail with RelWithDebInfo builds, but pass with all of the other builds (Debug, Release, and MinSizeRel). Right now though, I'm unable to reproduce the libc++ failure even with RelWithDebInfo.

Incidentally, I just rebased this patch, and it seems like the dr2xx.cpp test is also failing in a similar manner (fails on RelWithDebInfo but passes on other builds).

All of the builds are green now - it turns out that I had to resolve 2 instances of UB in this patch.

Also, it seems that ArrayFiller is not taken in to account in computeDependence and maybe it should be. I am not 100% sure, though: if ArrayFiller is only used for non-dependent code, it should not case this bug. It also does not explain the variation between modular and non-modular builds

I don't think this should be an issue - InitListExpr doesn't take ArrayFiller into account in computeDependence(...) either: https://github.com/llvm/llvm-project/blob/c8647738cd654d9ecfdc047e480d05a997d3127b/clang/lib/AST/ComputeDependence.cpp#L636-L641

LGTM! Thanks a lot for driving this to conclusion! Please note that there is one small NIT you may want to fix before landing this.
UB definitely explains why we saw the bugs. It's good that we caught it before this landed.

I don't think this should be an issue - InitListExpr doesn't take ArrayFiller into account in computeDependence(...) either: https://github.com/llvm/llvm-project/blob/c8647738cd654d9ecfdc047e480d05a997d3127b/clang/lib/AST/ComputeDependence.cpp#L636-L641

Yeah, makes sense, if InitListExpr does not take into account we are probably good here as well.
My intuition is that we can only figure out what to set in ArrayFiller when the expressions are non-dependent, therefore the filler itself should not add any extra "dependence" to the expression.
If that's the case, it would be nice to add an assertion. But that's unrelated to this change, we can do it some other day.

clang/lib/Sema/SemaInit.cpp
6096

NIT: use getFailureKind(), it asserts that Failed() == true and would have catched this particular UB.

This revision is now accepted and ready to land.Dec 14 2022, 7:08 AM
ayzhao updated this revision to Diff 482862.Dec 14 2022, 7:53 AM
ayzhao marked an inline comment as done.

use getFailureKind() + rebase

This revision was landed with ongoing or failed builds.Dec 14 2022, 7:54 AM
This revision was automatically updated to reflect the committed changes.
shafik added inline comments.Dec 14 2022, 12:18 PM
clang/include/clang/AST/ExprCXX.h
4761

It looks like we use this idiom in several places, it may be worth it to sink this as a member function of QualType

clang/lib/CodeGen/CGExprAgg.cpp
1601

This is second time I see you looking for the initialized field for a union while InitListExpr has getInitializedFieldInUnion(). Would it be too painful to also do that for CXXParenListInitExpr to avoid the code duplication?

clang/lib/Sema/SemaInit.cpp
5520

nit

ayzhao marked 3 inline comments as done.Dec 15 2022, 1:01 PM
ayzhao added inline comments.
clang/include/clang/AST/ExprCXX.h
4761

note: this patch was already merged, so I created a new patch addressing the revisions at D140159

It turns out there's a method getValueKindForType(...) that does the exact same thing, so I used that instead.

ayzhao marked an inline comment as done.Dec 19 2022, 4:08 PM
hokein added a subscriber: hokein.Dec 23 2022, 4:46 AM

FYI, this patch triggers a crash in chromium, see https://github.com/llvm/llvm-project/issues/59675 for details.

rsmith added inline comments.Jan 11 2023, 2:23 PM
clang/lib/Sema/SemaInit.cpp
6095–6096

For when you look at re-landing this, we encountered a regression for this case:

struct A {};
struct B : A { Noncopyable nc; };

Here, given B b, a B(b) direct-initialization should be rejected because it selects a deleted copy constructor of B, but it looks like Clang instead performed aggregate initialization, as if by B{(const A&)b, {}}. This is pretty bad -- copies that should be disallowed end up working but not actually copying any members of the derived class!

The general problem is that Clang's overload resolution incorrectly handles deleted functions, treating them as an overload resolution failure rather than a success. In this particular case, what happens is constructor overload resolution produces OR_Deleted, which gets mapped into FK_ConstructorOverloadFailed despite the fact that overload resolution succeeded and picked a viable function.

I guess we can split FK_*OverloadFailed into separate "failed" and "succeeded but found a deleted function" cases?

ayzhao added inline comments.Jan 11 2023, 2:37 PM
clang/lib/Sema/SemaInit.cpp
6095–6096

For when you look at re-landing this, we encountered a regression for this case:

struct A {};
struct B : A { Noncopyable nc; };

Here, given B b, a B(b) direct-initialization should be rejected because it selects a deleted copy constructor of B, but it looks like Clang instead performed aggregate initialization, as if by B{(const A&)b, {}}. This is pretty bad -- copies that should be disallowed end up working but not actually copying any members of the derived class!

The general problem is that Clang's overload resolution incorrectly handles deleted functions, treating them as an overload resolution failure rather than a success. In this particular case, what happens is constructor overload resolution produces OR_Deleted, which gets mapped into FK_ConstructorOverloadFailed despite the fact that overload resolution succeeded and picked a viable function.

I guess we can split FK_*OverloadFailed into separate "failed" and "succeeded but found a deleted function" cases?

This was the cause of https://github.com/llvm/llvm-project/issues/59675 as well. The reland at D141546 should handle this case.