This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Fix ResolveConstructorOverload to not select a conversion function if we are going use copy elision
ClosedPublic

Authored by shafik on Apr 16 2023, 12:01 PM.

Diff Detail

Event Timeline

shafik created this revision.Apr 16 2023, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2023, 12:01 PM
shafik requested review of this revision.Apr 16 2023, 12:01 PM
shafik added inline comments.Apr 16 2023, 12:04 PM
clang/lib/Sema/SemaTemplateInstantiate.cpp
813

I added this fix because the B3 test case I added below although was fixed by my patch it uncovered a new bug that resulted in the compiler locking up.

It looks like the malformed code results in a us seeing a ClassTemplateDecl where we would usually not expect it. If this handled better earlier let me know.

rsmith added inline comments.Apr 16 2023, 3:41 PM
clang/lib/Sema/SemaTemplateInstantiate.cpp
816

This diagnostic won't include the template arguments that we're using to instantiate, which seems like important information.

Do you know where we're creating the InstantiatingTemplate instance corresponding to this diagnostic? Usually we pass in the declaration whose definition we're instantiating for a TemplateInstantiation record; I wonder if we could do the same for this case.

clang/test/SemaCXX/cxx1z-copy-omission.cpp
175

We should also test that the right function is actually being selected and called. For example:

struct A {
  A();
  A(const A&) = delete;
};
struct B {
  operator A();
} C;
A::A() : A(C) {}

... should be rejected because it calls the deleted copy constructor. (Or you could test this via constant evaluation or look at the generated code, but this is probably the simplest way.)

193

Do you need to omit the { here as part of this test? This will cause problems for people editing the file due to mismatched braces, and makes this test fragile if our error recovery changes. It's best for the test case to be free of errors other than the one(s) being exercised. (Same for missing ; errors below.)

If these errors are necessary to trigger the bug you found, I wonder if there's a problem with our error recovery. (Maybe we create a "bad" InstantiatingTemplate record during error recovery, for example.)

shafik updated this revision to Diff 514091.Apr 16 2023, 9:21 PM
shafik marked 3 inline comments as done.
  • Fix up A3 test to be less malformed
  • Fix other test so we confirm we are attempting to call the right contructor
clang/lib/Sema/SemaTemplateInstantiate.cpp
816

This is happening in Sema::ActOnCallExpr here:

cpp
 // Diagnose uses of the C++20 "ADL-only template-id call" feature in earlier
  // language modes.
  if (auto *ULE = dyn_cast<UnresolvedLookupExpr>(Fn)) {
    if (ULE->hasExplicitTemplateArgs() &&
        ULE->decls_begin() == ULE->decls_end()) {
      Diag(Fn->getExprLoc(), getLangOpts().CPlusPlus20
                                 ? diag::warn_cxx17_compat_adl_only_template_id
                                 : diag::ext_adl_only_template_id)
          << ULE->getName();
    }
  }

if I check:

console
expr CodeSynthesisContexts->rbegin()->Entity

That is indeed the ClassTemplateDecl and we do have MultiExprArg ArgExprs there as well.

The call right before this TemplateInstantiator::RebuildCallExpr

clang/test/SemaCXX/cxx1z-copy-omission.cpp
193

They are not needed to trigger the issue, I removed them. I was torn about how faithful to remain to the original test case.

dim requested changes to this revision.Jul 11 2023, 9:37 AM

FWIW, this fix works for the test cases I had via https://bugs.freebsd.org/269067 and https://github.com/llvm/llvm-project/issues/60182, but I got the following failure during check-all:

********************
FAIL: Clang :: SemaCXX/cxx1z-copy-omission.cpp (15698 of 67299)
******************** TEST 'Clang :: SemaCXX/cxx1z-copy-omission.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang -cc1 -internal-isystem /home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/lib/clang/17/include -nostdsysteminc -std=c++1z -verify -Wno-unused /home/dim/src/llvm/llvm-project/clang/test/SemaCXX/cxx1z-copy-omission.cpp
--
Exit Code: 134

Command Output (stderr):
--
unexpected deduction guide in instantiation stack
UNREACHABLE executed at /home/dim/src/llvm/llvm-project/clang/lib/Sema/SemaTemplateInstantiate.cpp:1067!
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/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang -cc1 -internal-isystem /home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/lib/clang/17/include -nostdsysteminc -std=c++1z -verify -Wno-unused /home/dim/src/llvm/llvm-project/clang/test/SemaCXX/cxx1z-copy-omission.cpp
1.      /home/dim/src/llvm/llvm-project/clang/test/SemaCXX/cxx1z-copy-omission.cpp:198:8: current parser token ';'
2.      /home/dim/src/llvm/llvm-project/clang/test/SemaCXX/cxx1z-copy-omission.cpp:176:1: parsing namespace 'GH39319'
 #0 0x0000000002ca4727 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x2ca4727)
 #1 0x0000000002ca2508 llvm::sys::RunSignalHandlers() (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x2ca2508)
 #2 0x0000000002ca4f00 SignalHandler(int) Signals.cpp:0:0
 #3 0x0000000827a15a3e handle_signal /usr/src/lib/libthr/thread/thr_sig.c:0:3
 #4 0x0000000827a14ff9 thr_sighandler /usr/src/lib/libthr/thread/thr_sig.c:247:1
 #5 0x0000000826e18903 ([vdso]+0x2d3)
 #6 0x000000082dc3f97a __sys_thr_kill /usr/obj/usr/src/lib/libc/thr_kill.S:4:0
 #7 0x000000082dbb8954 __raise /usr/src/lib/libc/gen/raise.c:0:10
 #8 0x000000082dc693e9 abort /usr/src/lib/libc/stdlib/abort.c:73:17
 #9 0x0000000002c23b01 (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x2c23b01)
#10 0x000000000590e986 clang::Sema::PrintInstantiationStack() (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x590e986)
#11 0x000000000504555d clang::Sema::EmitCurrentDiagnostic(unsigned int) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x504555d)
#12 0x00000000050464ea clang::Sema::ImmediateDiagBuilder::~ImmediateDiagBuilder() Sema.cpp:0:0
#13 0x0000000005046668 clang::Sema::SemaDiagnosticBuilder::~SemaDiagnosticBuilder() (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x5046668)
#14 0x00000000053f7c5b clang::Sema::ActOnCallExpr(clang::Scope*, clang::Expr*, clang::SourceLocation, llvm::MutableArrayRef<clang::Expr*>, clang::SourceLocation, clang::Expr*) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x53f7c5b)
#15 0x0000000005920009 clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformCallExpr(clang::CallExpr*) SemaTemplateInstantiate.cpp:0:0
#16 0x000000000591713d clang::Sema::SubstExpr(clang::Expr*, clang::MultiLevelTemplateArgumentList const&) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x591713d)
#17 0x00000000059615e5 clang::TemplateDeclInstantiator::VisitNonTypeTemplateParmDecl(clang::NonTypeTemplateParmDecl*) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x59615e5)
#18 0x000000000599c9a9 void llvm::function_ref<void ()>::callback_fn<clang::Sema::SubstDecl(clang::Decl*, clang::DeclContext*, clang::MultiLevelTemplateArgumentList const&)::$_0>(long) SemaTemplateInstantiateDecl.cpp:0:0
#19 0x000000000503ec8e clang::Sema::runWithSufficientStackSpace(clang::SourceLocation, llvm::function_ref<void ()>) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x503ec8e)
#20 0x00000000059667a2 clang::Sema::SubstDecl(clang::Decl*, clang::DeclContext*, clang::MultiLevelTemplateArgumentList const&) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x59667a2)
#21 0x00000000057c6e6d clang::Sema::DeclareImplicitDeductionGuides(clang::TemplateDecl*, clang::SourceLocation) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x57c6e6d)
#22 0x000000000562bc8d LookupDirect(clang::Sema&, clang::LookupResult&, clang::DeclContext const*) SemaLookup.cpp:0:0
#23 0x0000000005627e5c CppNamespaceLookup(clang::Sema&, clang::LookupResult&, clang::ASTContext&, clang::DeclContext const*, (anonymous namespace)::UnqualUsingDirectiveSet&) SemaLookup.cpp:0:0
#24 0x00000000056274ad clang::Sema::CppLookupName(clang::LookupResult&, clang::Scope*) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x56274ad)
#25 0x000000000562b4d0 clang::Sema::LookupName(clang::LookupResult&, clang::Scope*, bool, bool) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x562b4d0)
#26 0x00000000051f037e clang::Sema::HandleDeclarator(clang::Scope*, clang::Declarator&, llvm::MutableArrayRef<clang::TemplateParameterList*>) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x51f037e)
#27 0x00000000051efc4b clang::Sema::ActOnDeclarator(clang::Scope*, clang::Declarator&) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x51efc4b)
#28 0x0000000004f81236 clang::Parser::ParseDeclarationAfterDeclaratorAndAttributes(clang::Declarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::ForRangeInit*) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f81236)
#29 0x0000000004f7f9f2 clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::ParsedAttributes&, clang::SourceLocation*, clang::Parser::ForRangeInit*) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f7f9f2)
#30 0x0000000004f11179 clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f11179)
#31 0x0000000004f10a78 clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f10a78)
#32 0x0000000004f0f80a clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f0f80a)
#33 0x0000000004f58a37 clang::Parser::ParseInnerNamespace(llvm::SmallVector<clang::Parser::InnerNamespaceInfo, 4u> const&, unsigned int, clang::SourceLocation&, clang::ParsedAttributes&, clang::BalancedDelimiterTracker&) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f58a37)
#34 0x0000000004f581f8 clang::Parser::ParseNamespace(clang::DeclaratorContext, clang::SourceLocation&, clang::SourceLocation) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f581f8)
#35 0x0000000004f7e414 clang::Parser::ParseDeclaration(clang::DeclaratorContext, clang::SourceLocation&, clang::ParsedAttributes&, clang::ParsedAttributes&, clang::SourceLocation*) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f7e414)
#36 0x0000000004f0f370 clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f0f370)
#37 0x0000000004f0d3ff clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f0d3ff)
#38 0x0000000004f0794e clang::ParseAST(clang::Sema&, bool, bool) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x4f0794e)
#39 0x0000000003780c33 clang::FrontendAction::Execute() (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x3780c33)
#40 0x0000000003704f2f clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x3704f2f)
#41 0x0000000003853750 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x3853750)
#42 0x0000000001ad1f28 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x1ad1f28)
#43 0x0000000001acd854 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) driver.cpp:0:0
#44 0x0000000001acc6ea clang_main(int, char**, llvm::ToolContext const&) (/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang+0x1acc6ea)
/home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/tools/clang/test/SemaCXX/Output/cxx1z-copy-omission.cpp.script: line 1:   661 Abort trap              (core dumped) /home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/bin/clang -cc1 -internal-isystem /home/dim/obj/llvmorg-17-init-17477-g3ab7ef28eebf-freebsd13-amd64-ninja-clang-rel-1/lib/clang/17/include -nostdsysteminc -std=c++1z -verify -Wno-unused /home/dim/src/llvm/llvm-project/clang/test/SemaCXX/cxx1z-copy-omission.cpp

--

********************

Any idea what that might be?

This revision now requires changes to proceed.Jul 11 2023, 9:37 AM
dim added a comment.Jul 11 2023, 9:38 AM

FWIW, this fix works for the test cases I had via https://bugs.freebsd.org/269067 and https://github.com/llvm/llvm-project/issues/60182, but I got the following failure

This was with llvmorg-17-init-17477-ab7ef28eebf (rG3ab7ef28eebf), by the way.

FWIW, this fix works for the test cases I had via https://bugs.freebsd.org/269067 and https://github.com/llvm/llvm-project/issues/60182, but I got the following failure during check-all:

I seem to be seeing the same issue with my build as well. Let me debug it and see if I can figure out what changed since I first worked on this and adjust.

shafik added a comment.Oct 3 2023, 9:18 AM

@dim the crash should be fixed by landing: https://github.com/llvm/llvm-project/pull/67373 I need to rebase and test

shafik added a comment.EditedOct 26 2023, 5:34 PM

ping, we have several bugs related to this. So it would be nice to land this fix soon if possible.

dim accepted this revision.Tue, Nov 14, 6:51 AM

Yes, please. :)

This revision is now accepted and ready to land.Tue, Nov 14, 6:51 AM
aaron.ballman accepted this revision.Tue, Nov 14, 10:16 AM

The changes look reasonable to me (@rsmith had a lot of comments, but I think you addressed them; it would be nice if he could confirm), but should definitely come with a release note. So LGTM modulo those nits and any last-minute comments from Richard.

clang/lib/Sema/SemaTemplateInstantiate.cpp
813
shafik edited the summary of this revision. (Show Details)Fri, Dec 8, 8:50 AM
shafik updated this revision to Diff 558218.Fri, Dec 8, 8:53 AM
  • Adding release note
Herald added a project: Restricted Project. · View Herald TranscriptFri, Dec 8, 9:40 AM