[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348210: [analyzer] MoveChecker: Restrict to locals and std:: 
objects. (authored by dergachev, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D54557?vs=174475=176491#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54557/new/

https://reviews.llvm.org/D54557

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -4,6 +4,14 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
+// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
+// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
 
 namespace std {
 
@@ -36,6 +44,13 @@
   b = std::move(c);
 }
 
+template 
+class vector {
+public:
+  vector();
+  void push_back(const T );
+};
+
 } // namespace std
 
 class B {
@@ -71,7 +86,10 @@
 moveconstruct(std::move(*a));
   }
   A(const A ) : i(other.i), d(other.d), b(other.b) {}
-  A(A &) : i(other.i), d(other.d), b(std::move(other.b)) { // expected-note {{'b' became 'moved-from' here}}
+  A(A &) : i(other.i), d(other.d), b(std::move(other.b)) {
+#ifdef AGGRESSIVE
+// expected-note@-2{{'b' became 'moved-from' here}}
+#endif
   }
   A(A &, char *k) {
 moveconstruct(std::move(other));
@@ -424,26 +442,51 @@
 
   void f() {
 A b;
-b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
-a.foo();  // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object 'a'}}
+b = std::move(a);
+a.foo();
+#ifdef AGGRESSIVE
+// expected-note@-3{{'a' became 'moved-from' here}}
+// expected-warning@-3 {{Method call on a 'moved-from' object 'a'}}
+// expected-note@-4{{Method call on a 'moved-from' object 'a'}}
+#endif
 
-b = std::move(static_a); // expected-note {{'static_a' became 'moved-from' here}}
-static_a.foo();  // expected-warning {{Method call on a 'moved-from' object 'static_a'}} expected-note {{Method call on a 'moved-from' object 'static_a'}}
+b = std::move(static_a);
+static_a.foo();
+#ifdef AGGRESSIVE
+// expected-note@-3{{'static_a' became 'moved-from' here}}
+// expected-warning@-3{{Method call on a 'moved-from' object 'static_a'}}
+// expected-note@-4{{Method call on a 'moved-from' object 'static_a'}}
+#endif
   }
 };
 
 void PtrAndArrayTest() {
   A *Ptr = new A(1, 1.5);
   A Arr[10];
-  Arr[2] = std::move(*Ptr); // expected-note {{Became 'moved-from' here}}
-  (*Ptr).foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}}
+  Arr[2] = std::move(*Ptr);
+  (*Ptr).foo();
+#ifdef AGGRESSIVE
+  // expected-note@-3{{Became 'moved-from' here}}
+  // expected-warning@-3{{Method call on a 'moved-from' object}}
+  // expected-note@-4{{Method call on a 'moved-from' object}}
+#endif
 
   Ptr = [1];
-  Arr[3] = std::move(Arr[1]); // expected-note {{Became 'moved-from' here}}
-  Ptr->foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}}
+  Arr[3] = std::move(Arr[1]);
+  Ptr->foo();
+#ifdef AGGRESSIVE
+  // expected-note@-3{{Became 'moved-from' here}}
+  // expected-warning@-3{{Method call on a 'moved-from' object}}
+  // expected-note@-4{{Method call on a 'moved-from' object}}
+#endif
 
-  Arr[3] = std::move(Arr[2]); // expected-note {{Became 'moved-from' here}}
-  Arr[2].foo();   // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}}
+  Arr[3] = std::move(Arr[2]);
+  Arr[2].foo();
+#ifdef AGGRESSIVE
+  // expected-note@-3{{Became 'moved-from' here}}
+  // expected-warning@-3{{Method call on a 'moved-from' object}}
+  // expected-note@-4{{Method call on a 'moved-from' object}}
+#endif
 
   Arr[2] = std::move(Arr[3]); // reinitialization
   Arr[2].foo();   // no-warning
@@ -649,13 +692,24 @@
 void subRegionMoveTest() {
   {
 A a;
-B b = std::move(a.b); // expected-note {{'b' became 'moved-from' here}}
-

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.

In D54557#1313382 , @NoQ wrote:

> In D54557#1301896 , @Szelethus wrote:
>
> > Wasn't the point of this patch to turn off part of this checkers 
> > functionality because it's immature just yet? From what I understand it is 
> > desired, but the FP rate is a little too high. I guess fixing that is the 
> > project.
> >
> > Hmm, actually, tinkering with HTML files might be overkill, especially 
> > since sphinx will hopefully end that era. Let's just add a TODO and let me 
> > deal with it later when I reorganize the checker options. Sorry for talking 
> > nonsense :D
>
>
> I would probably fine with shipping this other part as a lint check if we 
> start shipping lint checks. I don't have any specific improvements in mind 
> for that part. If somebody is willing to enforce "don't ever use things after 
> move" policy in his code, this will be a good checker for such person. At the 
> same time, i don't see how to improve that part so much that it can be 
> enabled by default. I think projects that consist of "i believe it's 
> impossible but you can try to come up with something" kind aren't very good 
> for the open projects list because the person who will try to work on them 
> may get disappointed when he realizes that his few at-a-glance solutions are 
> bad and in fact nobody ever had any better solutions in mind.


Yea, that makes sense.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54557/new/

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D54557#1301896 , @Szelethus wrote:

> Wasn't the point of this patch to turn off part of this checkers 
> functionality because it's immature just yet? From what I understand it is 
> desired, but the FP rate is a little too high. I guess fixing that is the 
> project.
>
> Hmm, actually, tinkering with HTML files might be overkill, especially since 
> sphinx will hopefully end that era. Let's just add a TODO and let me deal 
> with it later when I reorganize the checker options. Sorry for talking 
> nonsense :D


I would probably fine with shipping this other part as a lint check if we start 
shipping lint checks. I don't have any specific improvements in mind for that 
part. If somebody is willing to enforce "don't ever use things after move" 
policy in his code, this will be a good checker for such person. At the same 
time, i don't see how to improve that part so much that it can be enabled by 
default. I think projects that consist of "i believe it's impossible but you 
can try to come up with something" kind aren't very good for the open projects 
list because the person who will try to work on them may get disappointed when 
he realizes that his few at-a-glance solutions are bad and in fact nobody ever 
had any better solutions in mind.

In D54557#1302069 , @xazax.hun wrote:

> In D54557#1300654 , @NoQ wrote:
>
> > In D54557#1299736 , @xazax.hun 
> > wrote:
> >
> > > It would be great to have a way to extend the list of (possibly non-stl) 
> > > types to check. But I do understand that the analyzer does not have a 
> > > great way to set such configuration options right now.
> >
> >
> > Do you envision room for another attribute here? I.e., a class attribute 
> > that says "this object is always unsafe to use after move, unless a method 
> > annotated with `reinitializes` is called"?
>
>
> Exactly :) My only concern is that I doubt users will end up passing such 
> options using command line options. Having file base configuration options 
> would be more convenient. They can be easily checked in the repository and 
> evolve together with the product (like the .clang-tidy files in the LLVM 
> repos).


I mean, attribute, like, annotation in the source code.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54557/new/

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D54557#1300654, @NoQ wrote:

> In https://reviews.llvm.org/D54557#1299736, @xazax.hun wrote:
>
> > It would be great to have a way to extend the list of (possibly non-stl) 
> > types to check. But I do understand that the analyzer does not have a great 
> > way to set such configuration options right now.
>
>
> Do you envision room for another attribute here? I.e., a class attribute that 
> says "this object is always unsafe to use after move, unless a method 
> annotated with `reinitializes` is called"?


Exactly :) My only concern is that I doubt users will end up passing such 
options using command line options. Having file base configuration options 
would be more convenient. They can be easily checked in the repository and 
evolve together with the product (like the .clang-tidy files in the LLVM repos).


https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

In https://reviews.llvm.org/D54557#1301869, @NoQ wrote:

> In https://reviews.llvm.org/D54557#1300671, @Szelethus wrote:
>
> > Nice catch, would you like to make an issue on their project or shall I?
>
>
> I guess it can be an outright pull request :) I'll see if i have time for 
> that soon, please feel free to take initiative><


I'll do the honors then. :)

> 
> 
> In https://reviews.llvm.org/D54557#1301829, @Szelethus wrote:
> 
>> Yup, there seems to be a desire to keep it around. Let's add an entry to the 
>> open projects maybe?
> 
> 
> Mmm, what is the project here? What changes are still necessary?

Wasn't the point of this patch to turn off part of this checkers functionality 
because it's immature just yet? From what I understand it is desired, but the 
FP rate is a little too high. I guess fixing that is the project.

Hmm, actually, tinkering with HTML files might be overkill, especially since 
sphinx will hopefully end that era. Let's just add a TODO and let me deal with 
it later when I reorganize the checker options. Sorry for talking nonsense :D


https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In https://reviews.llvm.org/D54557#1300671, @Szelethus wrote:

> Nice catch, would you like to make an issue on their project or shall I?


I guess it can be an outright pull request :) I'll see if i have time for that 
soon, please feel free to take initiative><

In https://reviews.llvm.org/D54557#1301829, @Szelethus wrote:

> Yup, there seems to be a desire to keep it around. Let's add an entry to the 
> open projects maybe?


Mmm, what is the project here? What changes are still necessary?


https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 174475.
NoQ added a comment.

Give the rvalue reference test function a sensible name.


https://reviews.llvm.org/D54557

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -4,6 +4,14 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
+// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
+// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
 
 namespace std {
 
@@ -36,6 +44,13 @@
   b = std::move(c);
 }
 
+template 
+class vector {
+public:
+  vector();
+  void push_back(const T );
+};
+
 } // namespace std
 
 class B {
@@ -71,7 +86,10 @@
 moveconstruct(std::move(*a));
   }
   A(const A ) : i(other.i), d(other.d), b(other.b) {}
-  A(A &) : i(other.i), d(other.d), b(std::move(other.b)) { // expected-note {{'b' became 'moved-from' here}}
+  A(A &) : i(other.i), d(other.d), b(std::move(other.b)) {
+#ifdef AGGRESSIVE
+// expected-note@-2{{'b' became 'moved-from' here}}
+#endif
   }
   A(A &, char *k) {
 moveconstruct(std::move(other));
@@ -424,26 +442,51 @@
 
   void f() {
 A b;
-b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
-a.foo();  // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object 'a'}}
+b = std::move(a);
+a.foo();
+#ifdef AGGRESSIVE
+// expected-note@-3{{'a' became 'moved-from' here}}
+// expected-warning@-3 {{Method call on a 'moved-from' object 'a'}}
+// expected-note@-4{{Method call on a 'moved-from' object 'a'}}
+#endif
 
-b = std::move(static_a); // expected-note {{'static_a' became 'moved-from' here}}
-static_a.foo();  // expected-warning {{Method call on a 'moved-from' object 'static_a'}} expected-note {{Method call on a 'moved-from' object 'static_a'}}
+b = std::move(static_a);
+static_a.foo();
+#ifdef AGGRESSIVE
+// expected-note@-3{{'static_a' became 'moved-from' here}}
+// expected-warning@-3{{Method call on a 'moved-from' object 'static_a'}}
+// expected-note@-4{{Method call on a 'moved-from' object 'static_a'}}
+#endif
   }
 };
 
 void PtrAndArrayTest() {
   A *Ptr = new A(1, 1.5);
   A Arr[10];
-  Arr[2] = std::move(*Ptr); // expected-note {{Became 'moved-from' here}}
-  (*Ptr).foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}}
+  Arr[2] = std::move(*Ptr);
+  (*Ptr).foo();
+#ifdef AGGRESSIVE
+  // expected-note@-3{{Became 'moved-from' here}}
+  // expected-warning@-3{{Method call on a 'moved-from' object}}
+  // expected-note@-4{{Method call on a 'moved-from' object}}
+#endif
 
   Ptr = [1];
-  Arr[3] = std::move(Arr[1]); // expected-note {{Became 'moved-from' here}}
-  Ptr->foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}}
+  Arr[3] = std::move(Arr[1]);
+  Ptr->foo();
+#ifdef AGGRESSIVE
+  // expected-note@-3{{Became 'moved-from' here}}
+  // expected-warning@-3{{Method call on a 'moved-from' object}}
+  // expected-note@-4{{Method call on a 'moved-from' object}}
+#endif
 
-  Arr[3] = std::move(Arr[2]); // expected-note {{Became 'moved-from' here}}
-  Arr[2].foo();   // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object}}
+  Arr[3] = std::move(Arr[2]);
+  Arr[2].foo();
+#ifdef AGGRESSIVE
+  // expected-note@-3{{Became 'moved-from' here}}
+  // expected-warning@-3{{Method call on a 'moved-from' object}}
+  // expected-note@-4{{Method call on a 'moved-from' object}}
+#endif
 
   Arr[2] = std::move(Arr[3]); // reinitialization
   Arr[2].foo();   // no-warning
@@ -649,13 +692,24 @@
 void subRegionMoveTest() {
   {
 A a;
-B b = std::move(a.b); // expected-note {{'b' became 'moved-from' here}}
-a.b.foo();// expected-warning {{Method call on a 'moved-from' object 'b'}} expected-note {{Method call on a 'moved-from' object 'b'}}
+B b = std::move(a.b);
+a.b.foo();
+#ifdef AGGRESSIVE
+// expected-note@-3{{'b' became 'moved-from' 

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

> In https://reviews.llvm.org/D54557#1300653, @NoQ wrote:
> 
>> In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote:
>>
>> > I think we should either remove the non-default functionality (which 
>> > wouldn't be ideal), or emphasise somewhere (open projects?) that there is 
>> > still work to be done, but leaving it to be forgotten and essentially 
>> > making it an extra maintenance work would be, in my optinion, the worst 
>> > case scenario. `Aggressive` isn't `Pedantic` because it actually emits 
>> > warnings on correct code, and it's not a simple matter of too many reports 
>> > being emitted, let's also document that this is an experimental feature, 
>> > not a power-user-only thing.
>>
>>
>> I only kept the option around because i was under an impression that i'm 
>> intruding into a checker that already has some happy users, probably 
>> breaking existing workflows. If this option is unnecessary, i'd be happy to 
>> remove it :)
> 
> 
> Hmm, I'll ask around, but I'm not aware of any ongoing (or planned in the 
> near future) work on this particular checker.

Yup, there seems to be a desire to keep it around. Let's add an entry to the 
open projects maybe?


Repository:
  rC Clang

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

In https://reviews.llvm.org/D54557#1300581, @NoQ wrote:

> In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote:
>
> > Whenever I compile `Rtags`, I get a bunch of move related warnings, I'm 
> > curious how this patch behaves on that project. I'll take a look.
>
>
> Whoops, sry, i accidentally had a look because i misread your message as if 
> you wanted me to take a look >.<
>
> (IMPORTANT) **Spoiler alert!**
>
> It finds one positive (and one duplicate of that one positive):
>
> F7553145: rtags-move-positive.html 
>
> I believe this positive is a real bug, but we can still do better here. We 
> find it as a use-after-move of a local variable, which is not a bug on its 
> own, i.e. the user intended to empty and then re-use the storage and it's 
> fine, this is not a usual kind of typo where the user uses the pre-move 
> variable instead of the post-move variable. But the real bug here is that 
> this local variable uses an `std::string` as a field under the hood, which 
> is, well, not guaranteed to be empty after move, like all other STL objects: 
> https://stackoverflow.com/questions/27376623/can-you-reuse-a-moved-stdstring
>
> NOTE: As also mentioned in this stackoverflow thread, we also need to exclude 
> smart pointers from the STL check because they don't end up in unspecified 
> state, and see if there are other cornercases here.


OMG That is cool. :D That project was fishy. Nice catch, would you like to make 
an issue on their project or shall I?

> Unfortunately, we don't find this bug as an STL use-after-move bug because 
> inlining isn't happening. Why? I guess i'll leave it as an excercise ^.^ It's 
> a combination of running out of budget and a specific feature that we have. 
> By flipping a single `-analyzer-config` flag that represents that feature, we 
> are able to find such bugs as STL bugs (when local variable bugs are disabled 
> in the checker). We're still not able to find the original bug, most likely 
> due to running out of budget (i didn't debug this further), but we can find 
> it in a minimal example:
> 
>   #include "rct/Rct.h"
>   
>   void foo() {
> String S1;
> String S2 = std::move(S1);
> S1 += "asdfg"; // use-after-move of a std::string
>   }
> 
> 
> Here's the report that we are able to obtain for this trivial code snippet, 
> and you can look up the answer to the exercise in the collapsed run-line :)
> 
> F7553290: rtags-move-positive-simplified.html 
> 

Thanks! :) *throws confetti*

In https://reviews.llvm.org/D54557#1300653, @NoQ wrote:

> In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote:
>
> > I think we should either remove the non-default functionality (which 
> > wouldn't be ideal), or emphasise somewhere (open projects?) that there is 
> > still work to be done, but leaving it to be forgotten and essentially 
> > making it an extra maintenance work would be, in my optinion, the worst 
> > case scenario. `Aggressive` isn't `Pedantic` because it actually emits 
> > warnings on correct code, and it's not a simple matter of too many reports 
> > being emitted, let's also document that this is an experimental feature, 
> > not a power-user-only thing.
>
>
> I only kept the option around because i was under an impression that i'm 
> intruding into a checker that already has some happy users, probably breaking 
> existing workflows. If this option is unnecessary, i'd be happy to remove it 
> :)


Hmm, I'll ask around, but I'm not aware of any ongoing (or planned in the near 
future) work on this particular checker.


Repository:
  rC Clang

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In https://reviews.llvm.org/D54557#1299736, @xazax.hun wrote:

> It would be great to have a way to extend the list of (possibly non-stl) 
> types to check. But I do understand that the analyzer does not have a great 
> way to set such configuration options right now.


Do you envision room for another attribute here? I.e., a class attribute that 
says "this object is always unsafe to use after move, unless a method annotated 
with `reinitializes` is called"?


Repository:
  rC Clang

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote:

> I think we should either remove the non-default functionality (which wouldn't 
> be ideal), or emphasise somewhere (open projects?) that there is still work 
> to be done, but leaving it to be forgotten and essentially making it an extra 
> maintenance work would be, in my optinion, the worst case scenario. 
> `Aggressive` isn't `Pedantic` because it actually emits warnings on correct 
> code, and it's not a simple matter of too many reports being emitted, let's 
> also document that this is an experimental feature, not a power-user-only 
> thing.


I only kept the option around because i was under an impression that i'm 
intruding into a checker that already has some happy users, probably breaking 
existing workflows. If this option is unnecessary, i'd be happy to remove it :)


Repository:
  rC Clang

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote:

> Whenever I compile `Rtags`, I get a bunch of move related warnings, I'm 
> curious how this patch behaves on that project. I'll take a look.


Whoops, sry, i accidentally had a look because i misread your message as if you 
wanted me to take a look >.<

(IMPORTANT) **Spoiler alert!**

It finds one positive (and one duplicate of that one positive):

F7553145: rtags-move-positive.html 

I believe this positive is a real bug, but we can still do better here. We find 
it as a use-after-move of a local variable, which is not a bug on its own, i.e. 
the user intended to empty and then re-use the storage and it's fine, this is 
not a usual kind of typo where the user uses the pre-move variable instead of 
the post-move variable. But the real bug here is that this local variable uses 
an `std::string` as a field under the hood, which is, well, not guaranteed to 
be empty after move, like all other STL objects: 
https://stackoverflow.com/questions/27376623/can-you-reuse-a-moved-stdstring

NOTE: As also mentioned in this stackoverflow thread, we also need to exclude 
smart pointers from the STL check because they don't end up in unspecified 
state, and see if there are other cornercases here.

Unfortunately, we don't find this bug as an STL use-after-move bug because 
inlining isn't happening. Why? I guess i'll leave it as an excercise ^.^ It's a 
combination of running out of budget and a specific feature that we have. By 
flipping a single `-analyzer-config` flag that represents that feature, we are 
able to find such bugs as STL bugs (when local variable bugs are disabled in 
the checker). We're still not able to find the original bug, most likely due to 
running out of budget (i didn't debug this further), but we can find it in a 
minimal example:

  #include "rct/Rct.h"
  
  void foo() {
String S1;
String S2 = std::move(S1);
S1 += "asdfg"; // use-after-move of a std::string
  }

Here's the report that we are able to obtain for this trivial code snippet, and 
you can look up the answer to the exercise in the collapsed run-line :)

F7553290: rtags-move-positive-simplified.html 



Repository:
  rC Clang

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

Alpha checkers, at least to developers, are clearly stating "Please finish 
me!", but a non-alpha, enabled-by-default checker with a flag that you'd never 
know about unless you looked at the source code (at least up until I fix these) 
sounds like it'll be forgotten like many other similar flags.


Repository:
  rC Clang

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

I think we should either remove the non-default functionality (which wouldn't 
be ideal), or emphasise somewhere (open projects?) that there is still work to 
be done, but leaving it to be forgotten and essentially making it an extra 
maintenance work would be, in my optinion, the worst case scenario. 
`Aggressive` isn't `Pedantic` because it actually emits warnings on correct 
code, and it's not a simple matter of too many reports being emitted, let's 
also document that this is an experimental feature, not a power-user-only thing.

Whenever I compile `Rtags`, I get a bunch of move related warnings, I'm curious 
how this patch behaves on that project. I'll take a look.




Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:92-95
+  bool IsAggressive = false;
+
+public:
+  void setAggressiveness(bool Aggressive) { IsAggressive = Aggressive; }

How about a simple public data member? Since all callbacks are const, it 
wouldn't be modified after registration anyways.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:134
+static const MemRegion *unwrapRValueReferenceIndirection(const MemRegion *MR) {
+  if (const auto *SR = dyn_cast_or_null(MR)) {
+SymbolRef Sym = SR->getSymbol();

You use `dyn_cast_or_null`, which to me implies that the returned value may be 
`nullptr`, but you never check it on the call sites.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:273-282
+  // In non-aggressive mode, only warn on use-after-move of local variables (or
+  // local rvalue references) and of STL objects. The former is possible 
because
+  // local variables (or local rvalue references) are not tempting their user 
to
+  // re-use the storage. The latter is possible because STL objects are known
+  // to end up in a valid but unspecified state after the move and their
+  // state-reset methods are also known, which allows us to predict
+  // precisely when use-after-move is invalid.

I've seen checker option descriptions in registry functions, in-class where 
they are declared, and on top of the file, I think any of them would be more 
ideal. Since my checker option related efforts are still far in the distance 
(which will put all of them in `Checkers.td`), let's move (or at least copy) 
this somewhere else.



Comment at: test/Analysis/use-after-move.cpp:47-52
+template 
+class vector {
+public:
+  vector();
+  void push_back(const T );
+};

Any particular reason for not using `cxx-system-header-simulator.h`?


Repository:
  rC Clang

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

It would be great to have a way to extend the list of (possibly non-stl) types 
to check. But I do understand that the analyzer does not have a great way to 
set such configuration options right now.


Repository:
  rC Clang

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

So, like, with locals it should be possible to easily and intuitively suppress 
the false positive, even if there is one, by simply using an extra variable, 
and additionally there's the `[[clang::reinitializes]]` attribute added by 
@xazax.hun that will help people suppress the warning when their object has a 
popular state-reset method that Static Analyzer doesn't understand. I believe 
this is a good trade-off between usefulness and usability, given that most 
positives with locals i've seen so far looked like real bugs.


Repository:
  rC Clang

https://reviews.llvm.org/D54557



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, 
rnkovacs, Szelethus.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, 
baloghadamsoftware.

This patch implements the post important part of the plan proposed in 
https://reviews.llvm.org/D54556. Under `alpha.cplusplus.Move:Aggressive=true`, 
the checker retains the original behavior, but when that option is equal to 
`false` (which is the proposed default behavior), the checker only warns in the 
following three cases:

1. If the object that's being used after move is of a known STL type (the word 
"known" here makes no sense; what i'm trying to say is that we'll rant about 
objects of any STL type, unless we encounter false positives even there and 
it'll cause us to restrict the check even further). The point here is that 
according to the C++ Standard, STL objects are known to remain in "valid but 
unspecified state", which means that the state is going to be internally 
consistent state which is not known. A lot of operations clearly make no sense 
(eg., asking for the value of the first character in a string that has just 
been moved makes little sense), and we know it and we can warn in these cases 
reliably. If some operations do make sense or, moreover, if some operations 
return the object into a valid state, we can easily hardcode these operations 
in the checker because STL has limited amount of stuff in it.
2. If the object is a local or parameter variable. This not only correlates 
with the true positives that i've seen so far, but also kinda makes sense, 
though, as usual, i'd love to be proven wrong. When an object is, say, a field 
in a class, or, even worse, a global, then even if you move from that object, 
the object is still there. It won't go anywhere; it *will* have to be re-used 
eventually. Which means that if the object is left in a well-specified state 
after getting moved, there's no good reason to enforce the developer to reset 
it manually. However, if the object is a local that is going to be cleaned up 
soon anyway, it is very unlikely that the desire to re-use its storage is 
well-justified. It should be totally fine to create another local and use that 
as a storage for the next object that you need. Hence the idea.
3. If the object is an rvalue reference. The idea is exactly the same as in 2: 
an xvalue is something that's near the end of its lifetime, so there's no 
temptation to re-use the storage. Whoever passed us that value is already fine 
with it being moved away, we have no obligation to fill in the space again.


Repository:
  rC Clang

https://reviews.llvm.org/D54557

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -4,6 +4,14 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
+// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
+// RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
+// RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
+// RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
 
 namespace std {
 
@@ -36,6 +44,13 @@
   b = std::move(c);
 }
 
+template 
+class vector {
+public:
+  vector();
+  void push_back(const T );
+};
+
 } // namespace std
 
 class B {
@@ -71,7 +86,10 @@
 moveconstruct(std::move(*a));
   }
   A(const A ) : i(other.i), d(other.d), b(other.b) {}
-  A(A &) : i(other.i), d(other.d), b(std::move(other.b)) { // expected-note {{'b' became 'moved-from' here}}
+  A(A &) : i(other.i), d(other.d), b(std::move(other.b)) {
+#ifdef AGGRESSIVE
+// expected-note@-2{{'b' became 'moved-from' here}}
+#endif
   }
   A(A &, char *k) {
 moveconstruct(std::move(other));
@@ -424,26 +442,51 @@
 
   void f() {
 A b;
-b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
-a.foo();  // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object 'a'}}
+b = std::move(a);
+a.foo();
+#ifdef AGGRESSIVE
+// expected-note@-3{{'a' became 'moved-from' here}}
+// expected-warning@-3 {{Method call on a 'moved-from' object 'a'}}
+// expected-note@-4{{Method call on a 'moved-from' object 'a'}}
+#endif
 
-b = std::move(static_a); //