Handle bit-fields with PROP_ADDR_OFFSET

Message ID 20250415220211.2316256-1-tromey@adacore.com
State New
Headers
Series Handle bit-fields with PROP_ADDR_OFFSET |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed

Commit Message

Tom Tromey April 15, 2025, 10:02 p.m. UTC
  I found a situation where gdb could not properly decode an Ada type
(see the included test case).

In this scenario, the discriminant is a bit-field.  The
PROP_ADDR_OFFSET code does not handle this situation.

This patch changes this code to handle DW_AT_bit_size appropriately,
adding a new member to the baton.  Doing this showed that one of the
handle_member_location overloads incorrectly said it returned the byte
size -- when in fact it incoherently returned either the byte- or
bit-size depending on which path it took.  This bug is fixed here.

Finally, I noticed that if the discriminant field has a biased
representation, then PROP_ADDR_OFFSET would not handle this either.
This bug is also fixed here, and the test case checks this.

Regression tested on x86-64 Fedora 41.
---
 gdb/dwarf2/loc.c                              | 33 ++++++++----
 gdb/dwarf2/loc.h                              | 14 +++--
 gdb/dwarf2/read.c                             | 53 +++++++++++--------
 gdb/testsuite/gdb.ada/packed_record_2.exp     | 47 ++++++++++++++++
 .../gdb.ada/packed_record_2/exam.adb          | 38 +++++++++++++
 gdb/value.c                                   |  3 ++
 6 files changed, 152 insertions(+), 36 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/packed_record_2.exp
 create mode 100644 gdb/testsuite/gdb.ada/packed_record_2/exam.adb


base-commit: 7bef406490d0ecf2e8710a9ed28b0b377653159f
  

Comments

Tom Tromey April 17, 2025, 3:35 p.m. UTC | #1
>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> I found a situation where gdb could not properly decode an Ada type
Tom> (see the included test case).

After I sent this, with Eric's help I found another case that required a
different approach.  So, this patch is obsolete and I'll be sending a v2
series shortly.

Tom
  

Patch

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index e1a5fdd5b8d..ae35b1291d0 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -1736,7 +1736,6 @@  dwarf2_evaluate_property (const dynamic_prop *prop,
       {
 	const dwarf2_property_baton *baton = prop->baton ();
 	const struct property_addr_info *pinfo;
-	struct value *val;
 
 	for (pinfo = addr_stack; pinfo != NULL; pinfo = pinfo->next)
 	  {
@@ -1747,14 +1746,30 @@  dwarf2_evaluate_property (const dynamic_prop *prop,
 	  }
 	if (pinfo == NULL)
 	  error (_("cannot find reference address for offset property"));
-	if (pinfo->valaddr.data () != NULL)
-	  val = value_from_contents
-		  (baton->offset_info.type,
-		   pinfo->valaddr.data () + baton->offset_info.offset);
-	else
-	  val = value_at (baton->offset_info.type,
-			  pinfo->addr + baton->offset_info.offset);
-	*value = value_as_address (val);
+
+	/* Storage for memory if we need to read it.  */
+	gdb::byte_vector memory;
+	const gdb_byte *bytes = pinfo->valaddr.data ();
+	if (bytes == nullptr)
+	  {
+	    /* We read enough bytes to let us use unpack_bits_as_long
+	       without massaging the inputs.  This may read "too much"
+	       but normally this is only used for discriminants and
+	       these appear near the start of objects.  */
+	    ULONGEST bit_size = baton->offset_info.bit_size;
+	    if (bit_size == 0)
+	      bit_size = 8 * check_typedef (baton->offset_info.type)->length ();
+	    ULONGEST bits_to_read = align_up (baton->offset_info.bit_offset
+					      + bit_size,
+					      8);
+	    memory.resize (bits_to_read / 8);
+	    read_memory (pinfo->addr, memory.data (), memory.size ());
+	    bytes = memory.data ();
+	  }
+	*value = unpack_bits_as_long (baton->offset_info.type,
+				      bytes,
+				      baton->offset_info.bit_offset,
+				      baton->offset_info.bit_size);
 	return true;
       }
 
diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
index 30c528b525d..d4c3689cb45 100644
--- a/gdb/dwarf2/loc.h
+++ b/gdb/dwarf2/loc.h
@@ -203,16 +203,20 @@  struct dwarf2_loclist_baton
 };
 
 /* The baton used when a dynamic property is an offset to a parent
-   type.  This can be used, for instance, then the bound of an array
+   type.  This can be used, for instance, when the bound of an array
    inside a record is determined by the value of another field inside
    that record.  */
 
 struct dwarf2_offset_baton
 {
-  /* The offset from the parent type where the value of the property
-     is stored.  In the example provided above, this would be the offset
-     of the field being used as the array bound.  */
-  LONGEST offset;
+  /* The bit offset into the parent type where the value of the
+     property is stored.  In the example provided above, this would be
+     the offset of the field being used as the array bound.  */
+  LONGEST bit_offset;
+
+  /* The bit size of the field of interest.  If this is zero, the
+     type's size is used instead.  */
+  LONGEST bit_size;
 
   /* The type of the object whose property is dynamic.  In the example
      provided above, this would the array's index type.  */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 794c3973cb5..2d7ffdd2aa7 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -9919,20 +9919,20 @@  dwarf2_access_attribute (struct die_info *die, struct dwarf2_cu *cu)
 }
 
 /* Look for DW_AT_data_member_location or DW_AT_data_bit_offset.  Set
-   *OFFSET to the byte offset.  If the attribute was not found return
-   0, otherwise return 1.  If it was found but could not properly be
-   handled, set *OFFSET to 0.  */
+   *BIT_OFFSET to the bit offset.  If the attribute was not found return
+   false, otherwise return true.  If it was found but could not
+   properly be handled, set *BIT_OFFSET to 0.  */
 
-static int
+static bool
 handle_member_location (struct die_info *die, struct dwarf2_cu *cu,
-			LONGEST *offset)
+			LONGEST *bit_offset)
 {
   struct attribute *attr;
 
   attr = dwarf2_attr (die, DW_AT_data_member_location, cu);
   if (attr != NULL)
     {
-      *offset = 0;
+      *bit_offset = 0;
       CORE_ADDR temp;
 
       /* Note that we do not check for a section offset first here.
@@ -9940,30 +9940,28 @@  handle_member_location (struct die_info *die, struct dwarf2_cu *cu,
 	 so if we see it, we can assume that a constant form is really
 	 a constant and not a section offset.  */
       if (attr->form_is_constant ())
-	*offset = attr->constant_value (0);
+	*bit_offset = attr->constant_value (0) * bits_per_byte;
       else if (attr->form_is_section_offset ())
 	dwarf2_complex_location_expr_complaint ();
       else if (attr->form_is_block ()
 	       && decode_locdesc (attr->as_block (), cu, &temp))
-	{
-	  *offset = temp;
-	}
+	*bit_offset = temp * bits_per_byte;
       else
 	dwarf2_complex_location_expr_complaint ();
 
-      return 1;
+      return true;
     }
   else
     {
       attr = dwarf2_attr (die, DW_AT_data_bit_offset, cu);
       if (attr != nullptr)
 	{
-	  *offset = attr->constant_value (0);
-	  return 1;
+	  *bit_offset = attr->constant_value (0);
+	  return true;
 	}
     }
 
-  return 0;
+  return false;
 }
 
 /* Look for DW_AT_data_member_location or DW_AT_data_bit_offset and
@@ -10033,6 +10031,19 @@  handle_member_location (struct die_info *die, struct dwarf2_cu *cu,
     }
 }
 
+/* Return the bit size associated with DIE.  If DW_AT_bit_size is not
+   seen, return 0.  */
+
+static LONGEST
+get_member_bit_size (struct die_info *die, struct dwarf2_cu *cu)
+{
+  /* Get bit size of field (zero if none).  */
+  attribute *attr = dwarf2_attr (die, DW_AT_bit_size, cu);
+  if (attr != nullptr)
+    return attr->constant_value (0);
+  return 0;
+}
+
 /* Add an aggregate field to the field list.  */
 
 static void
@@ -10096,11 +10107,7 @@  dwarf2_add_field (struct field_info *fip, struct die_info *die,
       fp->set_loc_bitpos (0);
 
       /* Get bit size of field (zero if none).  */
-      attr = dwarf2_attr (die, DW_AT_bit_size, cu);
-      if (attr != nullptr)
-	fp->set_bitsize (attr->constant_value (0));
-      else
-	fp->set_bitsize (0);
+      fp->set_bitsize (get_member_bit_size (die, cu));
 
       /* Get bit offset of field.  */
       handle_member_location (die, cu, fp);
@@ -13944,15 +13951,17 @@  attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
 	  case DW_AT_data_member_location:
 	  case DW_AT_data_bit_offset:
 	    {
-	      LONGEST offset;
+	      LONGEST bit_offset;
 
-	      if (!handle_member_location (target_die, target_cu, &offset))
+	      if (!handle_member_location (target_die, target_cu, &bit_offset))
 		return 0;
 
 	      baton = XOBNEW (obstack, struct dwarf2_property_baton);
 	      baton->property_type = read_type_die (target_die->parent,
 						      target_cu);
-	      baton->offset_info.offset = offset;
+	      baton->offset_info.bit_offset = bit_offset;
+	      baton->offset_info.bit_size = get_member_bit_size (target_die,
+								 target_cu);
 	      baton->offset_info.type = die_type (target_die, target_cu);
 	      prop->set_addr_offset (baton);
 	      break;
diff --git a/gdb/testsuite/gdb.ada/packed_record_2.exp b/gdb/testsuite/gdb.ada/packed_record_2.exp
new file mode 100644
index 00000000000..2e4aea097f0
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/packed_record_2.exp
@@ -0,0 +1,47 @@ 
+# Copyright 2025 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/>.
+
+load_lib "ada.exp"
+
+require allow_ada_tests
+
+standard_ada_testfile exam
+
+set flags {debug}
+if {[ada_minimal_encodings]} {
+    lappend flags additional_flags=-fgnat-encodings=minimal
+}
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable $flags] != ""} {
+    return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/exam.adb]
+runto "exam.adb:$bp_location"
+
+gdb_test "print spr" \
+    " = \\(discr => 3, field => -4, array_field => \\(-5, -6, -7\\)\\)"
+
+gdb_test "print spr.discr" " = 3"
+
+gdb_test "ptype spr" \
+    [multi_line \
+	 "type = record" \
+	 "    discr: range 1 .. 8;" \
+	 "    field: range -7 .. -4;" \
+	 "    array_field: array \\(1 .. 3\\) of exam.small <packed: 2-bit elements>;" \
+	 "end record"]
diff --git a/gdb/testsuite/gdb.ada/packed_record_2/exam.adb b/gdb/testsuite/gdb.ada/packed_record_2/exam.adb
new file mode 100644
index 00000000000..04361f635da
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/packed_record_2/exam.adb
@@ -0,0 +1,38 @@ 
+--  Copyright 2025 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/>.
+
+procedure Exam is
+   type Small is range -7 .. -4;
+   for Small'Size use 2;
+
+   type Range_Int is range 1 .. 8;
+   for Range_Int'Size use 3;
+
+   type Packed_Array is array (Range_Int range <>) of Small;
+   pragma pack (Packed_Array);
+
+   type Some_Packed_Record (Discr : Range_Int := 3) is record
+      Field: Small;
+      Array_Field : Packed_Array (1 .. Discr);
+   end record;
+   pragma Pack (Some_Packed_Record);
+
+   SPR : Some_Packed_Record := (Discr => 3,
+                                Field => -4,
+                                Array_Field => (-5, -6, -7));
+
+begin
+   null;                        --  STOP
+end Exam;
diff --git a/gdb/value.c b/gdb/value.c
index 0f1be2e5d12..31aaa788742 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3266,6 +3266,9 @@  unpack_bits_as_long (struct type *field_type, const gdb_byte *valaddr,
 	}
     }
 
+  if (field_type->code () == TYPE_CODE_RANGE)
+    val += field_type->bounds ()->bias;
+
   return val;
 }