[RFA,3/3] Handle DW_TAG_variant_part and DW_TAG_variant

Message ID 20180220190613.24148-4-tom@tromey.com
State New
Headers show
Series
  • Discriminated unions
Related show

Commit Message

Tom Tromey Feb. 20, 2018, 7:06 p.m.
This changes dwarf2read to understand DW_TAG_variant_part and
DW_TAG_variant.

Note that DW_AT_discr_list is not handled.  I did not need this for
Rust.  I imagine this should not be too hard to add later, should
someone need it.  Meanwhile I have gdb emit a complaint if it is seen.

There is a lurking issue concerning the placement of the discriminant
in the DWARF.  For Rust, I ended up following the letter of the
standard and having the discriminant be a child of the
DW_TAG_variant_part.  However, GCC's Ada support does not do this.
Pierre-Marie filed this with the DWARF committee:

    http://dwarfstd.org/ShowIssue.php?issue=180123.1

However as that is read-only, if you have comments you might consider
adding them to the GCC bug:

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83935

Finally, there is a DWARF extension lurking in here.  In Rust, a
univariant enum will not have a discriminant.  However, in order to
unify the representation of all data-carrying enums, I've made LLVM
(and my forthcoming rustc patch) emit a univariant enum using a
DW_TAG_variant with a single variant part and without DW_AT_discr.
The lack of this DW_AT_discr is the extension.  I will submit an issue
on dwarfstd.org about this.

2018-02-19  Tom Tromey  <tom@tromey.com>

	* dwarf2read.c (struct nextfield) <discriminant_value,
	default_branch, is_discriminant>: New fields.
	(dwarf2_add_field): Handle DW_TAG_variant_part.
	(dwarf2_attach_fields_to_type): Attach a discriminant_info to a
	discriminated union.
	(read_structure_type): Handle DW_TAG_variant_part.
	(handle_struct_member_die): New function, extracted from
	process_structure_scope.  Handle DW_TAG_variant.
	(process_structure_scope): Handle discriminated unions.  Call
	handle_struct_member_die.

2018-02-20  Tom Tromey  <tom@tromey.com>

	* gdb.dwarf2/variant.c: New file.
	* gdb.dwarf2/variant.exp: New file.
---
 gdb/ChangeLog                        |  13 +++
 gdb/dwarf2read.c                     | 208 ++++++++++++++++++++++++++++-------
 gdb/testsuite/ChangeLog              |   5 +
 gdb/testsuite/gdb.dwarf2/variant.c   |  42 +++++++
 gdb/testsuite/gdb.dwarf2/variant.exp | 175 +++++++++++++++++++++++++++++
 5 files changed, 406 insertions(+), 37 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/variant.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/variant.exp

-- 
2.13.6

Comments

Joel Brobecker Feb. 21, 2018, 5:12 a.m. | #1
> 2018-02-19  Tom Tromey  <tom@tromey.com>

> 

> 	* dwarf2read.c (struct nextfield) <discriminant_value,

> 	default_branch, is_discriminant>: New fields.

> 	(dwarf2_add_field): Handle DW_TAG_variant_part.

> 	(dwarf2_attach_fields_to_type): Attach a discriminant_info to a

> 	discriminated union.

> 	(read_structure_type): Handle DW_TAG_variant_part.

> 	(handle_struct_member_die): New function, extracted from

> 	process_structure_scope.  Handle DW_TAG_variant.

> 	(process_structure_scope): Handle discriminated unions.  Call

> 	handle_struct_member_die.

> 

> 2018-02-20  Tom Tromey  <tom@tromey.com>

> 

> 	* gdb.dwarf2/variant.c: New file.

> 	* gdb.dwarf2/variant.exp: New file.


Looks good to me as well. One suggestion below.

Also, thanks for the gdb.dwarf2 testcase!

> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c

> index 12ed4ae33c..db23b1a421 100644

> --- a/gdb/dwarf2read.c

> +++ b/gdb/dwarf2read.c

> @@ -1659,6 +1659,15 @@ struct nextfield

>    int accessibility;

>    int virtuality;

>    struct field field;

> +  /* If we see a DW_TAG_variant, then this will be the discriminant

> +     value.  */

> +  ULONGEST discriminant_value;

> +  /* If we see a DW_TAG_variant, then this will be set if this is the

> +     default branch.  */

> +  bool default_branch;

> +  /* While reading a DW_TAG_variant_part, this will be set if this

> +     field is the discriminant.  */

> +  bool is_discriminant;


Since those 3 new fields are related, what do you think of putting all 3
fields into a new struct, and then add one field in this struct?

> +static void

> +handle_struct_member_die (struct die_info *child_die, struct type *type,

> +			  struct field_info *fi,

> +			  std::vector<struct symbol *> *template_args,

> +			  struct dwarf2_cu *cu)


Thanks for extracting this code out and making it a function! ;-)


-- 
Joel
Tom Tromey Feb. 21, 2018, 5:30 p.m. | #2
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:


Joel> Since those 3 new fields are related, what do you think of putting all 3
Joel> fields into a new struct, and then add one field in this struct?

I've made this change locally.

Tom

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b4c34e0e54..dcbbce1e5d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,18 @@ 
 2018-02-19  Tom Tromey  <tom@tromey.com>
 
+	* dwarf2read.c (struct nextfield) <discriminant_value,
+	default_branch, is_discriminant>: New fields.
+	(dwarf2_add_field): Handle DW_TAG_variant_part.
+	(dwarf2_attach_fields_to_type): Attach a discriminant_info to a
+	discriminated union.
+	(read_structure_type): Handle DW_TAG_variant_part.
+	(handle_struct_member_die): New function, extracted from
+	process_structure_scope.  Handle DW_TAG_variant.
+	(process_structure_scope): Handle discriminated unions.  Call
+	handle_struct_member_die.
+
+2018-02-19  Tom Tromey  <tom@tromey.com>
+
 	* rust-lang.h (rust_last_path_segment): Declare.
 	* rust-lang.c (rust_last_path_segment): Now public.  Change
 	contract.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 12ed4ae33c..db23b1a421 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1659,6 +1659,15 @@  struct nextfield
   int accessibility;
   int virtuality;
   struct field field;
+  /* If we see a DW_TAG_variant, then this will be the discriminant
+     value.  */
+  ULONGEST discriminant_value;
+  /* If we see a DW_TAG_variant, then this will be set if this is the
+     default branch.  */
+  bool default_branch;
+  /* While reading a DW_TAG_variant_part, this will be set if this
+     field is the discriminant.  */
+  bool is_discriminant;
 };
 
 struct nextfnfield
@@ -15599,6 +15608,20 @@  dwarf2_add_field (struct field_info *fip, struct die_info *die,
       FIELD_NAME (*fp) = type_name_no_tag (fp->type);
       fip->nbaseclasses++;
     }
+  else if (die->tag == DW_TAG_variant_part)
+    {
+      /* process_structure_scope will treat this DIE as a union.  */
+      process_structure_scope (die, cu);
+
+      /* The variant part is relative to the start of the enclosing
+	 structure.  */
+      SET_FIELD_BITPOS (*fp, 0);
+      fp->type = get_die_type (die, cu);
+      fp->artificial = 1;
+      fp->name = "<<variant>>";
+    }
+  else
+    gdb_assert_not_reached ("missing case in dwarf2_add_field");
 }
 
 /* Can the type given by DIE define another type?  */
@@ -15724,6 +15747,27 @@  dwarf2_attach_fields_to_type (struct field_info *fip, struct type *type,
       TYPE_N_BASECLASSES (type) = fip->nbaseclasses;
     }
 
+  if (TYPE_FLAG_DISCRIMINATED_UNION (type))
+    {
+      struct discriminant_info *di = alloc_discriminant_info (type, -1, -1);
+
+      int index = nfields - 1;
+      struct nextfield *field = fip->fields;
+
+      while (index >= 0)
+	{
+	  if (field->is_discriminant)
+	    di->discriminant_index = index;
+	  else if (field->default_branch)
+	    di->default_index = index;
+	  else
+	    di->discriminants[index] = field->discriminant_value;
+
+	  --index;
+	  field = field->next;
+	}
+    }
+
   /* Copy the saved-up fields into the field vector.  Start from the head of
      the list, adding to the tail of the field array, so that they end up in
      the same order in the array in which they were added to the list.  */
@@ -16185,6 +16229,11 @@  read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
     {
       TYPE_CODE (type) = TYPE_CODE_UNION;
     }
+  else if (die->tag == DW_TAG_variant_part)
+    {
+      TYPE_CODE (type) = TYPE_CODE_UNION;
+      TYPE_FLAG_DISCRIMINATED_UNION (type) = 1;
+    }
   else
     {
       TYPE_CODE (type) = TYPE_CODE_STRUCT;
@@ -16244,6 +16293,92 @@  read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
   return type;
 }
 
+/* A helper for process_structure_scope that handles a single member
+   DIE.  */
+
+static void
+handle_struct_member_die (struct die_info *child_die, struct type *type,
+			  struct field_info *fi,
+			  std::vector<struct symbol *> *template_args,
+			  struct dwarf2_cu *cu)
+{
+  if (child_die->tag == DW_TAG_member
+      || child_die->tag == DW_TAG_variable
+      || child_die->tag == DW_TAG_variant_part)
+    {
+      /* NOTE: carlton/2002-11-05: A C++ static data member
+	 should be a DW_TAG_member that is a declaration, but
+	 all versions of G++ as of this writing (so through at
+	 least 3.2.1) incorrectly generate DW_TAG_variable
+	 tags for them instead.  */
+      dwarf2_add_field (fi, child_die, cu);
+    }
+  else if (child_die->tag == DW_TAG_subprogram)
+    {
+      /* Rust doesn't have member functions in the C++ sense.
+	 However, it does emit ordinary functions as children
+	 of a struct DIE.  */
+      if (cu->language == language_rust)
+	read_func_scope (child_die, cu);
+      else
+	{
+	  /* C++ member function.  */
+	  dwarf2_add_member_fn (fi, child_die, type, cu);
+	}
+    }
+  else if (child_die->tag == DW_TAG_inheritance)
+    {
+      /* C++ base class field.  */
+      dwarf2_add_field (fi, child_die, cu);
+    }
+  else if (type_can_define_types (child_die))
+    dwarf2_add_type_defn (fi, child_die, cu);
+  else if (child_die->tag == DW_TAG_template_type_param
+	   || child_die->tag == DW_TAG_template_value_param)
+    {
+      struct symbol *arg = new_symbol (child_die, NULL, cu);
+
+      if (arg != NULL)
+	template_args->push_back (arg);
+    }
+  else if (child_die->tag == DW_TAG_variant)
+    {
+      /* In a variant we want to get the discriminant and also add a
+	 field for our sole member child.  */
+      struct attribute *discr = dwarf2_attr (child_die, DW_AT_discr_value, cu);
+
+      for (struct die_info *variant_child = child_die->child;
+	   variant_child != NULL;
+	   variant_child = sibling_die (variant_child))
+	{
+	  if (variant_child->tag == DW_TAG_member)
+	    {
+	      handle_struct_member_die (variant_child, type, fi,
+					template_args, cu);
+	      /* Only handle the one.  */
+	      break;
+	    }
+	}
+
+      /* We don't handle this but we might as well report it if we see
+	 it.  */
+      if (dwarf2_attr (child_die, DW_AT_discr_list, cu) != nullptr)
+	  complaint (&symfile_complaints,
+		     _("DW_AT_discr_list is not supported yet"
+		       " - DIE at 0x%x [in module %s]"),
+		     to_underlying (child_die->sect_off),
+		     objfile_name (cu->per_cu->dwarf2_per_objfile->objfile));
+
+      /* The first field was just added, so we can stash the
+	 discriminant there.  */
+      gdb_assert (fi->fields != NULL);
+      if (discr == NULL)
+	fi->fields->default_branch = true;
+      else
+	fi->fields->discriminant_value = DW_UNSND (discr);
+    }
+}
+
 /* Finish creating a structure or union type, including filling in
    its members and creating a symbol for it.  */
 
@@ -16258,6 +16393,39 @@  process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
   if (type == NULL)
     type = read_structure_type (die, cu);
 
+  /* When reading a DW_TAG_variant_part, we need to notice when we
+     read the discriminant member, so we can record it later in the
+     discriminant_info.  */
+  bool is_variant_part = TYPE_FLAG_DISCRIMINATED_UNION (type);
+  sect_offset discr_offset;
+
+  if (is_variant_part)
+    {
+      struct attribute *discr = dwarf2_attr (die, DW_AT_discr, cu);
+      if (discr == NULL)
+	{
+	  /* Maybe it's a univariant form, an extension we support.
+	     In this case arrange not to check the offset.  */
+	  is_variant_part = false;
+	}
+      else if (attr_form_is_ref (discr))
+	{
+	  struct dwarf2_cu *target_cu = cu;
+	  struct die_info *target_die = follow_die_ref (die, discr, &target_cu);
+
+	  discr_offset = target_die->sect_off;
+	}
+      else
+	{
+	  complaint (&symfile_complaints,
+		     _("DW_AT_discr does not have DIE reference form"
+		       " - DIE at 0x%x [in module %s]"),
+		     to_underlying (die->sect_off),
+		     objfile_name (cu->per_cu->dwarf2_per_objfile->objfile));
+	  is_variant_part = false;
+	}
+    }
+
   if (die->child != NULL && ! die_is_declaration (die, cu))
     {
       struct field_info fi;
@@ -16270,44 +16438,10 @@  process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
 
       while (child_die && child_die->tag)
 	{
-	  if (child_die->tag == DW_TAG_member
-	      || child_die->tag == DW_TAG_variable)
-	    {
-	      /* NOTE: carlton/2002-11-05: A C++ static data member
-		 should be a DW_TAG_member that is a declaration, but
-		 all versions of G++ as of this writing (so through at
-		 least 3.2.1) incorrectly generate DW_TAG_variable
-		 tags for them instead.  */
-	      dwarf2_add_field (&fi, child_die, cu);
-	    }
-	  else if (child_die->tag == DW_TAG_subprogram)
-	    {
-	      /* Rust doesn't have member functions in the C++ sense.
-		 However, it does emit ordinary functions as children
-		 of a struct DIE.  */
-	      if (cu->language == language_rust)
-		read_func_scope (child_die, cu);
-	      else
-		{
-		  /* C++ member function.  */
-		  dwarf2_add_member_fn (&fi, child_die, type, cu);
-		}
-	    }
-	  else if (child_die->tag == DW_TAG_inheritance)
-	    {
-	      /* C++ base class field.  */
-	      dwarf2_add_field (&fi, child_die, cu);
-	    }
-	  else if (type_can_define_types (child_die))
-	    dwarf2_add_type_defn (&fi, child_die, cu);
-	  else if (child_die->tag == DW_TAG_template_type_param
-		   || child_die->tag == DW_TAG_template_value_param)
-	    {
-	      struct symbol *arg = new_symbol (child_die, NULL, cu);
+	  handle_struct_member_die (child_die, type, &fi, &template_args, cu);
 
-	      if (arg != NULL)
-		template_args.push_back (arg);
-	    }
+	  if (is_variant_part && discr_offset == child_die->sect_off)
+	    fi.fields->is_discriminant = true;
 
 	  child_die = sibling_die (child_die);
 	}
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c7a2142f65..b62a24466b 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2018-02-20  Tom Tromey  <tom@tromey.com>
+
+	* gdb.dwarf2/variant.c: New file.
+	* gdb.dwarf2/variant.exp: New file.
+
 2018-02-19  Tom Tromey  <tom@tromey.com>
 
 	* gdb.rust/simple.exp: Accept more possible results in enum test.
diff --git a/gdb/testsuite/gdb.dwarf2/variant.c b/gdb/testsuite/gdb.dwarf2/variant.c
new file mode 100644
index 0000000000..e0feadbcae
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/variant.c
@@ -0,0 +1,42 @@ 
+/* Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+unsigned char buffer[8];
+
+static void
+func (void)
+{
+}
+
+int
+main (void)
+{
+  /* Write the expected values into the buffer.  */
+  unsigned int x = 23;
+  if (*(char *) &x)
+    {
+      /* Little endian.  */
+      buffer[0] = 23;
+      buffer[4] = 23;
+    }
+  else
+    {
+      buffer[3] = 23;
+      buffer[7] = 23;
+    }
+
+  func ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/variant.exp b/gdb/testsuite/gdb.dwarf2/variant.exp
new file mode 100644
index 0000000000..d182182eff
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/variant.exp
@@ -0,0 +1,175 @@ 
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test DW_TAG_variant_part and DW_TAG_variant.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use
+# gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .c variant.S
+
+# Make some DWARF for the test.
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    upvar cu_lang cu_lang
+
+    declare_labels uinteger_label float_label
+    declare_labels discr_1_label discr_2_label
+    declare_labels one_label two_label
+
+    # Creating a CU with 4-byte addresses lets this test link on
+    # both 32- and 64-bit machines.
+    cu { addr_size 4 } {
+	compile_unit {
+	    {name file1.txt}
+	    {language @DW_LANG_Rust}
+	} {
+            uinteger_label: DW_TAG_base_type {
+                {DW_AT_byte_size 4 DW_FORM_sdata}
+                {DW_AT_encoding  @DW_ATE_unsigned}
+                {DW_AT_name      {unsigned integer}}
+            }
+
+	    float_label: base_type {
+		{name float}
+		{encoding @DW_ATE_float}
+		{byte_size 4 DW_FORM_sdata}
+	    }
+
+	    one_label: structure_type {
+		{name One}
+		{byte_size 4 DW_FORM_sdata}
+	    } {
+		member {
+		    {name __0}
+		    {type :$uinteger_label}
+		    {data_member_location 0 data1}
+		}
+	    }
+
+	    two_label: structure_type {
+		{name Two}
+		{byte_size 4 DW_FORM_sdata}
+	    } {
+		member {
+		    {name __0}
+		    {type :$float_label}
+		    {data_member_location 0 data1}
+		}
+	    }
+
+	    structure_type {
+		{name Simple}
+		{byte_size 8 DW_FORM_sdata}
+	    } {
+		variant_part {
+		    {discr :$discr_1_label DW_FORM_ref4}
+		} {
+		    discr_1_label: member {
+			{type :$uinteger_label}
+			{data_member_location 0 data1}
+		    }
+
+		    variant {
+			{discr_value 23 udata}
+		    } {
+			member {
+			    {type :$one_label}
+			    {data_member_location 4 data1}
+			}
+		    }
+
+		    variant {
+			{discr_value 1 udata}
+		    } {
+			member {
+			    {type :$two_label}
+			    {data_member_location 4 data1}
+			}
+		    }
+		}
+	    }
+
+	    structure_type {
+		{name Defaulted}
+		{byte_size 8 DW_FORM_sdata}
+	    } {
+		variant_part {
+		    {discr :$discr_2_label DW_FORM_ref4}
+		} {
+		    discr_2_label: member {
+			{type :$uinteger_label}
+			{data_member_location 0 data1}
+		    }
+
+		    variant {
+		    } {
+			member {
+			    {type :$one_label}
+			    {data_member_location 4 data1}
+			}
+		    }
+
+		    variant {
+			{discr_value 1 udata}
+		    } {
+			member {
+			    {type :$two_label}
+			    {data_member_location 4 data1}
+			}
+		    }
+		}
+	    }
+
+	    structure_type {
+		{name Univariant}
+		{byte_size 8 DW_FORM_sdata}
+	    } {
+		variant_part {
+		} {
+		    variant {
+		    } {
+			member {
+			    {type :$one_label}
+			    {data_member_location 4 data1}
+			}
+		    }
+		}
+	    }
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile $asm_file] debug] } {
+    return -1
+}
+
+if ![runto func] {
+    return -1
+}
+
+# Get the value into history so we can use it from Rust.
+gdb_test "print (void *) buffer" "\\\$1 = .void .. $hex .buffer."
+
+gdb_test "set language rust"
+gdb_test "print *(\$1 as *mut Simple)" " = One\\(23\\)"
+gdb_test "print *(\$1 as *mut Defaulted)" " = One\\(23\\)"
+gdb_test "print *(\$1 as *mut Univariant)" " = One\\(23\\)"