c++: Extend -Wpessimizing-move for class prvalues [PR106276]

Message ID 20220728001424.396849-1-polacek@redhat.com
State New, archived
Headers
Series c++: Extend -Wpessimizing-move for class prvalues [PR106276] |

Commit Message

Marek Polacek July 28, 2022, 12:14 a.m. UTC
  We already have a warning that warns about pessimizing std::move
in a return statement, when it prevents the NRVO:

  T fn()
  {
    T t;
    return std::move (t); // warning \o/
  }

However, the warning doesn't warn when what we are returning is a class
prvalue, that is, when std::move prevents the RVO:

  T fn()
  {
    T t;
    return std::move (T{}); // no warning :-(
  }

This came up recently in GCC:
<https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598177.html>.

This patch fixes that.  I would like to extend the warning further, so
that it warns in more contexts, e.g.:

  T t = std::move(T());

or

  void foo (T);
  foo (std::move(T()));

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

	PR c++/106276

gcc/cp/ChangeLog:

	* typeck.cc (can_do_rvo_p): New.
	(maybe_warn_pessimizing_move): Warn when moving a temporary object
	in a return statement prevents copy elision.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp0x/Wpessimizing-move7.C: New test.
---
 gcc/cp/typeck.cc                              | 31 ++++++++-
 .../g++.dg/cpp0x/Wpessimizing-move7.C         | 63 +++++++++++++++++++
 2 files changed, 91 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C


base-commit: 219f86495791fd27b012678f13e10cada211daab
  

Comments

Jason Merrill Aug. 6, 2022, 10:49 p.m. UTC | #1
On 7/27/22 17:14, Marek Polacek wrote:
> We already have a warning that warns about pessimizing std::move
> in a return statement, when it prevents the NRVO:
> 
>    T fn()
>    {
>      T t;
>      return std::move (t); // warning \o/
>    }
> 
> However, the warning doesn't warn when what we are returning is a class
> prvalue, that is, when std::move prevents the RVO:
> 
>    T fn()
>    {
>      T t;
>      return std::move (T{}); // no warning :-(
>    }
> 
> This came up recently in GCC:
> <https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598177.html>.
> 
> This patch fixes that.  I would like to extend the warning further, so
> that it warns in more contexts, e.g.:
> 
>    T t = std::move(T());
> 
> or
> 
>    void foo (T);
>    foo (std::move(T()));
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> 	PR c++/106276
> 
> gcc/cp/ChangeLog:
> 
> 	* typeck.cc (can_do_rvo_p): New.
> 	(maybe_warn_pessimizing_move): Warn when moving a temporary object
> 	in a return statement prevents copy elision.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/Wpessimizing-move7.C: New test.
> ---
>   gcc/cp/typeck.cc                              | 31 ++++++++-
>   .../g++.dg/cpp0x/Wpessimizing-move7.C         | 63 +++++++++++++++++++
>   2 files changed, 91 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
> 
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 6e4f23af982..9500c4e2fe8 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -10287,12 +10287,29 @@ can_do_nrvo_p (tree retval, tree functype)
>   	  /* The cv-unqualified type of the returned value must be the
>   	     same as the cv-unqualified return type of the
>   	     function.  */
> -	  && same_type_p ((TYPE_MAIN_VARIANT (TREE_TYPE (retval))),
> -			  (TYPE_MAIN_VARIANT (functype)))
> +	  && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (retval)),
> +			  TYPE_MAIN_VARIANT (functype))
>   	  /* And the returned value must be non-volatile.  */
>   	  && !TYPE_VOLATILE (TREE_TYPE (retval)));
>   }
>   
> +/* Like can_do_nrvo_p, but we check if we're trying to move a class
> +   prvalue.  */
> +
> +static bool
> +can_do_rvo_p (tree retval, tree functype)
> +{
> +  if (functype == error_mark_node)
> +    return false;
> +  if (retval)
> +    STRIP_ANY_LOCATION_WRAPPER (retval);
> +  return (retval != NULL_TREE
> +	  && TREE_CODE (retval) == TARGET_EXPR

Maybe use !glvalue_p instead of specifically checking for TARGET_EXPR? 
I don't feel strongly about this.

> +	  && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (retval)),
> +			  TYPE_MAIN_VARIANT (functype))
> +	  && !TYPE_VOLATILE (TREE_TYPE (retval)));
> +}
> +
>   /* If we should treat RETVAL, an expression being returned, as if it were
>      designated by an rvalue, returns it adjusted accordingly; otherwise, returns
>      NULL_TREE.  See [class.copy.elision].  RETURN_P is true if this is a return
> @@ -10401,12 +10418,20 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
>   			      "prevents copy elision"))
>   		inform (loc, "remove %<std::move%> call");
>   	    }
> +	  else if (can_do_rvo_p (arg, functype))
> +	    {
> +	      auto_diagnostic_group d;
> +	      if (warning_at (loc, OPT_Wpessimizing_move,
> +			      "moving a temporary object in a return statement "
> +			      "prevents copy elision"))

It doesn't just prevent copy elision, it produces a dangling reference. 
  This is a special case of the warning we talked about passing a 
temporary to a function that returns a reference argument unchanged, and 
should probably use a different warning flag.

> +		inform (loc, "remove %<std::move%> call");
> +	    }
>   	  /* Warn if the move is redundant.  It is redundant when we would
>   	     do maybe-rvalue overload resolution even without std::move.  */
>   	  else if (warn_redundant_move
>   		   && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
>   	    {
> -	      /* Make sure that the overload resolution would actually succeed
> +	      /* Make sure that overload resolution would actually succeed
>   		 if we removed the std::move call.  */
>   	      tree t = convert_for_initialization (NULL_TREE, functype,
>   						   moved,
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
> new file mode 100644
> index 00000000000..cd4eaa09aae
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
> @@ -0,0 +1,63 @@
> +// PR c++/106276
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wpessimizing-move" }
> +
> +// Define std::move.
> +namespace std {
> +  template<typename _Tp>
> +    struct remove_reference
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    struct remove_reference<_Tp&>
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    struct remove_reference<_Tp&&>
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    constexpr typename std::remove_reference<_Tp>::type&&
> +    move(_Tp&& __t) noexcept
> +    { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); }
> +}
> +
> +struct A { A(); A(const A&) = delete; A(A&&); };
> +struct B { B(A); };
> +
> +static A foo ();
> +
> +A
> +fn1 ()
> +{
> +  return std::move (A{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +  return std::move (A()); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +  return std::move (foo ()); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +}
> +
> +B fn2 ()
> +{
> +  return std::move (A());
> +  return std::move (A{});
> +  return std::move (foo ());
> +}
> +
> +template <typename T1, typename T2>
> +T1
> +fn3 ()
> +{
> +  return std::move (T2{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +}
> +
> +void
> +do_fn3 ()
> +{
> +  fn3<A, A>();
> +  fn3<B, A>();
> +}
> +
> +char take_buffer;
> +struct label_text {
> +  label_text take() { return std::move(label_text(&take_buffer)); } // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +  label_text(char *);
> +};
> 
> base-commit: 219f86495791fd27b012678f13e10cada211daab
  
Jason Merrill Aug. 6, 2022, 11:07 p.m. UTC | #2
On 8/6/22 15:49, Jason Merrill wrote:
> On 7/27/22 17:14, Marek Polacek wrote:
>> We already have a warning that warns about pessimizing std::move
>> in a return statement, when it prevents the NRVO:
>>
>>    T fn()
>>    {
>>      T t;
>>      return std::move (t); // warning \o/
>>    }
>>
>> However, the warning doesn't warn when what we are returning is a class
>> prvalue, that is, when std::move prevents the RVO:
>>
>>    T fn()
>>    {
>>      T t;
>>      return std::move (T{}); // no warning :-(
>>    }
>>
>> This came up recently in GCC:
>> <https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598177.html>.
>>
>> This patch fixes that.  I would like to extend the warning further, so
>> that it warns in more contexts, e.g.:
>>
>>    T t = std::move(T());
>>
>> or
>>
>>    void foo (T);
>>    foo (std::move(T()));
>>
>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>
>>     PR c++/106276
>>
>> gcc/cp/ChangeLog:
>>
>>     * typeck.cc (can_do_rvo_p): New.
>>     (maybe_warn_pessimizing_move): Warn when moving a temporary object
>>     in a return statement prevents copy elision.
>>
>> gcc/testsuite/ChangeLog:
>>
>>     * g++.dg/cpp0x/Wpessimizing-move7.C: New test.
>> ---
>>   gcc/cp/typeck.cc                              | 31 ++++++++-
>>   .../g++.dg/cpp0x/Wpessimizing-move7.C         | 63 +++++++++++++++++++
>>   2 files changed, 91 insertions(+), 3 deletions(-)
>>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
>>
>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
>> index 6e4f23af982..9500c4e2fe8 100644
>> --- a/gcc/cp/typeck.cc
>> +++ b/gcc/cp/typeck.cc
>> @@ -10287,12 +10287,29 @@ can_do_nrvo_p (tree retval, tree functype)
>>         /* The cv-unqualified type of the returned value must be the
>>            same as the cv-unqualified return type of the
>>            function.  */
>> -      && same_type_p ((TYPE_MAIN_VARIANT (TREE_TYPE (retval))),
>> -              (TYPE_MAIN_VARIANT (functype)))
>> +      && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (retval)),
>> +              TYPE_MAIN_VARIANT (functype))
>>         /* And the returned value must be non-volatile.  */
>>         && !TYPE_VOLATILE (TREE_TYPE (retval)));
>>   }
>> +/* Like can_do_nrvo_p, but we check if we're trying to move a class
>> +   prvalue.  */
>> +
>> +static bool
>> +can_do_rvo_p (tree retval, tree functype)
>> +{
>> +  if (functype == error_mark_node)
>> +    return false;
>> +  if (retval)
>> +    STRIP_ANY_LOCATION_WRAPPER (retval);
>> +  return (retval != NULL_TREE
>> +      && TREE_CODE (retval) == TARGET_EXPR
> 
> Maybe use !glvalue_p instead of specifically checking for TARGET_EXPR? I 
> don't feel strongly about this.
> 
>> +      && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (retval)),
>> +              TYPE_MAIN_VARIANT (functype))
>> +      && !TYPE_VOLATILE (TREE_TYPE (retval)));
>> +}
>> +
>>   /* If we should treat RETVAL, an expression being returned, as if it 
>> were
>>      designated by an rvalue, returns it adjusted accordingly; 
>> otherwise, returns
>>      NULL_TREE.  See [class.copy.elision].  RETURN_P is true if this 
>> is a return
>> @@ -10401,12 +10418,20 @@ maybe_warn_pessimizing_move (tree retval, 
>> tree functype)
>>                     "prevents copy elision"))
>>           inform (loc, "remove %<std::move%> call");
>>           }
>> +      else if (can_do_rvo_p (arg, functype))
>> +        {
>> +          auto_diagnostic_group d;
>> +          if (warning_at (loc, OPT_Wpessimizing_move,
>> +                  "moving a temporary object in a return statement "
>> +                  "prevents copy elision"))
> 
> It doesn't just prevent copy elision, it produces a dangling reference. 
>   This is a special case of the warning we talked about passing a 
> temporary to a function that returns a reference argument unchanged, and 
> should probably use a different warning flag.

Wait, no, I'm confused, the temporary does live long enough to get copied.

I still don't think we want to suppress this warning in the other patch.

>> +        inform (loc, "remove %<std::move%> call");
>> +        }
>>         /* Warn if the move is redundant.  It is redundant when we would
>>            do maybe-rvalue overload resolution even without 
>> std::move.  */
>>         else if (warn_redundant_move
>>              && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
>>           {
>> -          /* Make sure that the overload resolution would actually 
>> succeed
>> +          /* Make sure that overload resolution would actually succeed
>>            if we removed the std::move call.  */
>>             tree t = convert_for_initialization (NULL_TREE, functype,
>>                              moved,
>> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C 
>> b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
>> new file mode 100644
>> index 00000000000..cd4eaa09aae
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
>> @@ -0,0 +1,63 @@
>> +// PR c++/106276
>> +// { dg-do compile { target c++11 } }
>> +// { dg-options "-Wpessimizing-move" }
>> +
>> +// Define std::move.
>> +namespace std {
>> +  template<typename _Tp>
>> +    struct remove_reference
>> +    { typedef _Tp   type; };
>> +
>> +  template<typename _Tp>
>> +    struct remove_reference<_Tp&>
>> +    { typedef _Tp   type; };
>> +
>> +  template<typename _Tp>
>> +    struct remove_reference<_Tp&&>
>> +    { typedef _Tp   type; };
>> +
>> +  template<typename _Tp>
>> +    constexpr typename std::remove_reference<_Tp>::type&&
>> +    move(_Tp&& __t) noexcept
>> +    { return static_cast<typename 
>> std::remove_reference<_Tp>::type&&>(__t); }
>> +}
>> +
>> +struct A { A(); A(const A&) = delete; A(A&&); };
>> +struct B { B(A); };
>> +
>> +static A foo ();
>> +
>> +A
>> +fn1 ()
>> +{
>> +  return std::move (A{}); // { dg-warning "moving a temporary object 
>> in a return statement prevents copy elision" }
>> +  return std::move (A()); // { dg-warning "moving a temporary object 
>> in a return statement prevents copy elision" }
>> +  return std::move (foo ()); // { dg-warning "moving a temporary 
>> object in a return statement prevents copy elision" }
>> +}
>> +
>> +B fn2 ()
>> +{
>> +  return std::move (A());
>> +  return std::move (A{});
>> +  return std::move (foo ());
>> +}
>> +
>> +template <typename T1, typename T2>
>> +T1
>> +fn3 ()
>> +{
>> +  return std::move (T2{}); // { dg-warning "moving a temporary object 
>> in a return statement prevents copy elision" }
>> +}
>> +
>> +void
>> +do_fn3 ()
>> +{
>> +  fn3<A, A>();
>> +  fn3<B, A>();
>> +}
>> +
>> +char take_buffer;
>> +struct label_text {
>> +  label_text take() { return std::move(label_text(&take_buffer)); } 
>> // { dg-warning "moving a temporary object in a return statement 
>> prevents copy elision" }
>> +  label_text(char *);
>> +};
>>
>> base-commit: 219f86495791fd27b012678f13e10cada211daab
>
  

Patch

diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 6e4f23af982..9500c4e2fe8 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -10287,12 +10287,29 @@  can_do_nrvo_p (tree retval, tree functype)
 	  /* The cv-unqualified type of the returned value must be the
 	     same as the cv-unqualified return type of the
 	     function.  */
-	  && same_type_p ((TYPE_MAIN_VARIANT (TREE_TYPE (retval))),
-			  (TYPE_MAIN_VARIANT (functype)))
+	  && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (retval)),
+			  TYPE_MAIN_VARIANT (functype))
 	  /* And the returned value must be non-volatile.  */
 	  && !TYPE_VOLATILE (TREE_TYPE (retval)));
 }
 
+/* Like can_do_nrvo_p, but we check if we're trying to move a class
+   prvalue.  */
+
+static bool
+can_do_rvo_p (tree retval, tree functype)
+{
+  if (functype == error_mark_node)
+    return false;
+  if (retval)
+    STRIP_ANY_LOCATION_WRAPPER (retval);
+  return (retval != NULL_TREE
+	  && TREE_CODE (retval) == TARGET_EXPR
+	  && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (retval)),
+			  TYPE_MAIN_VARIANT (functype))
+	  && !TYPE_VOLATILE (TREE_TYPE (retval)));
+}
+
 /* If we should treat RETVAL, an expression being returned, as if it were
    designated by an rvalue, returns it adjusted accordingly; otherwise, returns
    NULL_TREE.  See [class.copy.elision].  RETURN_P is true if this is a return
@@ -10401,12 +10418,20 @@  maybe_warn_pessimizing_move (tree retval, tree functype)
 			      "prevents copy elision"))
 		inform (loc, "remove %<std::move%> call");
 	    }
+	  else if (can_do_rvo_p (arg, functype))
+	    {
+	      auto_diagnostic_group d;
+	      if (warning_at (loc, OPT_Wpessimizing_move,
+			      "moving a temporary object in a return statement "
+			      "prevents copy elision"))
+		inform (loc, "remove %<std::move%> call");
+	    }
 	  /* Warn if the move is redundant.  It is redundant when we would
 	     do maybe-rvalue overload resolution even without std::move.  */
 	  else if (warn_redundant_move
 		   && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
 	    {
-	      /* Make sure that the overload resolution would actually succeed
+	      /* Make sure that overload resolution would actually succeed
 		 if we removed the std::move call.  */
 	      tree t = convert_for_initialization (NULL_TREE, functype,
 						   moved,
diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
new file mode 100644
index 00000000000..cd4eaa09aae
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
@@ -0,0 +1,63 @@ 
+// PR c++/106276
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wpessimizing-move" }
+
+// Define std::move.
+namespace std {
+  template<typename _Tp>
+    struct remove_reference
+    { typedef _Tp   type; };
+
+  template<typename _Tp>
+    struct remove_reference<_Tp&>
+    { typedef _Tp   type; };
+
+  template<typename _Tp>
+    struct remove_reference<_Tp&&>
+    { typedef _Tp   type; };
+
+  template<typename _Tp>
+    constexpr typename std::remove_reference<_Tp>::type&&
+    move(_Tp&& __t) noexcept
+    { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); }
+}
+
+struct A { A(); A(const A&) = delete; A(A&&); };
+struct B { B(A); };
+
+static A foo ();
+
+A
+fn1 ()
+{
+  return std::move (A{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
+  return std::move (A()); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
+  return std::move (foo ()); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
+}
+
+B fn2 ()
+{
+  return std::move (A());
+  return std::move (A{});
+  return std::move (foo ());
+}
+
+template <typename T1, typename T2>
+T1
+fn3 ()
+{
+  return std::move (T2{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
+}
+
+void
+do_fn3 ()
+{
+  fn3<A, A>();
+  fn3<B, A>();
+}
+
+char take_buffer;
+struct label_text {
+  label_text take() { return std::move(label_text(&take_buffer)); } // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
+  label_text(char *);
+};