Page MenuHomePhabricator

[Clang] Fix memory leak due to TemplateArgumentListInfo used in AST node.
ClosedPublic

Authored by browneee on Jun 2 2022, 9:59 PM.

Diff Detail

Event Timeline

browneee created this revision.Jun 2 2022, 9:59 PM
Herald added a project: Restricted Project. · View Herald Transcript
browneee requested review of this revision.Jun 2 2022, 9:59 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 2 2022, 9:59 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

More details on the bug: https://reviews.llvm.org/D125802#3551305

Alternative implementation for fix: https://reviews.llvm.org/D126937

I'm not yet sure if these changes are correct.

aaron.ballman added inline comments.
clang/include/clang/AST/DeclTemplate.h
2776

Can we make it a prerequisite that the argument has to be nonnull and thus we can pass in a const reference instead?

2778–2779

Do we want to assert that TemplateArgsInfo is nonnull and return a const reference instead?

Basically, I'm wondering if we can remove a bunch of the pointers and go with references as much as possible; it seems to me that a null pointer is a sign of an error, same as with TemplateArgs.

clang/lib/AST/ASTImporter.cpp
6021–6024

You should spell out the type name here instead of using auto (per coding standard).

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
5561

You should spell out the type name here as well.

5564–5566

More coding standard changes -- btw, it looks like you need to run the patch through clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).

browneee updated this revision to Diff 434075.Jun 3 2022, 11:10 AM

Remove auto used under clang/. Format.

browneee updated this revision to Diff 434135.Jun 3 2022, 1:58 PM
browneee marked 3 inline comments as done.

Formatting.

clang/include/clang/AST/DeclTemplate.h
2776

I tried this initially. Tests broke, so I went with this more conservative change. Someone may be able to reduce use of nulls in follow up changes.

2778–2779

This may be why https://reviews.llvm.org/D126937 is failing tests.

My goals is to fix the memory leak, so the sanitizer buildbots are clean. I would agree there is probably more cleanup to do for this member variable after https://reviews.llvm.org/D126944 - but fixing the memory leak is the urgent thing for me.

aaron.ballman accepted this revision.Jun 8 2022, 5:32 AM

LGTM; I think it's reasonable to not have test coverage specific to the changes here (the existing test coverage already uncovered this issue). It might be worth mentioning that the leak was fixed in a release note, however. Also, I did find one naming convention issue that should be fixed before landing.

Thanks for fixing this up!

clang/include/clang/AST/DeclTemplate.h
2778–2779

Okay, I'm fine not handling the extra safety measures at the moment.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
5564–5566

It looks like you may have missed the rename suggestion of switching arg to Arg per our usual coding conventions. You can fix that when landing though.

This revision is now accepted and ready to land.Jun 8 2022, 5:32 AM
browneee updated this revision to Diff 435224.Jun 8 2022, 9:51 AM
browneee marked an inline comment as done.

Added release notes and fixed variable naming.

erichkeane accepted this revision.Jun 8 2022, 9:52 AM

LGTM! Thanks for this!

This revision was landed with ongoing or failed builds.Jun 8 2022, 9:58 AM
This revision was automatically updated to reflect the committed changes.