[PATCH] D67743: [Consumed] Treat by-value class arguments as consuming by default, like rvalue refs.

[Nicholas Allegra via Phabricator via cfe-commits]

comex created this revision.
comex added a reviewer: dblaikie.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Also, fix the order of `if` statements so that an explicit `return_typestate` annotation takes precedence over the default behavior for rvalue refs.

Note that for by-value arguments, the object being consumed is always a temporary.  If you write:

`void a(X); a((X &&)x);`

...this first constructs a temporary X by moving from `x`, then calls `a` with a pointer to the temporary.  Before and after this change, the move copies `x`'s typestate to the temporary and marks `x` itself as consumed.  But before this change, if the temporary started out unconsumed (because `x` was unconsumed before the move), it would still be unconsumed when its destructor was run after the call.  Now it will be considered consumed at that point.

Also note that currently, only parameters explicitly marked `return_typestate` have their state-on-return checked on the callee side.  Parameters which are implicitly consuming due to being rvalue references – or, after this patch, by-value – are checked only on the caller side.  This discrepancy was very surprising to me as a user.  I wrote a fix, but in my codebase it broke some code that was using `std::forward`.  Therefore, I plan to hold off on submitting the fix until a followup patch, which will generalize the current `std::move` special case into an attribute that can be applied to any 'cast' function like `std::move` and `std::forward`.


Repository:
  rC Clang

https://reviews.llvm.org/D67743

Files:
  lib/Analysis/Consumed.cpp
  test/SemaCXX/warn-consumed-analysis.cpp


Index: test/SemaCXX/warn-consumed-analysis.cpp
===================================================================
--- test/SemaCXX/warn-consumed-analysis.cpp
+++ test/SemaCXX/warn-consumed-analysis.cpp
@@ -53,10 +53,15 @@
 public:
   DestructorTester();
   DestructorTester(int);
+  DestructorTester(nullptr_t) RETURN_TYPESTATE(unconsumed);
+  DestructorTester(DestructorTester &&);
   
   void operator*() CALLABLE_WHEN("unconsumed");
   
   ~DestructorTester() CALLABLE_WHEN("consumed");
+
+  static void byVal(DestructorTester);
+  static void byValMarkUnconsumed(DestructorTester RETURN_TYPESTATE(unconsumed));
 };
 
 void baf0(const ConsumableClass<int>  var);
@@ -120,6 +125,19 @@
              expected-warning {{invalid invocation of method '~DestructorTester' on object 'D1' while it is in the 'unconsumed' state}}
 }
 
+void testDestructionByVal() {
+  {
+    // both the var and the temporary are consumed:
+    DestructorTester D0(nullptr);
+    DestructorTester::byVal((DestructorTester &&)D0);
+  }
+  {
+    // the var is consumed but the temporary isn't:
+    DestructorTester D1(nullptr);
+    DestructorTester::byValMarkUnconsumed((DestructorTester &&)D1); // expected-warning {{invalid invocation of method '~DestructorTester' on a temporary object while it is in the 'unconsumed' state}}
+  }
+}
+
 void testTempValue() {
   *ConsumableClass<int>(); // expected-warning {{invalid invocation of method 'operator*' on a temporary object while it is in the 'consumed' state}}
 }
@@ -413,10 +431,15 @@
   Param.consume();
 }
 
+void testRvalueRefParamReturnTypestateCallee(ConsumableClass<int> &&Param RETURN_TYPESTATE(unconsumed)) {
+  Param.unconsume();
+}
+
 void testParamReturnTypestateCaller() {
   ConsumableClass<int> var;
   
   testParamReturnTypestateCallee(true, var);
+  testRvalueRefParamReturnTypestateCallee((ConsumableClass<int> &&)var);
   
   *var;
 }
@@ -480,6 +503,9 @@
   
   baf2(&var);  
   *var;
+
+  baf3(var);
+  *var;
   
   baf4(var);  
   *var; // expected-warning {{invalid invocation of method 'operator*' on object 'var' while it is in the 'unknown' state}}
Index: lib/Analysis/Consumed.cpp
===================================================================
--- lib/Analysis/Consumed.cpp
+++ lib/Analysis/Consumed.cpp
@@ -645,10 +645,10 @@
       continue;
 
     // Adjust state on the caller side.
-    if (isRValueRef(ParamType))
-      setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
-    else if (ReturnTypestateAttr *RT = Param->getAttr<ReturnTypestateAttr>())
+    if (ReturnTypestateAttr *RT = Param->getAttr<ReturnTypestateAttr>())
       setStateForVarOrTmp(StateMap, PInfo, mapReturnTypestateAttrState(RT));
+    else if (isRValueRef(ParamType) || isConsumableType(ParamType))
+      setStateForVarOrTmp(StateMap, PInfo, consumed::CS_Consumed);
     else if (isPointerOrRef(ParamType) &&
              (!ParamType->getPointeeType().isConstQualified() ||
               isSetOnReadPtrType(ParamType)))


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D67743.220778.patch
Type: text/x-patch
Size: 2976 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190919/30cd2a7d/attachment.bin>