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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1439
   // 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
 

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 4 inline comments as done.
mboehme added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 4 inline comments as done.
mboehme added a comment.

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.




Comment at: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp:68-73
+  for (const Expr *Arg : Call->arguments()) {
+if (isDescendantOrEqual(Descendant, Arg, Context))
+  return true;
+  }
+
+  return false;

PiotrZSL wrote:
> NOTE: This looks like llvm::any_of 
Done.



Comment at: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp:77-82
+  for (const Expr *Arg : Call->arguments()) {
+if (Arg == TheStmt)
+  return true;
+  }
+
+  return false;

PiotrZSL wrote:
> NOTE: this looks like llvm::any_of, or even something like contains/find
Done (with find).



Comment at: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp:161-165
+if (const auto *call = dyn_cast(Parent);
+call && call->getCallee() == Before) {
+  if (isDescendantOfArgs(After, call, Context))
+return true;
+}

PiotrZSL wrote:
> NOTE: probably you could move call outside if, and do rest with single if...
Or as a single if statement like this?



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:167
+  `:
+  - Fixed handling for designated initializers.
+  - Fix: In C++17 and later, the callee of a function is guaranteed to be

PiotrZSL wrote:
> 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.
Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

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 =(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);
+int consumeA(A &);
+
+void calleeSequencedBeforeArguments() {
+  {
+std::unique_ptr 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;
+std::unique_ptr getArg(std::unique_ptr 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 @@
   ` check to properly handle calls
   to ``std::forward``.
 
-- Improved :doc:`bugprone-use-after-move
-  ` check to also cover constructor
-  initializers.
+- :doc:`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
   ` check.
@@ -330,10 +333,6 @@
   ` check when warning would be
   emitted in constructor for virtual base class initialization.
 
-- Improved :doc:`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
   ` checker 

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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

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.




Comment at: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp:68-73
+  for (const Expr *Arg : Call->arguments()) {
+if (isDescendantOrEqual(Descendant, Arg, Context))
+  return true;
+  }
+
+  return false;

NOTE: This looks like llvm::any_of 



Comment at: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp:77-82
+  for (const Expr *Arg : Call->arguments()) {
+if (Arg == TheStmt)
+  return true;
+  }
+
+  return false;

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



Comment at: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp:161-165
+if (const auto *call = dyn_cast(Parent);
+call && call->getCallee() == Before) {
+  if (isDescendantOfArgs(After, call, Context))
+return true;
+}

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



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:167
+  `:
+  - Fixed handling for designated initializers.
+  - Fix: In C++17 and later, the callee of a function is guaranteed to be

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 3 inline comments as done.
mboehme added a comment.

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.




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:222
 
-- Improved :doc:`bugprone-use-after-move
-  ` to understand that there is a
-  sequence point between designated initializers.
+- In :doc:`bugprone-use-after-move
+  `:

Eugene.Zelenko wrote:
> Please keep alphabetical order (by check name) in this section.
> Please keep alphabetical order (by check name) in this section.

Done.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1304
+  std::unique_ptr a;
+  a->foo(std::move(a));
+}

PiotrZSL wrote:
> mboehme wrote:
> > PiotrZSL wrote:
> > > mboehme wrote:
> > > > PiotrZSL wrote:
> > > > > What about scenario like this:
> > > > > 
> > > > > ```
> > > > > b.foo(a->saveBIntoAAndReturnBool(std::move(b)));
> > > > > ```
> > > > > 
> > > > > Is first "b" still guaranteed to be alive after std::move ?
> > > > 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.
> > > 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
> > > 
> > > 
> > 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.
> 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 

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 514188.
mboehme added a comment.

Changes in response to review comments


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 =(A &&);
 
   void foo() const;
+  void bar(int i) const;
   int getInt() const;
 
   operator bool() const;
@@ -552,6 +554,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.
   {
@@ -1307,6 +1322,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);
+int consumeA(A &);
+
+void calleeSequencedBeforeArguments() {
+  {
+std::unique_ptr 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;
+std::unique_ptr getArg(std::unique_ptr 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
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -162,6 +162,13 @@
   ` check.
   Global options of the same name should be used instead.
 
+- In :doc:`bugprone-use-after-move
+  `:
+  - Fixed handling for designated initializers.
+  - Fix: 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 check-local options `HeaderFileExtensions`
   in :doc:`google-build-namespaces
   ` check.
@@ -219,10 +226,6 @@
   ` check when warning would be
   emitted in constructor for virtual base class initialization.
 
-- Improved :doc:`bugprone-use-after-move
-  ` to understand that there is a
-  sequence point between designated initializers.
-
 Removed checks
 ^^
 
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
@@ -63,6 +63,25 @@
   return false;
 }
 
+bool isDescendantOfArgs(const Stmt *Descendant, const CallExpr *Call,
+ASTContext *Context) {
+  for (const Expr *Arg : Call->arguments()) {
+if (isDescendantOrEqual(Descendant, Arg, Context))
+  return true;
+  }
+
+  

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 514182.
mboehme added a comment.

Changes in reponse to review comments.


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 =(A &&);
 
   void foo() const;
+  void bar(int i) const;
   int getInt() const;
 
   operator bool() const;
@@ -552,6 +554,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.
   {
@@ -1307,6 +1322,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);
+int consumeA(A &);
+
+void calleeSequencedBeforeArguments() {
+  {
+std::unique_ptr 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;
+std::unique_ptr getArg(std::unique_ptr 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
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -219,9 +219,12 @@
   ` check when warning would be
   emitted in constructor for virtual base class initialization.
 
-- Improved :doc:`bugprone-use-after-move
-  ` to understand that there is a
-  sequence point between designated initializers.
+- In :doc:`bugprone-use-after-move
+  `:
+  - Fixed handling for designated initializers.
+  - Fix: 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.
 
 Removed checks
 ^^
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
@@ -63,6 +63,25 @@
   return false;
 }
 
+bool isDescendantOfArgs(const Stmt *Descendant, const CallExpr *Call,
+ASTContext *Context) {
+  for (const Expr *Arg : Call->arguments()) {
+if (isDescendantOrEqual(Descendant, Arg, Context))
+  return true;
+  }
+
+  return false;
+}
+
+bool argsContain(const CallExpr *Call, const Stmt *TheStmt) {
+  for (const Expr *Arg : Call->arguments()) {
+if (Arg == TheStmt)
+  return true;
+  }
+
+  return false;
+}
+
 

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:222
 
-- Improved :doc:`bugprone-use-after-move
-  ` to understand that there is a
-  sequence point between designated initializers.
+- In :doc:`bugprone-use-after-move
+  `:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL requested changes to this revision.
PiotrZSL added a comment.
This revision now requires changes to proceed.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1304
+  std::unique_ptr a;
+  a->foo(std::move(a));
+}

mboehme wrote:
> PiotrZSL wrote:
> > mboehme wrote:
> > > PiotrZSL wrote:
> > > > What about scenario like this:
> > > > 
> > > > ```
> > > > b.foo(a->saveBIntoAAndReturnBool(std::move(b)));
> > > > ```
> > > > 
> > > > Is first "b" still guaranteed to be alive after std::move ?
> > > 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.
> > 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
> > 
> > 
> 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.
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...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1304
+  std::unique_ptr a;
+  a->foo(std::move(a));
+}

PiotrZSL wrote:
> mboehme wrote:
> > PiotrZSL wrote:
> > > What about scenario like this:
> > > 
> > > ```
> > > b.foo(a->saveBIntoAAndReturnBool(std::move(b)));
> > > ```
> > > 
> > > Is first "b" still guaranteed to be alive after std::move ?
> > 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.
> 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
> 
> 
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1304
+  std::unique_ptr a;
+  a->foo(std::move(a));
+}

mboehme wrote:
> PiotrZSL wrote:
> > What about scenario like this:
> > 
> > ```
> > b.foo(a->saveBIntoAAndReturnBool(std::move(b)));
> > ```
> > 
> > Is first "b" still guaranteed to be alive after std::move ?
> 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.
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




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

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




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:224
+  `:
+  - Fixed handling for designated initializers.
+  - Fix: In C++17 and later, the callee of a function is guaranteed to be

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



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1296
 
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments.

PiotrZSL wrote:
> 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));
> ```
> 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.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1304
+  std::unique_ptr a;
+  a->foo(std::move(a));
+}

PiotrZSL wrote:
> What about scenario like this:
> 
> ```
> b.foo(a->saveBIntoAAndReturnBool(std::move(b)));
> ```
> 
> Is first "b" still guaranteed to be alive after std::move ?
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.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1305
+  a->foo(std::move(a));
+}
+} // namespace CalleeSequencedBeforeArguments

PiotrZSL wrote:
> I didn't found any test for correct warning in this case:
> 
> ```
> std::unique_ptr getArg(std::unique_ptr);
> getArg(std::move(a))->foo(std::move(a));
> ```
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 506510.
mboehme added a comment.

Added entry to release notes.


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/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 =(A &&);
 
   void foo() const;
+  void bar(int i) const;
   int getInt() const;
 
   operator bool() const;
@@ -1307,6 +1309,30 @@
   }
 }
 
+// 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);
+
+void calleeSequencedBeforeArguments() {
+  {
+std::unique_ptr 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;
+std::unique_ptr getArg(std::unique_ptr 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
+  }
+}
+} // 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
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -219,9 +219,12 @@
   ` check when warning would be
   emitted in constructor for virtual base class initialization.
 
-- Improved :doc:`bugprone-use-after-move
-  ` to understand that there is a
-  sequence point between designated initializers.
+- In :doc:`bugprone-use-after-move
+  `:
+  - Fixed handling for designated initializers.
+  - Fix: 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.
 
 Removed checks
 ^^
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
@@ -95,9 +95,29 @@
   return true;
   }
 
+  SmallVector 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(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;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 506507.
mboehme added a comment.

Changes in response to review comments.


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/utils/ExprSequence.cpp
  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 =(A &&);
 
   void foo() const;
+  void bar(int i) const;
   int getInt() const;
 
   operator bool() const;
@@ -1307,6 +1309,30 @@
   }
 }
 
+// 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);
+
+void calleeSequencedBeforeArguments() {
+  {
+std::unique_ptr 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;
+std::unique_ptr getArg(std::unique_ptr 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
+  }
+}
+} // 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
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
@@ -95,9 +95,29 @@
   return true;
   }
 
+  SmallVector 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(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;
   }


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 =(A &&);
 
   void foo() const;
+  void bar(int i) const;
   int getInt() const;
 
   operator bool() const;
@@ -1307,6 +1309,30 @@
   }
 }
 
+// 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);
+
+void calleeSequencedBeforeArguments() {
+  {
+std::unique_ptr 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
+// 

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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL requested changes to this revision.
PiotrZSL added a comment.
This revision now requires changes to proceed.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1296
 
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments.

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));
```



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1304
+  std::unique_ptr a;
+  a->foo(std::move(a));
+}

What about scenario like this:

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

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



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1305
+  a->foo(std::move(a));
+}
+} // namespace CalleeSequencedBeforeArguments

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

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1379
+};
\ No newline at end of file


Please add.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145581/new/

https://reviews.llvm.org/D145581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Böhme via Phabricator via cfe-commits]

mboehme created this revision.
Herald added subscribers: PiotrZSL, carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
mboehme requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145581

Files:
  clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
  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
@@ -1293,6 +1293,18 @@
   }
 }
 
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments.
+namespace CalleeSequencedBeforeArguments {
+struct A {
+  void foo(std::unique_ptr) {}
+};
+void calleeSequencedBeforeArguments() {
+  std::unique_ptr 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
@@ -1363,4 +1375,4 @@
 
 private:
   std::string val_;
-};
+};
\ No newline at end of file
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
@@ -82,9 +82,29 @@
   return true;
   }
 
+  SmallVector 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(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;
   }


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
@@ -1293,6 +1293,18 @@
   }
 }
 
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments.
+namespace CalleeSequencedBeforeArguments {
+struct A {
+  void foo(std::unique_ptr) {}
+};
+void calleeSequencedBeforeArguments() {
+  std::unique_ptr 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
@@ -1363,4 +1375,4 @@
 
 private:
   std::string val_;
-};
+};
\ No newline at end of file
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
@@ -82,9 +82,29 @@
   return true;
   }
 
+  SmallVector 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(Parent);
+  call && call->getCallee() == Before) {
+for (const Expr *arg : call->arguments()) {
+  if