This is an archive of the discontinued LLVM Phabricator instance.

clang-tidy: Detect use-after-move in CXXCtorInitializer
ClosedPublic

Authored by MarcoFalke on Mar 17 2023, 5:04 AM.

Diff Detail

Event Timeline

MarcoFalke created this revision.Mar 17 2023, 5:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
MarcoFalke requested review of this revision.Mar 17 2023, 5:04 AM

clang-format+doc

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
194

Please keep alphabetical order (by check name) in this section.

MarcoFalke marked an inline comment as done.Mar 17 2023, 7:38 AM

I will check this more deeply during weekend.

PiotrZSL requested changes to this revision.Mar 18 2023, 4:26 AM

Requesting changes due to lack of support for base class initializes & got some concerns related lambdas used in initializers.

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
406–413

This is correct but consider this:

auto TryEmplaceMatcher = cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));

auto CallMoveMatcher =
callExpr(argumentCountIs(1), // because matching this will be faster than checking name.
             callee(functionDecl(hasName("::std::move"))),
             unless(inDecltypeOrTemplateArg()),
             expr().bind("call-move"),
             unless(hasParent(TryEmplaceMatcher)),
             anyOf(hasAncestor(compoundStmt(hasParent(lambdaExpr().bind("containing-lambda")))),
                        hasAncestor(functionDecl(anyOf(cxxConstructorDecl(hasAnyConstructorInitializer(
                                                                                                                   withInitializer(expr(
                                                                                                                          anyOf(equalsBounNode("call-move"), 
                                                                                                                                     hasDescendant(equalsBounNode("call-move")))
                                                                                                                     ).bind("containing-ctor-init-stmt")
                                                                                                              ))
                                                                                                             ).bind("containing-ctor"),
                                                                             functionDecl().bind("containing-func")))))
            );
472

consider using here ->IgnoreImplicit() == ContainingCtorInitStmt->IgnoreImplicit()

477–486

This won't work for std::move used to initialize base class. Assuming that Inits are in specific order, then you just should include all init's after one found, or use things like SourceManager::isBeforeInTranslationUnit, but still probably using something like:

bool PastMove = false;
for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
if (!PastMove  && Init->getInit()->IgnoreImplicit() == ContainingCtorInitStmt->IgnoreImplicit()) {
    PastMove  = true;
}

if (PastMove && Init->getInit())
{
    CodeBlocks.push_back(Init->getInit());
}

would be better, as it should work for both field inits and base constructor calls

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1506

Missing tests:

  • Test with A derive from B, C, D, and argument is moved into C, but D initialization also uses it.
  • Test with lambda call inside init, and in lambda std::move of captured by reference constructor parameter, and lambda could be mutable, and lambda could be saved into member (or passed to base class), or called instantly (better both tests).
  • Test with lambda that takes argument into capture by move

In short:

struct Obj {};
struct D
{
    D(Obj b);
};

struct C
{
     C(Obj b);
};

struct B
{
     B(Obj& b);
}

struct A : B, C, D
{
    A(Obj b)
      : B(b), C(std::move(b)), D(b)
    {
    }
};

and second:

struct Base
{
    Base(Obj b) : bb(std::move(b)) {}
     template<typename T>
     Base(T&& b) : bb(b()) {};
   
     Obj bb;
};

struct Derived : Base, C
{
     Derived(Obj b) 
           : Base([&] mutable { return std::move(b); }()), C(b)
   {
   }
};

struct Derived2 : Base, C
{
     Derived2(Obj b) 
           : Base([&] mutable { return std::move(b); }), C(b)
   {
   }
};

struct Derived3 : Base, C
{
     Derived3(Obj b) 
           : Base([c = std::move(b)] mutable { return std::move(c); }), C(b)
   {
   }
};
This revision now requires changes to proceed.Mar 18 2023, 4:26 AM
MarcoFalke marked an inline comment as done.

Took some review suggestions (Thanks!)

MarcoFalke added inline comments.Mar 20 2023, 7:56 AM
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
406–413

I get:

error: no matching function for call to object of type 'const internal::ArgumentAdaptingMatcherFunc<clang::ast_matchers::internal::HasDescendantMatcher>'
                    anyOf(equalsBoundNode("call-move"),hasDescendant(equalsBoundNode("call-move")))
                                                       ^~~~~~~~~~~~~
./llvm-project/clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1491:3: note: candidate template ignored: could not match 'Matcher' against 'PolymorphicMatcher'
  operator()(const Matcher<T> &InnerMatcher) const {
  ^
./llvm-project/clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1498:3: note: candidate template ignored: could not match 'MapAnyOfHelper' against 'PolymorphicMatcher'
  operator()(const MapAnyOfHelper<T...> &InnerMatcher) const {
  ^
1 error generated.
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1506

I added the test Derived2, however, I wonder if it should throw a warning. In the general case, I don't think we can know whether Base(Call&&call) will actually call call and execute the std::move, no?

PiotrZSL added inline comments.Mar 20 2023, 8:30 AM
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
406–413

yee, forgot type: hasDescendant(callExpr(equalsBoundNode("call-move"))))
+- something like this..

PiotrZSL added inline comments.Mar 20 2023, 8:36 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1506

For me:
Derived - should throw a warning - same type used before C(b) (but questionable)
Derived 2 -> also should throw warning (but questionable)
Derived 3 -> also should throw warning

Note that Derived & Derived 2 could be considered different issue, like use after free (because we take reference to argument), it's safe to assume that it would be called anyway, because later that argument reference could become invalid. But it could be also fine to not throw issue. For is better to throw & nolint than ignore and crash.
But thats your call.

address feedback about matcher, clang-format

MarcoFalke marked 3 inline comments as done.Mar 20 2023, 10:07 AM

Thanks, addressed some more review comments.

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1506

Ok, maybe it can be done in a follow-up? This should be a separate and pre-existing issue on main already (see also my other added test case about lambdas).

PiotrZSL added inline comments.Mar 20 2023, 1:00 PM
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1506

Yes it could be.

PiotrZSL accepted this revision.EditedMar 20 2023, 1:04 PM

Overall LGTM, give it few days, maybe something will pop up.
I will try to run this change on my project and check if it finds any false-positives or real-issues.
Edit: Windows build failed due to clangd, try rebasing this to newer main branch

This revision is now accepted and ready to land.Mar 20 2023, 1:04 PM
MarcoFalke marked 2 inline comments as done.Mar 22 2023, 3:38 AM

Minor NFC, I plan to land this tomorrow or Friday

PiotrZSL accepted this revision.Mar 22 2023, 4:43 AM