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