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.
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
-Wunsequenced should warn on "std::unique_ptr p; f(p.get(), std::move(p));" #21193
Comments
-Wunsequenced is probably not the right warning flag (the operations are sequenced, but sequenced indeterminately), but this seems like a good warning to have. I've seen similar bugs that look like: my_map[p->foo()] = p.release(); The tricky part is avoiding false positives. I tried doing this with a heuristic "calling a non-const member function is a modification; indeterminately-sequenced modifications and accesses gets a warning" but the false positive rate is extremely high. We could special-case std::move, but it'd be better to implement something more general. Ideas? |
One idea is to remember which parameters of a function are converted to rvalue references in the function, and then at the call site consider objects passed in these parameters as dead. This would generalize std::move, but it wouldn't work for your example :-/ |
As C++11 becomes more common, I think rvalue references may be the way to go. For Chromium's pre-C++11 codebase, I think we should add a custom check for scoped_ptr and base::Passed to the Chromium plugin. |
For the p.release() thing in comment 1, maybe functions that are marked warn_unused_result should be considered to poison an object, and accesses to the same object without sequence point should trigger the warning? |
*** Bug llvm/llvm-bugzilla-archive#27562 has been marked as a duplicate of this bug. *** |
From the dupe: """Maybe this could work similar to -Wuninitialized where we conceptually treat things that have been move()d from as uninitialized from that point on""" (yes, nothing guarantees that objects are uninitialized after they're moved from, but in practice they usually are. this is a fairly common problem; we should come up with some approach) |
wip |
We just hit a similar case and would much appreciate having this warning. In struct foo { Clang warns about the "unsequenced modification", but if the int is replaced with a struct that is moved, no warning is issued: #include |
clang-tidy got this bugprone-use-after-move check here: It seems to heavily use AST matchers, which wouldn't be allowed in clang proper. This is a pretty popular feature request, so even if it gets added as an off-by-default warning, I think this would be valuable. Then we could reassess enabling it by default once we better understand the false positive rate. |
Sorry for going back and forth, but the standard is particularly hard to read around this. In the case clang does warn: struct foo { Isn't https://eel.is/c++draft/expr.call#8 guaranteeing that the "get_foo(a)" is evaluated before "a++"? |
Yes; that's a bug in this warning in -std=c++17 and later. SequenceChecker::VisitCallExpr has no idea that this sequencing exists. (But it does know about the other C++17 sequencing changes... I guess we overlooked this one.) |
The patch for this rule is in D58579. I wrote it at the same time I implemented the other C++17 rules in SequenceChecker but this one has been stuck for a while. |
mentioned in issue llvm/llvm-bugzilla-archive#27562 |
Extended Description
-Wunsequenced should warn on this:
#include
void f(int *p, std::unique_ptr foo) {}
int main() {
std::unique_ptr p;
f(p.get(), std::move(p));
}
(motivated by http://crbug.com/409318)
The text was updated successfully, but these errors were encountered: