[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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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
-  <clang-tidy/checks/bugprone/use-after-move>` to understand that there is a
-  sequence point between designated initializers.
+- In :doc:`bugprone-use-after-move
+  <clang-tidy/checks/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;
+  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 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...
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145581