mboehme updated this revision to Diff 516359.
mboehme marked 2 inline comments as done.
mboehme added a comment.
Changes in response to review comments, and rebased to head.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145581/new/
https://reviews.llvm.org/D145581
Files:
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -1,3 +1,4 @@
+// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
typedef decltype(nullptr) nullptr_t;
@@ -123,6 +124,7 @@
A &operator=(A &&);
void foo() const;
+ void bar(int i) const;
int getInt() const;
operator bool() const;
@@ -564,6 +566,19 @@
std::move(a);
}
}
+ // Same as above, but the use and the move are in different CFG blocks.
+ {
+ A a;
+ for (int i = 0; i < 10; ++i) {
+ if (i < 10)
+ a.foo();
+ // CHECK-NOTES: [[@LINE-1]]:9: warning: 'a' used after it was moved
+ // CHECK-NOTES: [[@LINE+3]]:9: note: move occurred here
+ // CHECK-NOTES: [[@LINE-3]]:9: note: the use happens in a later loop
+ if (i < 10)
+ std::move(a);
+ }
+ }
// However, this case shouldn't be flagged -- the scope of the declaration of
// 'a' is important.
{
@@ -1319,6 +1334,40 @@
}
}
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments -- but only in C++17 and later.
+namespace CalleeSequencedBeforeArguments {
+int consumeA(std::unique_ptr<A> a);
+int consumeA(A &&a);
+
+void calleeSequencedBeforeArguments() {
+ {
+ std::unique_ptr<A> a;
+ a->bar(consumeA(std::move(a)));
+ // CHECK-NOTES-CXX11: [[@LINE-1]]:5: warning: 'a' used after it was moved
+ // CHECK-NOTES-CXX11: [[@LINE-2]]:21: note: move occurred here
+ // CHECK-NOTES-CXX11: [[@LINE-3]]:5: note: the use and move are unsequenced
+ }
+ {
+ std::unique_ptr<A> a;
+ std::unique_ptr<A> getArg(std::unique_ptr<A> a);
+ getArg(std::move(a))->bar(a->getInt());
+ // CHECK-NOTES: [[@LINE-1]]:31: warning: 'a' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:12: note: move occurred here
+ // CHECK-NOTES-CXX11: [[@LINE-3]]:31: note: the use and move are unsequenced
+ }
+ {
+ A a;
+ // Nominally, the callee `a.bar` is evaluated before the argument
+ // `consumeA(std::move(a))`, but in effect `a` is only accessed after the
+ // call to `A::bar()` happens, i.e. after the argument has been evaluted.
+ a.bar(consumeA(std::move(a)));
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'a' used after it was moved
+ // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+ }
+}
+} // 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
@@ -1436,7 +1485,6 @@
// CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved
s{std::move(val)} {} // wrong order
// CHECK-NOTES: [[@LINE-1]]:9: note: move occurred here
- // CHECK-NOTES: [[@LINE-4]]:11: note: the use happens in a later loop iteration than the move
private:
bool a;
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -213,9 +213,12 @@
<clang-tidy/checks/bugprone/unchecked-optional-access>` check to properly handle calls
to ``std::forward``.
-- Improved :doc:`bugprone-use-after-move
- <clang-tidy/checks/bugprone/use-after-move>` check to also cover constructor
- initializers.
+- :doc:`bugprone-use-after-move
+ <clang-tidy/checks/bugprone/use-after-move>`: Improved check to also cover
+ constructor initializers. Fixed sequencing of designated initializers.
+ Fixed sequencing of callees: In C++17 and later, the callee of a function is
+ guaranteed to be sequenced before the arguments, so don't warn if the use
+ happens in the callee and the move happens in one of the arguments.
- Deprecated :doc:`cert-dcl21-cpp
<clang-tidy/checks/cert/dcl21-cpp>` check.
@@ -330,10 +333,6 @@
<clang-tidy/checks/cppcoreguidelines/slicing>` check when warning would be
emitted in constructor for virtual base class initialization.
-- Improved :doc:`bugprone-use-after-move
- <clang-tidy/checks/bugprone/use-after-move>` to understand that there is a
- sequence point between designated initializers.
-
- Fixed an issue in the :doc:`performance-noexcept-move-constructor
<clang-tidy/checks/performance/noexcept-move-constructor>` checker that was causing
false-positives when the move constructor or move assign operator were defaulted.
Index: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
+++ clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
@@ -55,12 +55,23 @@
ASTContext *Context) {
if (Descendant == Ancestor)
return true;
- for (const Stmt *Parent : getParentStmts(Descendant, Context)) {
- if (isDescendantOrEqual(Parent, Ancestor, Context))
- return true;
- }
+ return llvm::any_of(getParentStmts(Descendant, Context),
+ [Ancestor, Context](const Stmt *Parent) {
+ return isDescendantOrEqual(Parent, Ancestor, Context);
+ });
+}
- return false;
+bool isDescendantOfArgs(const Stmt *Descendant, const CallExpr *Call,
+ ASTContext *Context) {
+ return llvm::any_of(Call->arguments(),
+ [Descendant, Context](const Expr *Arg) {
+ return isDescendantOrEqual(Descendant, Arg, Context);
+ });
+}
+
+bool argsContain(const CallExpr *Call, const Stmt *TheStmt) {
+ return std::find(Call->arguments().begin(), Call->arguments().end(),
+ TheStmt) != Call->arguments().end();
}
llvm::SmallVector<const InitListExpr *>
@@ -95,9 +106,59 @@
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.
+ for (const Stmt *Parent : BeforeParents) {
+ // 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))`. Note that this is not specific to C++17, so
+ // we implement this logic unconditionally.
+ if (const auto *call = dyn_cast<CXXMemberCallExpr>(Parent)) {
+ if (argsContain(call, Before) &&
+ isa<DeclRefExpr>(
+ call->getImplicitObjectArgument()->IgnoreParenImpCasts()) &&
+ isDescendantOrEqual(After, call->getImplicitObjectArgument(),
+ Context))
+ return true;
+
+ // We need this additional early exit so that we don't fall through to the
+ // more general logic below.
+ if (auto *Member = dyn_cast<MemberExpr>(Before);
+ Member && call->getCallee() == Member &&
+ isa<DeclRefExpr>(Member->getBase()->IgnoreParenImpCasts()) &&
+ isDescendantOfArgs(After, call, Context))
+ return false;
+ }
+
+ if (!Context->getLangOpts().CPlusPlus17)
+ continue;
+
+ if (const auto *call = dyn_cast<CallExpr>(Parent);
+ call && call->getCallee() == Before &&
+ isDescendantOfArgs(After, call, 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;
}
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -14,6 +14,7 @@
#include "clang/Analysis/CFG.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "../utils/ExprSequence.h"
#include "../utils/Matchers.h"
@@ -35,6 +36,9 @@
// Is the order in which the move and the use are evaluated undefined?
bool EvaluationOrderUndefined;
+
+ // Does the use happen in a later loop iteration than the move?
+ bool UseHappensInLaterLoopIteration;
};
/// Finds uses of a variable after a move (and maintains state required by the
@@ -48,7 +52,7 @@
// use-after-move is found, writes information about it to 'TheUseAfterMove'.
// Returns whether a use-after-move was found.
bool find(Stmt *CodeBlock, const Expr *MovingCall,
- const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove);
+ const DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove);
private:
bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
@@ -69,6 +73,30 @@
llvm::SmallPtrSet<const CFGBlock *, 8> Visited;
};
+/// Returns whether the `Before` block can reach the `After` block.
+bool reaches(const CFGBlock *Before, const CFGBlock *After) {
+ llvm::SmallVector<const CFGBlock *> Stack;
+ llvm::SmallPtrSet<const CFGBlock *, 1> Visited;
+
+ Stack.push_back(Before);
+ while (!Stack.empty()) {
+ const CFGBlock *Current = Stack.back();
+ Stack.pop_back();
+
+ if (Current == After)
+ return true;
+
+ Visited.insert(Current);
+
+ for (const auto &Succ : Current->succs()) {
+ if (!Visited.count(Succ))
+ Stack.push_back(Succ);
+ }
+ }
+
+ return false;
+}
+
} // namespace
// Matches nodes that are
@@ -89,7 +117,7 @@
: Context(TheContext) {}
bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
- const ValueDecl *MovedVariable,
+ const DeclRefExpr *MovedVariable,
UseAfterMove *TheUseAfterMove) {
// Generate the CFG manually instead of through an AnalysisDeclContext because
// it seems the latter can't be used to generate a CFG for the body of a
@@ -110,15 +138,31 @@
BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Context);
Visited.clear();
- const CFGBlock *Block = BlockMap->blockContainingStmt(MovingCall);
- if (!Block) {
+ const CFGBlock *MoveBlock = BlockMap->blockContainingStmt(MovingCall);
+ if (!MoveBlock) {
// This can happen if MovingCall is in a constructor initializer, which is
// not included in the CFG because the CFG is built only from the function
// body.
- Block = &TheCFG->getEntry();
+ MoveBlock = &TheCFG->getEntry();
}
- return findInternal(Block, MovingCall, MovedVariable, TheUseAfterMove);
+ bool found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
+ TheUseAfterMove);
+
+ if (found) {
+ if (const CFGBlock *UseBlock =
+ BlockMap->blockContainingStmt(TheUseAfterMove->DeclRef))
+ // Does the use happen in a later loop iteration than the move?
+ // - If they are in the same CFG block, we know the use happened in a
+ // later iteration if we visited that block a second time.
+ // - Otherwise, we know the use happened in a later iteration if the
+ // move is reachable from the use.
+ TheUseAfterMove->UseHappensInLaterLoopIteration =
+ UseBlock == MoveBlock ? Visited.count(UseBlock) != 0
+ : reaches(UseBlock, MoveBlock);
+ }
+
+ return found;
}
bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
@@ -175,6 +219,10 @@
MovingCall != nullptr &&
Sequence->potentiallyAfter(MovingCall, Use);
+ // We default to false here and change this to true if required in
+ // find().
+ TheUseAfterMove->UseHappensInLaterLoopIteration = false;
+
return true;
}
}
@@ -373,7 +421,7 @@
"the use and move are unsequenced, i.e. there is no guarantee "
"about the order in which they are evaluated",
DiagnosticIDs::Note);
- } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) {
+ } else if (Use.UseHappensInLaterLoopIteration) {
Check->diag(UseLoc,
"the use happens in a later loop iteration than the move",
DiagnosticIDs::Note);
@@ -469,7 +517,7 @@
for (Stmt *CodeBlock : CodeBlocks) {
UseAfterMoveFinder Finder(Result.Context);
UseAfterMove Use;
- if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use))
+ if (Finder.find(CodeBlock, MovingCall, Arg, &Use))
emitDiagnostic(MovingCall, Arg, Use, this, Result.Context);
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
