Page MenuHomePhabricator

[clang][Sema] Create delegating constructors even in templates
Needs ReviewPublic

Authored by fwolff on Nov 9 2021, 2:05 PM.

Details

Summary

This fixes a bug in clang-tidy (PR#37902), even though the code for the cppcoreguidelines-pro-type-member-init check itself is perfectly fine. It uses the isDelegatingConstructor() matcher, which does not match delegating constructors in templates because the AST contains incorrect information (constructors in templates are never marked as delegating). This patch fixes this behavior by constructing a correct CXXCtorInitializer for delegating constructors in templates.

Diff Detail

Unit TestsFailed

TimeTest
7,600 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest

Event Timeline

fwolff created this revision.Nov 9 2021, 2:05 PM
fwolff requested review of this revision.Nov 9 2021, 2:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2021, 2:05 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
carlosgalvezp requested changes to this revision.Nov 10 2021, 10:52 AM

Amazing, thanks a lot for fixing this long-standing issue! I have a couple comments. I'm not familiar at all with the Sema part so someone that knows should look at it :)

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
374

Not really sure what this test is meant to do. Why would it call the destructor of the own class in it's own constructor? Looks very strange to me.

If anything, the constructor should call the constructor of the base:

PositiveSelfInitialization() : NegativeAggregateType()

What do you think?

555

typename T?

Not sure if it makes a difference

559

Could you add another test where X is initialized via NSDMI instead of in the constructor initializer list?

int X{};
TemplateWithDelegatingCtor() = default;
TemplateWithDelegatingCtor(int) : TemplateWithDelegatingCtor() {}

This revision now requires changes to proceed.Nov 10 2021, 10:52 AM

Also, what's with the pre-merge test, is it related?

fwolff updated this revision to Diff 386297.Nov 10 2021, 1:15 PM
fwolff marked 2 inline comments as done.Nov 10 2021, 1:23 PM

Thanks for your review @carlosgalvezp! I have addressed your inline comments. I'm not sure about the failing test, but it looks like a regression test internal to libFuzzer? Maybe it goes away with the rebuild.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
374

The comment above talks about a "pathological template", so my guess is that this checks that clang-tidy doesn't crash for this input. The only reason why I had to touch this test at all is that the constructor is now treated as a delegating constructor, which suppresses the warning.

carlosgalvezp added inline comments.Nov 11 2021, 12:55 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
374

Hmm, I see. I would like to make sure we still catch the failure mode "this constructor does not initialize these base classes" for class templates.

I don't see such test existing (only for non-template classes), maybe you can add that too?

fwolff updated this revision to Diff 386620.Nov 11 2021, 12:08 PM
fwolff added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
374

Good point, done now.

fwolff marked 2 inline comments as done.Nov 11 2021, 12:11 PM
carlosgalvezp accepted this revision.Nov 14 2021, 7:33 AM

Looks great, thanks! I'm not comfortable reviewing the Sema part, maybe someone else can have a look?

This revision is now accepted and ready to land.Nov 14 2021, 7:33 AM
whisperity retitled this revision from [clang] Create delegating constructors even in templates to [clang][Sema] Create delegating constructors even in templates.Nov 26 2021, 12:38 AM
whisperity edited reviewers, added: Quuxplusone; removed: carlosgalvezp, whisperity.
whisperity added subscribers: carlosgalvezp, whisperity.

(@carlosgalvezp Removing you from reviewers so the patch shows up as not yet reviewed for others! The significant part is the Sema which should be reviewed.)

This revision now requires review to proceed.Nov 26 2021, 12:39 AM

Thanks for the finding and sorry for delaying the review, I didn't know that :( Do you know if there's an option for signaling "LGTM but I want someone else to review?". Otherwise I can just leave a comment and don't accept the revision.

Thanks for the finding and sorry for delaying the review, I didn't know that :( Do you know if there's an option for signaling "LGTM but I want someone else to review?". Otherwise I can just leave a comment and don't accept the revision.

There is an action "resign from review", but as I have taken you off explicitly and by force, no need for that. 🙂 I just wrote the comment FYI so that you know I'm not just interfering destructively! (And due to no accepting reviewers remaining acting as such, the patch automatically went back to "now require review to proceed".)

aaron.ballman added inline comments.Nov 29 2021, 12:07 PM
clang/lib/Sema/SemaDeclCXX.cpp
4524–4525

Looking at the source for BuildDelegatingInitializer(), it looks like we should still be able to call this in a dependent context. In fact, it has a fixme comment specifically about that:

// If we are in a dependent context, template instantiation will
// perform this type-checking again. Just save the arguments that we
// received in a ParenListExpr.
// FIXME: This isn't quite ideal, since our ASTs don't capture all
// of the information that we have about the base
// initializer. However, deconstructing the ASTs is a dicey process,
// and this approach is far more likely to get the corner cases right.

I'm wondering if the better fix here is to hoist the delegation check out of the if (!Dependent). Did you try that and run into issues?

5071–5075

Can you explain why this change was needed?

fwolff marked 2 inline comments as done.Dec 4 2021, 9:52 AM
fwolff added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
4524–4525

Yes, I have tried this, and it leads to a crash here (because the cast fails). Apparently, this code path in BuildDelegatingInitializer() is never exercised for dependent contexts, despite the comment, and therefore doesn't work.

5071–5075

Without this, clang crashes because LookupDestructor() calls LookupSpecialMember(), which then runs into this assertion:

clang-14: /[...]/llvm-project/clang/lib/Sema/SemaLookup.cpp:3064: clang::Sema::SpecialMemberOverloadResult clang::Sema::LookupSpecialMember(clang::CXXRecordDecl*, clang::Sema::CXXSpecialMember, bool, bool, bool, bool, bool): Assertion `CanDeclareSpecialMemberFunction(RD) && "doing special member lookup into record that isn't fully complete"' failed.

Previously, this wasn't necessary because no delegating constructors would be generated for templates, and so this function wasn't ever called in a dependent context.

fwolff marked 2 inline comments as done.Dec 4 2021, 9:54 AM

@aaron.ballman Ping? I think I've responded to all comments so far; let me know if you still have concerns.