[analyzer] MoveChecker Pt.4: Add a few more state reset methods.

[NoQ]

Summary

This covers methods that may reset the state depending on the value of the argument: resize(), shrink_to_fit(), and shrink(). The latter is something i've seen on a custom collection; we don't have to hardcode those, but i guess it's a good safe way to be a bit better out of the box. As a TODO, we might want to warn anyway when the value of the argument is not 0.

[Szelethus]

How about std::list<T>::splice?

[NoQ]

In D54563#1299918, @Szelethus wrote:

How about std::list<T>::splice?

Hmm, you mean that it leaves its argument empty? Interesting, i guess we may need a separate facility for that. I wonder how many other state-reset methods are we forgetting.

[Szelethus]

In D54563#1301893, @NoQ wrote:

In D54563#1299918, @Szelethus wrote:

How about std::list<T>::splice?

Hmm, you mean that it leaves its argument empty? Interesting, i guess we may need a separate facility for that.

I looked it up, and this is what I found: (source: http://eel.is/c++draft/list.ops)

list provides three splice operations that destructively move elements from one list to another. The behavior of splice operations is undefined if get_­allocator() != x.get_­allocator().

void splice(const_iterator position, list& x);
void splice(const_iterator position, list&& x);

Effects: Inserts the contents of x before position and x becomes empty. Pointers and references to the moved elements of x now refer to those same elements but as members of *this. Iterators referring to the moved >elements will continue to refer to their elements, but they now behave as iterators into *this, not into x.

This suggests to me that list1.splice(list1.begin(), std::move(list2)) leaves list2 in a valid, well-defined, but empty state.

I wonder how many other state-reset methods are we forgetting.

merge does something similar too for map, multimap, set, multiset, as well as their unordered counterparts.

As a TODO, we might want to warn anyway when the value of the argument is not 0.

Maybe leave a TODO in the code as well? ^-^

[NoQ]

In D54563#1306358, @Szelethus wrote:

This suggests to me that list1.splice(list1.begin(), std::move(list2)) leaves list2 in a valid, well-defined, but empty state.

Wait, i think this should work automagically. Because list2 is passed by non-const reference (eg., rvalue reference) into an unknown function, it will be invalidated when the call is modeled conservatively, and therefore we will stop tracking it in the checkRegionChanges callback.

[NoQ]

Add a TODO.

[Szelethus]

Can you add tests for that just in case? :)

test/Analysis/use-after-move.cpp

330–331

Because list2 is passed by non-const reference (eg., rvalue reference) into an unknown function, it will be invalidated when the call is modeled conservatively, and therefore we will stop tracking it in the checkRegionChanges callback.

Hmmm. Doesn't this check something similar, but still cause an warning?

[NoQ]

test/Analysis/use-after-move.cpp

260–262

This would have been the test for our case, but in this test the function has a body and will not be evaluated conservatively.

330–331

In this case a is passed by value. Therefore a move-constructor is called, and then the function does not have access to a.

597–615

This would have been the test for our case, but the && case isn't tested.

[Szelethus]

Okay, I submit! :D

test/Analysis/use-after-move.cpp

260–262

Hmm, since those are all STL containers, we should be able to have their definition? Or only with c++-container-inlining=true? Take this as more of a question than anything else.

[NoQ]

I'm glad you brought this stuff up, found two more bugs to fix.

In D54563#1317678, @Szelethus wrote:

Can you add tests for that just in case? :)

The test works, but i noticed that we aren't respecting const references. That is, we believe that passing the object by const reference or pointer may reset it. I guess i'll make a separate commit.

test/Analysis/use-after-move.cpp

260–262

Yeah, assuming that we only need containers.

As usual, i expect only good things to happen when we inline them. Say, if a field is overwritten in the inlined call, this would also trigger checkRegionChanges. And i doubt that in these examples there will be a path through the function that leaves the object entirely untouched.

As a side note, i doubt that we'll actually react to field assignment, because this checker's checkRegionChanges callback doesn't account for that: it only iterates through explicit regions. I guess this needs fixing.

[NoQ]

Hmm, i shouldn't have included shrink_to_fit. It definitely keeps the object in unspecified state if it is already in unspecified state.

lib/StaticAnalyzer/Checkers/MoveChecker.cpp

 
 bool MoveChecker::isStateResetMethod(const CXXMethodDecl *MethodDec) const {
   if (!MethodDec)
       return false;
   if (MethodDec->hasAttr<ReinitializesAttr>())
       return true;
   if (MethodDec->getDeclName().isIdentifier()) {
     std::string MethodName = MethodDec->getName().lower();
+    // TODO: Some of these methods (eg., resize) are not always resetting
+    // the state, so we should consider looking at the arguments.
     if (MethodName == "reset" || MethodName == "clear" ||
-        MethodName == "destroy")
+        MethodName == "destroy" || MethodName == "resize" ||
+        MethodName == "shrink")
       return true;
   }
   return false;
 }
 
 // Don't report an error inside a move related operation.
 // We assume that the programmer knows what she does.
 bool MoveChecker::isInMoveSafeContext(const LocationContext *LC) const {

test/Analysis/use-after-move.cpp

 // 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 {
 
+typedef __typeof(sizeof(int)) size_t;
+
 template <typename>
 struct remove_reference;
 
 template <typename _Tp>
 struct remove_reference { typedef _Tp type; };
 
 template <typename _Tp>
 struct remove_reference<_Tp &> { typedef _Tp type; };
void operator=(A &&other) {
     return;
   }
   int getI() { return i; }
   int foo() const;
   void bar() const;
   void reset();
   void destroy();
   void clear();
+  void resize(std::size_t);
   bool empty() const;
   bool isEmpty() const;
   operator bool() const;
 };
 
 int bignum();
 
 void moveInsideFunctionCall(A a) {
void decltypeIsNotUseTest() {
   A a;
   // A b(std::move(a));
   decltype(a) other_a; // no-warning
 }
 
 void loopTest() {
   {
     A a;
     for (int i = 0; i < bignum(); i++) { // expected-note {{Loop condition is false. Execution jumps to the end of the function}}
       rightRefCall(std::move(a));        // no-warning
     }
   }
   {
     A a;
     for (int i = 0; i < 2; i++) { // expected-note {{Loop condition is true.  Entering loop body}}
       //expected-note@-1 {{Loop condition is true.  Entering loop body}}
       //expected-note@-2 {{Loop condition is false. Execution jumps to the end of the function}}
       rightRefCall(std::move(a)); // no-warning
     }
void loopTest() {
     for (int i = 0; i < 2; i++) { // expected-note {{Loop condition is true.}}
       //expected-note@-1 {{Loop condition is true.  Entering loop body}}
       //expected-note@-2 {{Loop condition is false. Execution jumps to the end of the function}}
       copyOrMoveCall(a);          // no-warning
     }
   }
   {
     A a;
     for (int i = 0; i < bignum(); i++) { // expected-note {{Loop condition is true.  Entering loop body}} expected-note {{Loop condition is true.  Entering loop body}}
       constCopyOrMoveCall(std::move(a)); // expected-warning {{Moved-from object 'a' is moved}} expected-note {{Moved-from object 'a' is moved}}
       // expected-note@-1 {{Object 'a' is moved}}
     }
   }
 
   // Don't warn if we return after the move.
   {
     A a;
     for (int i = 0; i < 3; ++i) {
void moveStateResetFunctionsTest() {
   }
   {
     A a;
     A b = std::move(a);
     a.clear(); // no-warning
     a.foo();   // no-warning
     a.b.foo(); // no-warning
   }
+  {
+    A a;
+    A b = std::move(a);
+    a.resize(0); // no-warning
+    a.foo();   // no-warning
+    a.b.foo(); // no-warning
+  }
 }
 
 // Moves or uses that occur as part of template arguments.
 template <int>
 class ClassTemplate {
 public:
   void foo(A a);
 };
void paramEvaluateOrderTest() {
   A a;
   foobar(std::move(a), a.getI()); // expected-warning {{Method called on moved-from object 'a'}} expected-note {{Method called on moved-from object 'a'}}
   // expected-note@-1 {{Object 'a' is moved}}
 
   //FALSE NEGATIVE since parameters evaluate order is undefined
   foobar(a.getI(), std::move(a)); //no-warning
 }
 
 void not_known(A &a);
 void not_known(A *a);
 
 void regionAndPointerEscapeTest() {
   {
     A a;
     A b;
     b = std::move(a);
     not_known(a);
     a.foo(); //no-warning
   }
   {
     A a;
     A b;
     b = std::move(a);
     not_known(&a);
     a.foo(); // no-warning
   }
 }
 
 // A declaration statement containing multiple declarations sequences the
 // initializer expressions.
 void declarationSequenceTest() {
   {
     A a;
     A a1 = a, a2 = std::move(a); // no-warning
   }