[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;
+  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