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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
