PR symtab/14441 - rvalue references

PR symtab/14441 - rvalue references

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2104 bytes --]

Hi,

I'm trying to fix http://sourceware.org/bugzilla/show_bug.cgi?id=14441
by adding support for rvalue references.  I plan to contribute this if
I get it finished before anyone else fixes it, so I'll get a copyright
assignment for GDB filed with the FSF clerk.

My first attempt, shown in the attachment, was very simple:

* added an rvalue_reference_type field to struct type
* modified make_reference_type() to take an extra parameter indicating
whether the type was an rvalue reference or lvalue reference
* call the modified make_reference_type() when the debug info contains
DW_TAG_rvalue_reference_type
* adjust c_type_print_varspec_prefix() to print "&&" when type ==
TYPE_RVALUE_REFERENCE_TYPE (TYPE_TARGET_TYPE (type)) i.e. when the
reference being printed is the target type's rvalue_reference_type,
not its reference_type.

That worked well enough to fix the <unknown type ...> problem shown in
comment 2 on the PR, but didn't solve all cases.  Saying "print r" or
"ptype r" for an rvalue reference type displayed the type with a
single ampersand rather than two.

So my second attempt is more invasive, adding TYPE_CODE_RVAL_REF and
changing everywhere that uses TYPE_CODE_REF to also handle the new
type code.  This gets me closer, fixing how types are displayed so now
I get:

(gdb) n
10        X&& xrr = X();
(gdb)
11        return;
(gdb) p xrr
$1 = (X &&) @0x7fffffffdd2f: {<No data fields>}
(gdb) ptype xrr
type = struct X {
  public:
    X & operator=(const X &);
    X & operator=(X &&);
} &&

This is all great, but I'm not finished yet. I still get the wrong
output if I take the address of an rvalue reference:

(gdb) p &xrr
$2 = (X &&*) 0x7fffffffdd30

As with a normal (lvalue) reference I should get the address of the
reference's target, not the reference itself.

Before I continue altering every function that checks TYPE_CODE_REF,
is my approach the right one?  Is adding TYPE_CODE_RVAL_REF and
touching dozens of functions the best approach, or should I go back to
my first attempt and just fix the few places that need to handle
lvalue and rvalue references differently?

[-- Attachment #2: patch-1.txt --]
[-- Type: text/plain, Size: 8124 bytes --]

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 8b5bc21..c5aa2c5 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -280,7 +280,10 @@ c_type_print_varspec_prefix (struct type *type,
     case TYPE_CODE_REF:
       c_type_print_varspec_prefix (TYPE_TARGET_TYPE (type),
 				   stream, show, 1, 0);
-      fprintf_filtered (stream, "&");
+      if (TYPE_RVALUE_REFERENCE_TYPE (TYPE_TARGET_TYPE (type)) == type)
+	fprintf_filtered (stream, "&&");
+      else
+	fprintf_filtered (stream, "&");
       c_type_print_modifier (type, stream, 1, need_post_space);
       break;
 
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 2dcba20..00a7ef7 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -7069,6 +7069,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu)
     case DW_TAG_pointer_type:
     case DW_TAG_ptr_to_member_type:
     case DW_TAG_reference_type:
+    case DW_TAG_rvalue_reference_type:
     case DW_TAG_string_type:
       break;
 
@@ -11481,7 +11482,8 @@ read_tag_ptr_to_member_type (struct die_info *die, struct dwarf2_cu *cu)
    the user defined type vector.  */
 
 static struct type *
-read_tag_reference_type (struct die_info *die, struct dwarf2_cu *cu)
+do_read_tag_reference_type (struct die_info *die, struct dwarf2_cu *cu,
+			    int rvalue_ref)
 {
   struct comp_unit_head *cu_header = &cu->header;
   struct type *type, *target_type;
@@ -11494,7 +11496,11 @@ read_tag_reference_type (struct die_info *die, struct dwarf2_cu *cu)
   if (type)
     return type;
 
-  type = lookup_reference_type (target_type);
+  if (rvalue_ref)
+    type = lookup_rvalue_reference_type (target_type);
+  else
+    type = lookup_reference_type (target_type);
+
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
   if (attr)
     {
@@ -11508,6 +11514,18 @@ read_tag_reference_type (struct die_info *die, struct dwarf2_cu *cu)
 }
 
 static struct type *
+read_tag_reference_type (struct die_info *die, struct dwarf2_cu *cu)
+{
+  return do_read_tag_reference_type (die, cu, 0);
+}
+
+static struct type *
+read_tag_rvalue_reference_type (struct die_info *die, struct dwarf2_cu *cu)
+{
+  return do_read_tag_reference_type (die, cu, 1);
+}
+
+static struct type *
 read_tag_const_type (struct die_info *die, struct dwarf2_cu *cu)
 {
   struct type *base_type, *cv_type;
@@ -15721,6 +15739,9 @@ read_type_die_1 (struct die_info *die, struct dwarf2_cu *cu)
     case DW_TAG_reference_type:
       this_type = read_tag_reference_type (die, cu);
       break;
+    case DW_TAG_rvalue_reference_type:
+      this_type = read_tag_rvalue_reference_type (die, cu);
+      break;
     case DW_TAG_const_type:
       this_type = read_tag_const_type (die, cu);
       break;
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 149d31f..a588066 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -359,15 +359,21 @@ lookup_pointer_type (struct type *type)
 /* Lookup a C++ `reference' to a type TYPE.  TYPEPTR, if nonzero,
    points to a pointer to memory where the reference type should be
    stored.  If *TYPEPTR is zero, update it to point to the reference
-   type we return.  We allocate new memory if needed.  */
+   type we return.  We allocate new memory if needed.  If RVALUE_REF
+   is zero lookup lvalue reference type, otherwise rvalue reference
+   type.  */
 
-struct type *
-make_reference_type (struct type *type, struct type **typeptr)
+static struct type *
+do_make_reference_type (struct type *type, struct type **typeptr,
+			int rvalue_ref)
 {
   struct type *ntype;	/* New type */
   struct type *chain;
 
-  ntype = TYPE_REFERENCE_TYPE (type);
+  if (rvalue_ref)
+    ntype = TYPE_RVALUE_REFERENCE_TYPE (type);
+  else
+    ntype = TYPE_REFERENCE_TYPE (type);
 
   if (ntype)
     {
@@ -396,7 +402,10 @@ make_reference_type (struct type *type, struct type **typeptr)
     }
 
   TYPE_TARGET_TYPE (ntype) = type;
-  TYPE_REFERENCE_TYPE (type) = ntype;
+  if (rvalue_ref)
+    TYPE_RVALUE_REFERENCE_TYPE (type) = ntype;
+  else
+    TYPE_REFERENCE_TYPE (type) = ntype;
 
   /* FIXME!  Assume the machine has only one representation for
      references, and that it matches the (only) representation for
@@ -406,9 +415,6 @@ make_reference_type (struct type *type, struct type **typeptr)
     gdbarch_ptr_bit (get_type_arch (type)) / TARGET_CHAR_BIT;
   TYPE_CODE (ntype) = TYPE_CODE_REF;
 
-  if (!TYPE_REFERENCE_TYPE (type))	/* Remember it, if don't have one.  */
-    TYPE_REFERENCE_TYPE (type) = ntype;
-
   /* Update the length of all the other variants of this type.  */
   chain = TYPE_CHAIN (ntype);
   while (chain != ntype)
@@ -420,13 +426,38 @@ make_reference_type (struct type *type, struct type **typeptr)
   return ntype;
 }
 
+/* Same as above, looking up lvalue reference type.  */
+
+struct type *
+make_reference_type (struct type *type, struct type **typeptr)
+{
+  return do_make_reference_type (type, (struct type **) 0, 0);
+}
+
 /* Same as above, but caller doesn't care about memory allocation
    details.  */
 
 struct type *
 lookup_reference_type (struct type *type)
 {
-  return make_reference_type (type, (struct type **) 0);
+  return do_make_reference_type (type, (struct type **) 0, 0);
+}
+
+/* Same as make_reference_type, but looking up rvalue reference type.  */
+
+struct type *
+make_rvalue_reference_type (struct type *type, struct type **typeptr)
+{
+  return do_make_reference_type (type, (struct type **) 0, 1);
+}
+
+/* Same as above, but caller doesn't care about memory allocation
+   details.  */
+
+struct type *
+lookup_rvalue_reference_type (struct type *type)
+{
+  return do_make_reference_type (type, (struct type **) 0, 1);
 }
 
 /* Lookup a function type that returns type TYPE.  TYPEPTR, if
@@ -586,6 +617,7 @@ make_qualified_type (struct type *type, int new_flags,
      the new type.  */
   TYPE_POINTER_TYPE (ntype) = (struct type *) 0;
   TYPE_REFERENCE_TYPE (ntype) = (struct type *) 0;
+  TYPE_RVALUE_REFERENCE_TYPE (ntype) = (struct type *) 0;
 
   /* Chain the new qualified type to the old type.  */
   TYPE_CHAIN (ntype) = TYPE_CHAIN (type);
@@ -3165,6 +3197,9 @@ recursive_dump_type (struct type *type, int spaces)
   printfi_filtered (spaces, "reference_type ");
   gdb_print_host_address (TYPE_REFERENCE_TYPE (type), gdb_stdout);
   printf_filtered ("\n");
+  printfi_filtered (spaces, "rvalue_reference_type ");
+  gdb_print_host_address (TYPE_RVALUE_REFERENCE_TYPE (type), gdb_stdout);
+  printf_filtered ("\n");
   printfi_filtered (spaces, "type_chain ");
   gdb_print_host_address (TYPE_CHAIN (type), gdb_stdout);
   printf_filtered ("\n");
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 59a6a65..6345e7d 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -650,9 +650,10 @@ struct type
 
   struct type *pointer_type;
 
-  /* C++: also need a reference type.  */
+  /* C++: also need reference types.  */
 
   struct type *reference_type;
+  struct type *rvalue_reference_type;
 
   /* Variant chain.  This points to a type that differs from this one only
      in qualifiers and length.  Currently, the possible qualifiers are
@@ -1050,6 +1051,7 @@ extern void allocate_gnat_aux_type (struct type *);
 #define TYPE_TARGET_TYPE(thistype) TYPE_MAIN_TYPE(thistype)->target_type
 #define TYPE_POINTER_TYPE(thistype) (thistype)->pointer_type
 #define TYPE_REFERENCE_TYPE(thistype) (thistype)->reference_type
+#define TYPE_RVALUE_REFERENCE_TYPE(thistype) (thistype)->rvalue_reference_type
 #define TYPE_CHAIN(thistype) (thistype)->chain
 /* Note that if thistype is a TYPEDEF type, you have to call check_typedef.
    But check_typedef does set the TYPE_LENGTH of the TYPEDEF type,
@@ -1475,8 +1477,12 @@ extern struct type *init_vector_type (struct type *elt_type, int n);
 
 extern struct type *lookup_reference_type (struct type *);
 
+extern struct type *lookup_rvalue_reference_type (struct type *);
+
 extern struct type *make_reference_type (struct type *, struct type **);
 
+extern struct type *make_rvalue_reference_type (struct type *, struct type **);
+
 extern struct type *make_cv_type (int, int, struct type *, struct type **);
 
 extern void replace_type (struct type *, struct type *);

Re: PR symtab/14441 - rvalue references

[Tom Tromey]

>>>>> "Jonathan" == Jonathan Wakely <jwakely.gcc@gmail.com> writes:

Jonathan> I'm trying to fix
Jonathan> http://sourceware.org/bugzilla/show_bug.cgi?id=14441 by adding
Jonathan> support for rvalue references.  I plan to contribute this if I
Jonathan> get it finished before anyone else fixes it, so I'll get a
Jonathan> copyright assignment for GDB filed with the FSF clerk.

Very nice, thank you.

Jonathan> Before I continue altering every function that checks TYPE_CODE_REF,
Jonathan> is my approach the right one?  Is adding TYPE_CODE_RVAL_REF and
Jonathan> touching dozens of functions the best approach, or should I go back to
Jonathan> my first attempt and just fix the few places that need to handle
Jonathan> lvalue and rvalue references differently?

I would accept either one.  Abstractly, I suppose a new type code would
be more in keeping with the current design.  But I think the other
choice would be ok, too, if it is a reasonably accurate model of C++.  I
don't know enough about rvalue references to say whether this is the
case.

Tom

Re: PR symtab/14441 - rvalue references

[Jonathan Wakely]

On 29 October 2012 14:25, Tom Tromey wrote:
> Jonathan> Before I continue altering every function that checks TYPE_CODE_REF,
> Jonathan> is my approach the right one?  Is adding TYPE_CODE_RVAL_REF and
> Jonathan> touching dozens of functions the best approach, or should I go back to
> Jonathan> my first attempt and just fix the few places that need to handle
> Jonathan> lvalue and rvalue references differently?
>
> I would accept either one.  Abstractly, I suppose a new type code would
> be more in keeping with the current design.  But I think the other
> choice would be ok, too, if it is a reasonably accurate model of C++.  I
> don't know enough about rvalue references to say whether this is the
> case.

I believe it is the case.

An rvalue-reference can only bind to an rvalue, however once it's
bound to a variable the reference *is* an lvalue, so for all intents
and purposes GDB could treat variables of rvalue-reference type
exactly the same as lvalue references, except for showing "&&" when
printing types.

For example:

struct X { };

X
f (X&& param)
{
  // param is an lvalue here!
  return param;  // return a copy
}

X
g (void)
{
  X x;
  return x;  // return a copy
 }

int main()
{
  X&& rv = f ( g () );
  // rv is an lvalue!
  X* addr = &rv;  // OK to take address
}

In this example, although the function parameter param and variable rv
are rvalue-references and bound to rvalues, they are named variables
and are lvalues.  i.e. the act of binding an rvalue reference to an
rvalue, thereby giving it a name, makes it an lvalue.

Which is why my gut feeling (having barely looked at the gdb code two
days ago) is that rvalue references don't need any special handling
different to existing TYPE_CODE_REF variables, except for "ptype"
printing "X&&" instead of "X&"

Unfortunately my first attempt at doing that didn't work, so I started
adding TYPE_CODE_RVAL_REF and handling it everywhere ... but I'm
having second thoughts.

Re: PR symtab/14441 - rvalue references

[Tom Tromey]

>>>>> "Jonathan" == Jonathan Wakely <jwakely.gcc@gmail.com> writes:

Jonathan> My first attempt, shown in the attachment, was very simple:
[...]
Jonathan> * adjust c_type_print_varspec_prefix() to print "&&" when type ==
Jonathan> TYPE_RVALUE_REFERENCE_TYPE (TYPE_TARGET_TYPE (type)) i.e. when the
Jonathan> reference being printed is the target type's rvalue_reference_type,
Jonathan> not its reference_type.

The above seems weird to me somehow.
I wonder why it didn't work.
FWIW a flag bit on struct main_type would be ok.

Jonathan> Unfortunately my first attempt at doing that didn't work, so I
Jonathan> started adding TYPE_CODE_RVAL_REF and handling it everywhere
Jonathan> ... but I'm having second thoughts.

Yeah.  That patch would be fairly large.
If you want to persevere, though, I will review it, despite its eventual
size.  But like I said, I think either way is ok.

It seems like the expression parser should at least need one change to
parse the new type, so that "ptype int&&" works.

Tom