Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clang-tidy: bugprone-use-after-move: false negative #59612

Closed
JVApen opened this issue Dec 20, 2022 · 4 comments · Fixed by #93623
Closed

Clang-tidy: bugprone-use-after-move: false negative #59612

JVApen opened this issue Dec 20, 2022 · 4 comments · Fixed by #93623
Labels
clang-tidy false-positive Warning fires when it should not

Comments

@JVApen
Copy link

JVApen commented Dec 20, 2022 •
edited by VoltrexKeyva
Loading

Reproduction:

// https://compiler-explorer.com/z/8asKxxeGh
// clang++ -std=c++20  -stdlib=libc++  -Weverything -Wno-c++98-compat -Wno-missing-prototypes t.cpp
// clang-tidy -checks=bugprone-use-after-move
#include <utility>

struct C {};

struct S {
    auto g(C c) -> void;
};

auto f(C &c) -> S;

int main(int, char**) {
    auto c = C{};
    f(c).g(std::move(c));
}

Expected:

[<source>:13:7: warning: 'c' used after it was moved [bugprone-use-after-move]]
    f(c).g(std::move(c));
      ^
[<source>:13:12: note: move occurred here]
    f(c).g(std::move(c));
           ^
[<source>:13:7: note: the use and move are unsequenced, i.e. there is no guarantee about the order in which they are evaluated]
    f(c).g(std::move(c));
      ^
1 warning generated.

When the argument of f is changed from C &c to either const C &c or C c, the expected warning shows up. However, when the class is an (in/)out argument, this does not show. I don't believe there is a difference in the sequence of execution for these cases, so the warning should happen in all cases.
@EugeneZelenko EugeneZelenko added clang-tidy false-positive Warning fires when it should not and removed new issue labels Dec 20, 2022
@llvmbot
Copy link

llvmbot commented Dec 20, 2022

@llvm/issue-subscribers-clang-tidy

@dotnwat
Copy link
Contributor

dotnwat commented May 25, 2023 •
edited
Loading

I was under the impression that following c++17 this was safe. @EugeneZelenko just wondering if you could confirm that, which I think is what your false-positive flag is indicating.

@dotnwat dotnwat mentioned this issue May 25, 2023
Merged
7 tasks
@JVApen
Copy link
Author

JVApen commented May 25, 2023

@dotnwat You might be right about that. I forgot about that change. Based on that, it implies this isn't a false negative but a false positive.

I've checked the proposals:
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0145r3.pdf

In summary, the following expressions are evaluated in the order a, then b, then c, then d:

  1. a.b
  2. a->b
    ...

So this indeed seems like 2 false positives for C++17 and above, and 1 false negative for C++14 and below.

@PiotrZSL
Copy link
Member

https://reviews.llvm.org/D145581
Reviewers wanted...

@tchaikov tchaikov mentioned this issue May 10, 2024
Open
tchaikov pushed a commit to tchaikov/llvm-project that referenced this issue May 29, 2024
@martinboehme @tchaikov
[clang-tidy] Let bugprone-use-after-move ignore the moved variable in…
5cbb966 
… callee

In C++17, callee is guaranteed to be sequenced before arguments.

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.

Fixes llvm#57758
Fixes llvm#59612
@tchaikov tchaikov mentioned this issue May 29, 2024
Merged
tchaikov pushed a commit to tchaikov/llvm-project that referenced this issue May 29, 2024
@martinboehme @tchaikov
[clang-tidy] Let bugprone-use-after-move ignore the moved variable in…
3f1ef81 
… callee

In C++17, callee is guaranteed to be sequenced before arguments.

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.

Fixes llvm#57758
Fixes llvm#59612
tchaikov pushed a commit to tchaikov/llvm-project that referenced this issue May 30, 2024
@martinboehme @tchaikov
[clang-tidy] Let bugprone-use-after-move ignore the moved variable in…
e9e03e8 
… callee

In C++17, callee is guaranteed to be sequenced before arguments.

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.

Fixes llvm#57758
Fixes llvm#59612
tchaikov pushed a commit to tchaikov/llvm-project that referenced this issue May 31, 2024
@martinboehme @tchaikov
[clang-tidy] Let bugprone-use-after-move ignore the moved variable in…
14106f8 
… callee

In C++17, callee is guaranteed to be sequenced before arguments.

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.

Fixes llvm#57758
Fixes llvm#59612
tchaikov pushed a commit to tchaikov/llvm-project that referenced this issue Jun 8, 2024
@martinboehme @tchaikov
[clang-tidy] Let bugprone-use-after-move ignore the moved variable in…
81df014 
… callee

In C++17, callee is guaranteed to be sequenced before arguments.

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.

Fixes llvm#57758
Fixes llvm#59612
tchaikov pushed a commit to tchaikov/llvm-project that referenced this issue Jun 8, 2024
@martinboehme @tchaikov
[clang-tidy] Let bugprone-use-after-move ignore the moved variable in…
cb1dfa3 
… callee

In C++17, callee is guaranteed to be sequenced before arguments.

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.

Fixes llvm#57758
Fixes llvm#59612
tchaikov pushed a commit to tchaikov/llvm-project that referenced this issue Jun 8, 2024
@martinboehme @tchaikov
[clang-tidy] Let bugprone-use-after-move ignore the moved variable in…
00df701 
… callee

In C++17, callee is guaranteed to be sequenced before arguments.

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.

Fixes llvm#57758
Fixes llvm#59612
tchaikov pushed a commit to tchaikov/llvm-project that referenced this issue Jun 8, 2024
@martinboehme @tchaikov
[clang-tidy] Let bugprone-use-after-move ignore the moved variable in…
e07681e 
… callee

In C++17, callee is guaranteed to be sequenced before arguments.

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.

Fixes llvm#57758
Fixes llvm#59612
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
clang-tidy false-positive Warning fires when it should not
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[clang-tidy] In C++17, callee is guaranteed to be sequenced before arguments. tchaikov/llvm-project
5 participants
@dotnwat @PiotrZSL @JVApen @EugeneZelenko @llvmbot