Bug 87150 - [8 Regression] move ctor wrongly chosen in return stmt (derived vs. base)
Summary: [8 Regression] move ctor wrongly chosen in return stmt (derived vs. base)
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 8.0
: P2 normal
Target Milestone: 8.3
Assignee: Marek Polacek
URL:
Keywords: accepts-invalid, wrong-code
Depends on:
Blocks:
 
Reported: 2018-08-30 07:40 UTC by Stephan Bergmann
Modified: 2019-08-13 01:40 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work: 7.3.1
Known to fail: 8.1.0
Last reconfirmed: 2018-08-30 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Bergmann 2018-08-30 07:40:13 UTC
struct S1 { S1(S1 &&); };
  struct S2: S1 {};
  S1 f(S2 s) { return s; }

is accepted by GCC 8.1 but violates [class.copy.elision]/3, which dismisses the move ctor if "the type of the first parameter of the selected constructor is not an rvalue reference to the object’s type (possibly cv-qualified)" (and which is the case here, where the object's type is S2 but the ctor parameter's type is S1&&.  (Referring to C++17, but wording is similar back to C++11, and GCC's behavior is the same for -std=c++{11,14,17}.)

Checking on godbolt.org, the error of accepting the code appears to have been introduced between GCC 7.3 and 8.1.

(Also see mail thread starting at <http://lists.llvm.org/pipermail/cfe-dev/2018-August/059186.html> "return lvalue move instead of copy?".)
Comment 1 Richard Biener 2018-08-30 08:01:33 UTC
Confirmed.
Comment 2 Jakub Jelinek 2018-08-30 12:32:10 UTC
This changed with r251035 aka PR80452 aka C++ Core issue 1579.
So, is this really invalid?
Comment 3 Stephan Bergmann 2018-08-30 12:51:40 UTC
(In reply to Jakub Jelinek from comment #2)
> This changed with r251035 aka PR80452 aka C++ Core issue 1579.
> So, is this really invalid?

but CWG1579 didn't change the "if the type of the first parameter of the selected constructor is not an rvalue reference to the object’s type (possibly cv-qualified), overload resolution is performed again, considering the object as an
lvalue." part
Comment 4 Jonathan Wakely 2018-08-30 12:57:50 UTC
Right. CWG 1579 says we should move here:

struct A { };
struct B { B(A&&) { } };
B f() { A a; return a; }

That's a different case though.
Comment 5 Jakub Jelinek 2018-08-30 12:59:26 UTC
r251035 clearly has code that handles that rule:
+
+      if (flags & LOOKUP_PREFER_RVALUE)
+	{
+	  /* The implicit move specified in 15.8.3/3 fails "...if the type of
+	     the first parameter of the selected constructor is not an rvalue
+	     reference to the object’s type (possibly cv-qualified)...." */
+	  gcc_assert (!(complain & tf_error));
+	  tree ptype = convs[0]->type;
+	  if (TREE_CODE (ptype) != REFERENCE_TYPE
+	      || !TYPE_REF_IS_RVALUE (ptype)
+	      || CONVERSION_RANK (convs[0]) > cr_exact)
+	    return error_mark_node;
+	}

The thing is that LOOKUP_PREFER_RVALUE isn't set in this case, as we trigger first:
  else if (MAYBE_CLASS_TYPE_P (to) && MAYBE_CLASS_TYPE_P (from)
           && is_properly_derived_from (from, to))
    {
      if (conv->kind == ck_rvalue)
        conv = next_conversion (conv);
      conv = build_conv (ck_base, to, conv);
      /* The derived-to-base conversion indicates the initialization
         of a parameter with base type from an object of a derived
         type.  A temporary object is created to hold the result of
         the conversion unless we're binding directly to a reference.  */
      conv->need_temporary_p = !(flags & LOOKUP_NO_TEMP_BIND);
    }
  else
    return NULL;
in standard_conversion, where
*conv
$17 = {kind = ck_rvalue, rank = cr_identity, user_conv_p = 0, ellipsis_p = 0, this_p = 0, bad_p = 0, need_temporary_p = 0, base_p = 0, 
  rvaluedness_matches_p = 1, check_narrowing = 0, check_narrowing_const_only = 0, type = <record_type 0x7fffefdd32a0 S2>, u = {next = 0x2f92e80, 
    expr = <error_mark 0x2f92e80>, list = 0x2f92e80}, cand = 0x0}
and
*next_conversion (conv)
$18 = {kind = ck_identity, rank = cr_identity, user_conv_p = 0, ellipsis_p = 0, this_p = 0, bad_p = 0, need_temporary_p = 0, base_p = 0, 
  rvaluedness_matches_p = 0, check_narrowing = 0, check_narrowing_const_only = 0, type = <record_type 0x7fffefdd32a0 S2>, u = {
    next = 0x7fffefdbd6c0, expr = <indirect_ref 0x7fffefdbd6c0>, list = 0x7fffefdbd6c0}, cand = 0x0}
so we are no longer using ck_rvalue conversion with rvaluedness_matches_p, but ck_base conversion instead.
Comment 6 Jakub Jelinek 2018-08-30 13:57:13 UTC
Perhaps that
      if (conv->kind == ck_rvalue)
        conv = next_conversion (conv);
shouldn't be done if (flags & LOOKUP_PREFER_RVALUE) or if conv->rvaluedness_matches_p?  Just a wild guess though, that code dates back to:
Fri Feb 13 14:55:37 1998  Jason Merrill  <jason@yorick.cygnus.com>

        * call.c (standard_conversion): Fix multi-level ptr conversions.
Comment 7 Marek Polacek 2018-08-31 19:45:37 UTC
This feels very much like 87109.
Comment 8 Marek Polacek 2018-09-07 14:38:16 UTC
It appears that the sentiment is that this testcase should actually be valid; suspending until then.
Comment 9 Stephan Bergmann 2018-09-07 14:54:01 UTC
(In reply to Marek Polacek from comment #8)
> It appears that the sentiment is that this testcase should actually be
> valid

Do you have a reference for that?  The reason for this not to be valid, presented at the bottom of <http://lists.llvm.org/pipermail/cfe-dev/2018-August/059190.html> "Re: [cfe-dev] return lvalue move instead of copy?" looks rather convincing to me.
Comment 10 Marek Polacek 2018-09-07 15:00:24 UTC
I suspended this in view of <https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00428.html>.
Comment 11 Jason Merrill 2018-09-08 12:29:03 UTC
(In reply to Stephan Bergmann from comment #9)
> (In reply to Marek Polacek from comment #8)
> > It appears that the sentiment is that this testcase should actually be
> > valid
> 
> Do you have a reference for that?  The reason for this not to be valid,
> presented at the bottom of
> <http://lists.llvm.org/pipermail/cfe-dev/2018-August/059190.html> "Re:
> [cfe-dev] return lvalue move instead of copy?" looks rather convincing to me.

(Namely, slicing by move leaving the object in a partially-moved state that might not maintain invariants.)

That's a good point.  I guess the rule we're looking for will still require that the returned object being treated as an xvalue be bound to some rvalue reference to the whole object, rather than a base subobject.

So Marek, please go ahead and apply your patch after all.  And similarly, maybe refine your 87109 patch to only reject the conversion function if it comes from a base class.
Comment 12 Marek Polacek 2018-09-08 15:15:46 UTC
(In reply to Jason Merrill from comment #11)
> (In reply to Stephan Bergmann from comment #9)
> > (In reply to Marek Polacek from comment #8)
> > > It appears that the sentiment is that this testcase should actually be
> > > valid
> > 
> > Do you have a reference for that?  The reason for this not to be valid,
> > presented at the bottom of
> > <http://lists.llvm.org/pipermail/cfe-dev/2018-August/059190.html> "Re:
> > [cfe-dev] return lvalue move instead of copy?" looks rather convincing to me.
> 
> (Namely, slicing by move leaving the object in a partially-moved state that
> might not maintain invariants.)
> 
> That's a good point.  I guess the rule we're looking for will still require
> that the returned object being treated as an xvalue be bound to some rvalue
> reference to the whole object, rather than a base subobject.
> 
> So Marek, please go ahead and apply your patch after all.  

Will do.

> And similarly,
> maybe refine your 87109 patch to only reject the conversion function if it
> comes from a base class.

I'll send a patch to the ML, I don't feel like committing that without a review.
Comment 13 Marek Polacek 2018-09-08 17:36:40 UTC
Author: mpolacek
Date: Sat Sep  8 17:36:08 2018
New Revision: 264172

URL: https://gcc.gnu.org/viewcvs?rev=264172&root=gcc&view=rev
Log:
	PR c++/87150 - wrong ctor with maybe-rvalue semantics.
	* call.c (struct conversion): Update commentary.
	(standard_conversion): Set rvaluedness_matches_p if LOOKUP_PREFER_RVALUE
	for ck_base.

	* g++.dg/cpp0x/move-return2.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/cpp0x/move-return2.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/call.c
    trunk/gcc/testsuite/ChangeLog
Comment 14 Marek Polacek 2018-09-08 17:37:45 UTC
Fixed.  (I don't think this should be backported to 8.)
Comment 15 Stephan Bergmann 2018-09-10 12:20:06 UTC
I see that with the fix from comment 13 included, the slightly changed source

  #include <utility>
  struct S1 { S1(S1 &&); };
  struct S2: S1 {};
  S1 f(S2 s) { return std::move(s); }

causes -Wredundant-move (when that warning is explicitly requested).
Comment 16 Stephan Bergmann 2018-09-21 12:07:18 UTC
(In reply to Stephan Bergmann from comment #15)
> I see that with the fix from comment 13 included, the slightly changed source
> 
>   #include <utility>
>   struct S1 { S1(S1 &&); };
>   struct S2: S1 {};
>   S1 f(S2 s) { return std::move(s); }
> 
> causes -Wredundant-move (when that warning is explicitly requested).

Shall I file a separate issue for that?
Comment 17 Jonathan Wakely 2018-09-21 13:02:23 UTC
Yes please.
Comment 18 Stephan Bergmann 2018-09-21 14:20:42 UTC
(In reply to Jonathan Wakely from comment #17)
> Yes please.

bug 87378
Comment 19 Jason Merrill 2019-08-12 18:34:09 UTC
And now P1155/P1825 removes that text, so in C++20 mode the original testcase needs to call the move constructor again.  Marek, I don't see P1825R0 in cxx-status.html, was there a reason not to add it?
Comment 20 Marek Polacek 2019-08-12 18:40:02 UTC
(In reply to Jason Merrill from comment #19)
> And now P1155/P1825 removes that text, so in C++20 mode the original
> testcase needs to call the move constructor again.  Marek, I don't see
> P1825R0 in cxx-status.html, was there a reason not to add it?

No, I followed clang's table but they're missing that one.  I'll fix that & open a PR for P1825R0.
Comment 21 Jason Merrill 2019-08-12 20:26:51 UTC
(In reply to Jason Merrill from comment #19)
> And now P1155/P1825 removes that text, so in C++20 mode the original
> testcase needs to call the move constructor again.

Actually, it's a DR, so not just C++20 mode.
Comment 22 Richard Smith 2019-08-12 23:22:43 UTC
(In reply to Marek Polacek from comment #20)
> (In reply to Jason Merrill from comment #19)
> > And now P1155/P1825 removes that text, so in C++20 mode the original
> > testcase needs to call the move constructor again.  Marek, I don't see
> > P1825R0 in cxx-status.html, was there a reason not to add it?
> 
> No, I followed clang's table but they're missing that one.  I'll fix that &
> open a PR for P1825R0.

FYI, it is in Clang's table, but I put it in the C++11 features list since it's a DR and we're considering it to retroactively apply to C++11's addition of move semantics.
Comment 23 Marek Polacek 2019-08-12 23:30:35 UTC
Thanks, missed that.  Looks like Jason wants us to fix it retroactively, too.  I suppose we should just revert my r264172.
Comment 24 Jason Merrill 2019-08-13 01:35:06 UTC
(In reply to Marek Polacek from comment #23)
> Thanks, missed that.  Looks like Jason wants us to fix it retroactively,
> too.  I suppose we should just revert my r264172.

Better to disable the section in build_over_call that implemented the check removed by this paper:

           /* The implicit move specified in 15.8.3/3 fails "...if the type of
              the first parameter of the selected constructor is not an rvalue
              reference to the object’s type (possibly cv-qualified)...." */

I wouldn't remove it entirely until we have a better idea of what gets broken by this change.