This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments.
Needs ReviewPublic

Authored by mboehme on Mar 8 2023, 5:06 AM.
Tokens
"Like" token, awarded by PiotrZSL.

Details

Summary

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

We introduce one special case: If the callee is a MemberExpr with a DeclRefExpr as its base, we consider it to be sequenced after the arguments. This is because the variable referenced in the base will only actually be accessed when the call happens, i.e. once all of the arguments have been evaluated. This has no basis in the C++ standard, but it reflects actual behavior that is relevant to a use-after-move scenario:

a.bar(consumeA(std::move(a));

In this example, we end up accessing a after it has been moved from, even though nominally the callee a.bar is evaluated before the argument consumeA(std::move(a)).

Treating this scenario correctly has required rewriting the logic in bugprone-use-after-move that governs whether the use happens in a later loop iteration than the move. This was previously based on an unsound heuristic (does the use come lexically before the move?); we now use a more rigourous criterion based on reachability in the CFG.

Diff Detail

Event Timeline

mboehme created this revision.Mar 8 2023, 5:06 AM
Herald added a project: Restricted Project. · View Herald Transcript
mboehme requested review of this revision.Mar 8 2023, 5:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 5:06 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1407

Please add.

PiotrZSL added inline comments.Mar 8 2023, 8:21 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1312

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

What about scenario like this:

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

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

1321

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

std::unique_ptr<A> getArg(std::unique_ptr<A>);
getArg(std::move(a))->foo(std::move(a));
PiotrZSL requested changes to this revision.Mar 8 2023, 10:22 PM

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

This revision now requires changes to proceed.Mar 8 2023, 10:22 PM
mboehme updated this revision to Diff 506507.Mar 20 2023, 2:46 AM

Changes in response to review comments.

mboehme updated this revision to Diff 506510.Mar 20 2023, 2:51 AM

Added entry to release notes.

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

clang-tools-extra/docs/ReleaseNotes.rst
224

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

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1312

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.

1320

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.

1321

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.

PiotrZSL added inline comments.Mar 20 2023, 3:17 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1320

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

mboehme added inline comments.Mar 21 2023, 4:58 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1320

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.

PiotrZSL added inline comments.Mar 21 2023, 6:00 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1320

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...

PiotrZSL requested changes to this revision.Mar 23 2023, 12:41 AM

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

This revision now requires changes to proceed.Mar 23 2023, 12:41 AM

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.

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

Thanks for making me aware of this!

Eugene.Zelenko added inline comments.Mar 27 2023, 7:23 AM
clang-tools-extra/docs/ReleaseNotes.rst
222

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

mboehme updated this revision to Diff 514182.Apr 17 2023, 5:59 AM

Changes in reponse to review comments.

mboehme edited the summary of this revision. (Show Details)Apr 17 2023, 6:01 AM
mboehme updated this revision to Diff 514188.Apr 17 2023, 6:06 AM

Changes in response to review comments

mboehme marked 3 inline comments as done.Apr 17 2023, 6:19 AM

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.

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.

clang-tools-extra/docs/ReleaseNotes.rst
222

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

Done.

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1320

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.

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.

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
68–73
NOTE: This looks like llvm::any_of
77–82
NOTE: this looks like llvm::any_of, or even something like contains/find
142–146
NOTE: probably you could move call outside if, and do rest with single if...
clang-tools-extra/docs/ReleaseNotes.rst
167
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.
mboehme updated this revision to Diff 516359.Apr 24 2023, 5:04 AM
mboehme marked 2 inline comments as done.

Changes in response to review comments, and rebased to head.

mboehme marked 4 inline comments as done.Apr 24 2023, 5:06 AM

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.

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp
68–73

Done.

77–82

Done (with find).

142–146

Or as a single if statement like this?

clang-tools-extra/docs/ReleaseNotes.rst
167

Done.

mboehme marked 4 inline comments as done.

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

mboehme added inline comments.Apr 24 2023, 5:14 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
1369–1370

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

ymandel added a subscriber: ymandel.May 4 2023, 6:04 AM

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.