[clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments.

[mboehme]

Summary

This eliminates false positives in bugprone-use-after-move where a variable
is used in the callee and moved from in the arguments.

We introduce one special case: If the callee is a MemberExpr with a DeclRefExpr as its base, we consider it to be sequenced after the arguments. This is because the variable referenced in the base will only actually be accessed when the call happens, i.e. once all of the arguments have been evaluated. This has no basis in the C++ standard, but it reflects actual behavior that is relevant to a use-after-move scenario:

a.bar(consumeA(std::move(a));

In this example, we end up accessing a after it has been moved from, even though nominally the callee a.bar is evaluated before the argument consumeA(std::move(a)).

Treating this scenario correctly has required rewriting the logic in bugprone-use-after-move that governs whether the use happens in a later loop iteration than the move. This was previously based on an unsound heuristic (does the use come lexically before the move?); we now use a more rigourous criterion based on reachability in the CFG.

[Eugene.Zelenko]

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

1379

Please add.

[PiotrZSL]

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

1296

This entire file tests only C++17 and later, when std::move is C++11 feature.
Add separate test file for C++11 (or even better split this one between C++11 and C++17)

Scenario to test in C++11, should warn ?

a->foo(std::move(a));

1304

What about scenario like this:

b.foo(a->saveBIntoAAndReturnBool(std::move(b)));

Is first "b" still guaranteed to be alive after std::move ?

1305

I didn't found any test for correct warning in this case:

std::unique_ptr<A> getArg(std::unique_ptr<A>);
getArg(std::move(a))->foo(std::move(a));

[PiotrZSL]

Add more tests (those missing one), to make sure that this wont break proper cases.
Refer to inline comments.

[mboehme]

Changes in response to review comments.

[mboehme]

Added entry to release notes.

[mboehme]

Sorry for the delay in replying -- I was caught up in other projects.

clang-tools-extra/docs/ReleaseNotes.rst

224 ↗

(On Diff #506510)

Reworded according to comments in https://reviews.llvm.org/D145906.

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

1296

This entire file tests only C++17 and later, when std::move is C++11 feature.
Add separate test file for C++11 (or even better split this one between C++11 and C++17)

Good point. I've added a new RUN line that also runs this in C++11 mode.

Scenario to test in C++11, should warn ?

See below -- this does produce a warning in C++11 mode (but not C++17 mode).

I've also redone the tests to reuse the existing class A without having to define a new one.

1304

I'm not exactly sure what you're asking here... or how this scenario is materially different from the other scenarios we already have?

Is first "b" still guaranteed to be alive after std::move ?

The b in b.foo is guaranteed to be evaluated before the call a->saveBIntoAAndReturnBool(std::move(b)) -- but I'm not sure if this is what you're asking?

Or are you asking whether the a->saveBIntoAAndReturnBool(std::move(b)) can cause the underlying object to be destroyed before the call to b.foo happenss? In other words, do we potentially have a use-after-free here?

I think the answer to this depends on what exactly saveBIntoAAndReturnBool() does (what was your intent here?). I also think it's probably beyond the scope of this check in any case, as this check is about diagnosing use-after-move, not use-after-free.

1305

Good point -- added below (in slightly different form, but testing the same thing).

Note that this is an error in both C++11 and C++17, but in C++11 the use and move are unsequenced, whereas in C++17 the use definitely comes after the move.

[PiotrZSL]

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

1304

I see this `b.foo(a->saveBIntoAAndReturnBool(std::move(b)));` like this:
we call saveBIntoAAndReturnBool, that takes b by std::move, then we call foo on already moved object.
For me this is use after move, that's why I was asking.

And in "b.foo" there is almost nothing to evaluate, maybe address of foo, but at the end foo will be called on already moved object.
If we would have something like "getSomeObj(b).boo(std::move(b))" then we can think about "evaluate", but when we directly call method on moved object, then we got use after move

[mboehme]

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

1304

Ah, I think I understand what you're getting at now. I was assuming for some reason that b was also a unique_ptr in this example, but of course that doesn't make sense because in that case we wouldn't be able to use the dot operator on b (i.e. b.foo).

Distinguishing between these two cases will require making the check more sophisticated -- the logic that the callee is sequenced before the arguments is not sufficient on its own. I'll have to take a closer look at how to do this, but it will likely involve looking at the MemberExpr inside the CXXMemberCallExpr. If MemberExpr::getBase() is simply a DeclRefExpr, we'll want to do one thing, and if MemberExpr::getBase() is some sort of CallExpr, we'll want to do something else. There will likely need to be other considerations involved as well, but I wanted to sketch out in broad lines where I think this should go.

I'll likely take a few days to turn this around, but in the meantime I wanted to get this comment out to let you know that I now understand the issue.

[PiotrZSL]

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

1304

Yes but that's not so easy, as there can be thing like:
x.y.foo(std::move(x));

To be honest probably easiest way would be to extract isIdenticalStmt from clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp, and then we could just check if callExpr callee contain argument of std::move, and that argument does not contain any other callExpr before current one.
For such cases we could warn, but for all other cases when there any other sub callExpr involved we woudn't need to wanr.

to be honest I need isIdenticalStmt for my other checks, so if you decide to go this route do this under separate patch.

Reasn why I mention isIdenticalStmt is because this would handle also things like this:

x.z.foo(std::move(y)), where x and y are same types.

However if you decide to do some tricks with MemberExpr, good luck (i wouldn't bother) there are other usecases to watch out:
like getX().z.y.foo(std::move(getX().z)), and partial moving examples...

[PiotrZSL]

Switching status of review, once you will be ready with changes (or your decision), just mark it ready for review again.

[PiotrZSL]

And actually there is issue for this: https://github.com/llvm/llvm-project/issues/57758

[mboehme]

In D145581#4215602, @PiotrZSL wrote:

Switching status of review, once you will be ready with changes (or your decision), just mark it ready for review again.

Thanks, and sorry for the delayed response on this -- I've been juggling this patch with other work.

In D145581#4223185, @PiotrZSL wrote:

And actually there is issue for this: https://github.com/llvm/llvm-project/issues/57758

Thanks for making me aware of this!

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

222 ↗

(On Diff #506510)

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

[mboehme]

Changes in reponse to review comments.

[mboehme]

Changes in response to review comments

[mboehme]

In D145581#4215602, @PiotrZSL wrote:

Switching status of review, once you will be ready with changes (or your decision), just mark it ready for review again.

Did I do this correctly? It says "Needs Review" now, though I think I didn't do anything specific to trigger this.

In D145581#4223185, @PiotrZSL wrote:

And actually there is issue for this: https://github.com/llvm/llvm-project/issues/57758

Thanks, I'll update this once this change is submitted.

clang-tools-extra/docs/ReleaseNotes.rst

222 ↗

(On Diff #506510)

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

Done.

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

1304

Yes but that's not so easy, as there can be thing like:
x.y.foo(std::move(x));

To be honest probably easiest way would be to extract isIdenticalStmt from clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp, and then we could just check if callExpr callee contain argument of std::move, and that argument does not contain any other callExpr before current one.
For such cases we could warn, but for all other cases when there any other sub callExpr involved we woudn't need to wanr.

to be honest I need isIdenticalStmt for my other checks, so if you decide to go this route do this under separate patch.

Reasn why I mention isIdenticalStmt is because this would handle also things like this:

x.z.foo(std::move(y)), where x and y are same types.

However if you decide to do some tricks with MemberExpr, good luck (i wouldn't bother) there are other usecases to watch out:
like getX().z.y.foo(std::move(getX().z)), and partial moving examples...

Sorry for the really late response to this.

The delay is partially because I was busy with other projects and partially because I found it hard to decide how far to take this.

In the end, I implemented something that will only happen the very basic case where the base of the MemberExpr is the same as the argument of the std::move. This seems a common case that's worth handling correctly. (To make this work correctly, by the way, I had to change the logic that decides whether the use happens on a later loop iteration than the move. This was previously based on a purely syntactic criterion, but that no longer works in this case.)

There's a lot more that could be done, but I wonder a) how many of these cases are synthetic cases that don't turn up in reality, and b) you quickly get into territory where you need interprocedural analysis. For example:

A& id(A& a) { return a; }
A a;
a.bar(consumeA(std::move(a)));
id(a).bar(consumeA(std::move(a)));

We would like to treat the last two lines identically, as a use-after-move, but the second one is impossible to see without interprocedural analysis.

[PiotrZSL]

First, re-base code, looks like there were some changes in this check, and now there are conflicts with this path.
Second I don't any more comments, for me this code looks fine, BUT I'm not familiar too much with this check.
Check history for this check, and maybe consider adding to review some other people who modify it recently.

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

65–70

NOTE: This looks like llvm::any_of

74–79

NOTE: this looks like llvm::any_of, or even something like contains/find

129–133

NOTE: probably you could move call outside if, and do rest with single if...

clang-tools-extra/docs/ReleaseNotes.rst

167 ↗

(On Diff #514188)

NOTE: Probably better would be to keep similar template to rest of checks, just list fixes in sentences (or separated with ,), not as an list.

[mboehme]

Changes in response to review comments, and rebased to head.

[mboehme]

In D145581#4290839, @PiotrZSL wrote:

First, re-base code, looks like there were some changes in this check, and now there are conflicts with this path.

Done. (Not sure what the etiquette is with respect to rebasing -- that's why I kept it based on an old revision.)

Second I don't any more comments, for me this code looks fine, BUT I'm not familiar too much with this check.
Check history for this check, and maybe consider adding to review some other people who modify it recently.

Thanks, will do.

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

65–70

Done.

74–79

Done (with find).

129–133

Or as a single if statement like this?

clang-tools-extra/docs/ReleaseNotes.rst

167 ↗

(On Diff #514188)

Done.

[mboehme]

@MarcoFalke Would you be willing to review? (Thanks for your recent fix in D146288!)

[mboehme]

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

1355–1356

Rewriting the logic for the "use happens in a later loop iteration" has also eliminated this erroneous note.

[ymandel]

In D145581#4223656, @mboehme wrote:

In D145581#4223185, @PiotrZSL wrote:

And actually there is issue for this: https://github.com/llvm/llvm-project/issues/57758

Thanks for making me aware of this!

You can auto-link it by mentioning the issue in the commit message of the patch. If it fixes it, for example, add "Fixes #57758" and github will pick up on it automatically.

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

for (const Stmt *Parent : getParentStmts(Descendant, Context)) {
     if (isDescendantOrEqual(Parent, Ancestor, Context))
       return true;
   }
 
   return false;
 }
 } // namespace
 
 ExprSequence::ExprSequence(const CFG *TheCFG, const Stmt *Root,
                            ASTContext *TheContext)
     : Context(TheContext), Root(Root) {
   for (const auto &SyntheticStmt : TheCFG->synthetic_stmts()) {
     SyntheticStmtSourceMap[SyntheticStmt.first] = SyntheticStmt.second;
   }
 }
 
 bool ExprSequence::inSequence(const Stmt *Before, const Stmt *After) const {
   Before = resolveSyntheticStmt(Before);
   After = resolveSyntheticStmt(After);
 
   // If 'After' is in the subtree of the siblings that follow 'Before' in the
   // chain of successors, we know that 'After' is sequenced after 'Before'.
   for (const Stmt *Successor = getSequenceSuccessor(Before); Successor;
        Successor = getSequenceSuccessor(Successor)) {
     if (isDescendantOrEqual(After, Successor, Context))
       return true;
   }
 
+  SmallVector<const Stmt *, 1> BeforeParents = getParentStmts(Before, Context);
+
+  // Since C++17, the callee of a call expression is guaranteed to be sequenced
+  // before all of the arguments.
+  // We handle this as a special case rather than using the general
+  // `getSequenceSuccessor` logic above because the callee expression doesn't
+  // have an unambiguous successor; the order in which arguments are evaluated
+  // is indeterminate.
+  if (Context->getLangOpts().CPlusPlus17) {
+    for (const Stmt *Parent : BeforeParents) {
+      if (const auto *call = dyn_cast<CallExpr>(Parent);
+          call && call->getCallee() == Before) {
+        for (const Expr *arg : call->arguments()) {
+          if (isDescendantOrEqual(After, arg, Context))
+            return true;
+        }
+      }
+    }
+  }
+
   // If 'After' is a parent of 'Before' or is sequenced after one of these
   // parents, we know that it is sequenced after 'Before'.
-  for (const Stmt *Parent : getParentStmts(Before, Context)) {
+  for (const Stmt *Parent : BeforeParents) {
     if (Parent == After || inSequence(Parent, After))
       return true;
   }
 
   return false;
 }
 
 bool ExprSequence::potentiallyAfter(const Stmt *After,
                                     const Stmt *Before) const {
   return !inSequence(After, Before);
 }
 
 const Stmt *ExprSequence::getSequenceSuccessor(const Stmt *S) const {
   for (const Stmt *Parent : getParentStmts(S, Context)) {
     // If a statement has multiple parents, make sure we're using the parent
     // that lies within the sub-tree under Root.
     if (!isDescendantOrEqual(Parent, Root, Context))
       continue;
 
     if (const auto *BO = dyn_cast<BinaryOperator>(Parent)) {
       // Comma operator: Right-hand side is sequenced after the left-hand side.
       if (BO->getLHS() == S && BO->getOpcode() == BO_Comma)
         return BO->getRHS();
     } else if (const auto *InitList = dyn_cast<InitListExpr>(Parent)) {
       // Initializer list: Each initializer clause is sequenced after the
       // clauses that precede it.
       for (unsigned I = 1; I < InitList->getNumInits(); ++I) {
         if (InitList->getInit(I - 1) == S)
           return InitList->getInit(I);
       }
     } else if (const auto *Compound = dyn_cast<CompoundStmt>(Parent)) {
       // Compound statement: Each sub-statement is sequenced after the
       // statements that precede it.
       const Stmt *Previous = nullptr;

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

for (int i = 0; i < 10; ++i) {
     A a1;
     switch (A a2 = a1; A a3 = std::move(a2)) {
       case true:
         std::move(a3);
     }
   }
 }
 
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments.
+namespace CalleeSequencedBeforeArguments {
+struct A {
+  void foo(std::unique_ptr<A>) {}
+};
+void calleeSequencedBeforeArguments() {
+  std::unique_ptr<A> a;
+  a->foo(std::move(a));
+}
+} // namespace CalleeSequencedBeforeArguments
+
 // Some statements in templates (e.g. null, break and continue statements) may
 // be shared between the uninstantiated and instantiated versions of the
 // template and therefore have multiple parents. Make sure the sequencing code
 // handles this correctly.
 template <class> void nullStatementSequencesInTemplate() {
   int c = 0;
   (void)c;
   ;

 }
 void typeId() {
   Foo Bar;
   // error: you need to include <typeinfo> before using the 'typeid' operator
   // (void) typeid(Foo{std::move(Bar)}).name();
   Foo Other{std::move(Bar)};
 }
 } // namespace UnevalContext
 
 class PR38187 {
 public:
   PR38187(std::string val) : val_(std::move(val)) {
     val.empty();
     // CHECK-NOTES: [[@LINE-1]]:5: warning: 'val' used after it was moved
     // CHECK-NOTES: [[@LINE-3]]:30: note: move occurred here
   }
 
 private:
   std::string val_;
-};
+};
 No newline at end of file