diff mbox series

Handle DW_AT_endianity on enumeration types

Message ID 20240112141341.2575981-1-tromey@adacore.com
State New
Headers
Series Handle DW_AT_endianity on enumeration types |

Checks

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

Commit Message

Tom Tromey Jan. 12, 2024, 2:13 p.m. UTC 
  A user found that gdb would not correctly print a field from an Ada
record using the scalar storage order feature.  We tracked this down
to a combination of problems.

First, GCC did not emit DW_AT_endianity on the enumeration type.
DWARF does not specify this, but it is an obvious and harmless
extension.  This was fixed in GCC recently:

    https://gcc.gnu.org/pipermail/gcc-patches/2024-January/642347.html
    https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=5d8b60effc7268448a94fbbbad923ab6871252cd

Second, GDB did not handle this attribute on enumeration types.  This
patch makes this change and adds a test case that will pass with the
patched GCC.

So far, the GCC patch isn't on the gcc-13 branch; but if it ever goes
in, the test case in this patch can be updated to reflect that.
---
 gdb/dwarf2/read.c                             | 66 ++++++++++++-------
 gdb/testsuite/gdb.ada/scalar_storage.exp      |  9 ++-
 .../gdb.ada/scalar_storage/storage.adb        |  9 ++-
 3 files changed, 57 insertions(+), 27 deletions(-)

Comments

Keith Seitz Jan. 23, 2024, 7:36 p.m. UTC | #1 
On 1/12/24 06:13, Tom Tromey wrote:
> A user found that gdb would not correctly print a field from an Ada
> record using the scalar storage order feature.  We tracked this down
> to a combination of problems.
> 
> First, GCC did not emit DW_AT_endianity on the enumeration type.
> DWARF does not specify this, but it is an obvious and harmless
> extension.  This was fixed in GCC recently:
> 
>      https://gcc.gnu.org/pipermail/gcc-patches/2024-January/642347.html
>      https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=5d8b60effc7268448a94fbbbad923ab6871252cd
> 
> Second, GDB did not handle this attribute on enumeration types.  This
> patch makes this change and adds a test case that will pass with the
> patched GCC.
> 
> So far, the GCC patch isn't on the gcc-13 branch; but if it ever goes
> in, the test case in this patch can be updated to reflect that.

I appreciate the KFAIL here to keep the test suite from throwing
transient FAILS awaiting compiler updates.

This all looks pretty straight-forward.

Feel free to add my Reviewed-by:.

Keith
Tom Tromey Jan. 24, 2024, 2:38 p.m. UTC | #2 
>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> This all looks pretty straight-forward.

Keith> Feel free to add my Reviewed-by:.

Thank you.  I'm going to check this in.

Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 76dbeb8d536..4bcd95925fb 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -13170,6 +13170,44 @@  process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
     }
 }
 
+/* Read DW_AT_endianity from DIE and compute the byte order that
+   should be used.  The CU's arch is used as the default.  The result
+   is true if the returned arch differs from the default, and false if
+   they are the same.  If provided, the out parameter BYTE_ORDER is
+   also set.  */
+
+static bool
+die_byte_order (die_info *die, dwarf2_cu *cu, enum bfd_endian *byte_order)
+{
+  gdbarch *arch = cu->per_objfile->objfile->arch ();
+  enum bfd_endian arch_order = gdbarch_byte_order (arch);
+  enum bfd_endian new_order = arch_order;
+
+  attribute *attr = dwarf2_attr (die, DW_AT_endianity, cu);
+  if (attr != nullptr && attr->form_is_constant ())
+    {
+      int endianity = attr->constant_value (0);
+
+      switch (endianity)
+	{
+	case DW_END_big:
+	  new_order = BFD_ENDIAN_BIG;
+	  break;
+	case DW_END_little:
+	  new_order = BFD_ENDIAN_LITTLE;
+	  break;
+	default:
+	  complaint (_("DW_AT_endianity has unrecognized value %d"), endianity);
+	  break;
+	}
+    }
+
+  if (byte_order != nullptr)
+    *byte_order = new_order;
+
+  return new_order != arch_order;
+}
+
 /* Assuming DIE is an enumeration type, and TYPE is its associated
    type, update TYPE using some information only available in DIE's
    children.  In particular, the fields are computed.  */
@@ -13317,6 +13355,8 @@  read_enumeration_type (struct die_info *die, struct dwarf2_cu *cu)
 
   type->set_is_declared_class (dwarf2_flag_true_p (die, DW_AT_enum_class, cu));
 
+  type->set_endianity_is_not_default (die_byte_order (die, cu, nullptr));
+
   set_die_type (die, type, cu);
 
   /* Finish the creation of this type by using the enum's children.
@@ -15220,7 +15260,6 @@  read_base_type (struct die_info *die, struct dwarf2_cu *cu)
   struct attribute *attr;
   int encoding = 0, bits = 0;
   const char *name;
-  gdbarch *arch;
 
   attr = dwarf2_attr (die, DW_AT_encoding, cu);
   if (attr != nullptr && attr->form_is_constant ())
@@ -15232,27 +15271,8 @@  read_base_type (struct die_info *die, struct dwarf2_cu *cu)
   if (!name)
     complaint (_("DW_AT_name missing from DW_TAG_base_type"));
 
-  arch = objfile->arch ();
-  enum bfd_endian byte_order = gdbarch_byte_order (arch);
-
-  attr = dwarf2_attr (die, DW_AT_endianity, cu);
-  if (attr != nullptr && attr->form_is_constant ())
-    {
-      int endianity = attr->constant_value (0);
-
-      switch (endianity)
-	{
-	case DW_END_big:
-	  byte_order = BFD_ENDIAN_BIG;
-	  break;
-	case DW_END_little:
-	  byte_order = BFD_ENDIAN_LITTLE;
-	  break;
-	default:
-	  complaint (_("DW_AT_endianity has unrecognized value %d"), endianity);
-	  break;
-	}
-    }
+  enum bfd_endian byte_order;
+  bool not_default = die_byte_order (die, cu, &byte_order);
 
   if ((encoding == DW_ATE_signed_fixed || encoding == DW_ATE_unsigned_fixed)
       && cu->lang () == language_ada
@@ -15390,7 +15410,7 @@  read_base_type (struct die_info *die, struct dwarf2_cu *cu)
 
   maybe_set_alignment (cu, die, type);
 
-  type->set_endianity_is_not_default (gdbarch_byte_order (arch) != byte_order);
+  type->set_endianity_is_not_default (not_default);
 
   if (TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_INT)
     {
diff --git a/gdb/testsuite/gdb.ada/scalar_storage.exp b/gdb/testsuite/gdb.ada/scalar_storage.exp
index 1c60774a13f..6b83fab709e 100644
--- a/gdb/testsuite/gdb.ada/scalar_storage.exp
+++ b/gdb/testsuite/gdb.ada/scalar_storage.exp
@@ -33,5 +33,10 @@  if {![runto "storage.adb:$bp_location"]} {
   return
 }
 
-gdb_test "print V_LE" "= \\(value => 126, another_value => 12\\)"
-gdb_test "print V_BE" "= \\(value => 126, another_value => 12\\)"
+gdb_test "print V_LE" "= \\(value => 126, another_value => 12, color => green\\)"
+
+# This requires a compiler fix that is in GCC 14.
+if {[gcc_major_version] < 14} {
+    setup_kfail "DW_AT_endianity on enum types" *-*-*
+}
+gdb_test "print V_BE" "= \\(value => 126, another_value => 12, color => green\\)"
diff --git a/gdb/testsuite/gdb.ada/scalar_storage/storage.adb b/gdb/testsuite/gdb.ada/scalar_storage/storage.adb
index 7c93e2eba4b..009d0b070fd 100644
--- a/gdb/testsuite/gdb.ada/scalar_storage/storage.adb
+++ b/gdb/testsuite/gdb.ada/scalar_storage/storage.adb
@@ -20,14 +20,19 @@  procedure Storage is
    subtype Some_Range is Natural range 0..127;
    subtype Another_Range is Natural range 0..15;
 
+   type Colors is (Red, Green, Blue);
+   for Colors use (Red => 0, Green => 1, Blue => 2);
+
    type Rec is record
       Value : Some_Range;
       Another_Value : Another_Range;
+      Color : Colors;
    end record;
    
    for Rec use record
       Value at 0 range 0..6;
       Another_Value at 0 range 7..10;
+      Color at 0 range 11..13;
    end record;
 
    type Rec_LE is new Rec;
@@ -42,8 +47,8 @@  procedure Storage is
    V_BE : Rec_BE;
 
 begin
-   V_LE := (126, 12);
-   V_BE := (126, 12);
+   V_LE := (126, 12, Green);
+   V_BE := (126, 12, Green);
 
    Do_Nothing (V_LE'Address);  --  START
    Do_Nothing (V_BE'Address);