review request: implementing DW_AT_endianity

[Peeter Joot <peeter.joot@lzlabs.com>]

I've implemented support in gdb for DW_AT_endianity, DW_END_bigendian, and DW_END_littleendian, and would like to ask for gdb community review of this change.


Purpose: gcc6+ supports mixed endian attributes on structures and unions, and flags these dwarf instrumentation of these structure members with DW_END_... attributes.  As intel appears to have introduced these dwarf flags into the standard, I expect their compiler and debugger also supports them.  However, gdb does not.  The following example, compiled on a little endian system, is an example of this attribute use:


#include <stdio.h>

#include <string.h>


struct big {

    int v;

    short a[4];

} __attribute__( ( scalar_storage_order( "big-endian" ) ) );


struct little {

    int v;

    short a[4];

} __attribute__( ( scalar_storage_order( "little-endian" ) ) );


struct native {

    int v;

    short a[4];

};


int main() {

    struct big b = {3, {1, 2, 3, 4}};

    struct native n = {3, {1, 2, 3, 4}};

    struct little l = {3, {1, 2, 3, 4}};

    int cb = memcmp( &b, &n, sizeof( b ) );

    int cl = memcmp( &l, &n, sizeof( l ) );


    printf( "%d %d %d: big %s native.  little %s native\n", b.v, n.v, l.v,

            cb == 0 ? "==" : "!=", cl == 0 ? "==" : "!=" );

    return 0;

}



Running this produces the expected result, and the endianness of the underlying stores can be seen in a debugger session:


Breakpoint 1, main () at test.c:20

20          struct big b = {3, {1, 2, 3, 4}};

(gdb) n

21          struct native n = {3, {1, 2, 3, 4}};

(gdb) n

22          struct little l = {3, {1, 2, 3, 4}};

(gdb) n

23          int cb = memcmp( &b, &n, sizeof( b ) );

(gdb) p b

$1 = {v = 50331648, a = {256, 512, 768, 1024}}

(gdb) p n

$2 = {v = 3, a = {1, 2, 3, 4}}

(gdb) p l

$3 = {v = 3, a = {1, 2, 3, 4}}

(gdb) x/4x &b

0x7fffffffd96c: 0x03000000      0x02000100      0x04000300      0x00000000

(gdb) x/4x &l

0x7fffffffd954: 0x00000003      0x00020001      0x00040003      0x00000003

(gdb) x/4x &n

0x7fffffffd960: 0x00000003      0x00020001      0x00040003      0x03000000

(gdb) n

24          int cl = memcmp( &l, &n, sizeof( l ) );

(gdb)

26          printf( "%d %d %d: big %s native.  little %s native\n", b.v, n.v, l.v,

(gdb) n

3 3 3: big != native.  little == native

28          return 0;



This debugger session also shows that gdb currently ignores the DW_END_bigendian (and littleendian) attributes, showing the internal representation of the data instead of what the values that memory represents.  It turns out that propagating the DW_AT_endianity dwarf attributes to the gdb print_scalar_formatted function is fairly straightforward, which allows gdb to display the results in a user-friendly way regardless of the endian attributes:


diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index 91f95ff..e79fd5e 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -2283,6 +2283,17 @@ read_and_display_attr_value (unsigned long attribute,
  }
       break;

+    case DW_AT_endianity:
+      printf ("\t");
+      switch (uvalue)
+ {
+ case DW_END_default: printf ("(default)"); break;
+ case DW_END_big: printf ("(big)"); break;
+ case DW_END_little: printf ("(little)"); break;
+ default: printf (_("(unknown endianity)")); break;
+ }
+      break;
+
     case DW_AT_virtuality:
       printf ("\t");
       switch (uvalue)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 18224e0..66a8eaf 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@
+2017-10-06  Peeter Joot  <peeter.joot@lzlabs.com>
+
+ * gdb/gdbtypes.h (global scope): define TYPE_ENDIANITY_BIG,
+ TYPE_ENDIANITY_LITTLE.
+ * binutils/dwarf.c (read_and_display_attr_value): Handle
+ DW_AT_endianity, DW_END_default, DW_END_big, DW_END_little
+ * gdb/dwarf2read.c (read_base_type): Handle DW_END_big, DW_END_little
+ * gdb/gdbtypes.c (check_types_equal): Require matching
+ TYPE_ENDIANITY_BIG, and TYPE_ENDIANITY_LITTLE if set.
+ (recursive_dump_type): Print TYPE_ENDIANITY_BIG, and
+ TYPE_ENDIANITY_LITTLE if set.
+ (struct main_type): Add flag_endianity_big, flag_endianity_little
+ * gdb/printcmd.c (print_scalar_formatted): Use compiler supplied
+ endianness instead of arch endianness if TYPE_ENDIANITY_BIG or
+ TYPE_ENDIANITY_LITTLE is set.
+
 2017-10-06  Yao Qi  <yao.qi@linaro.org>

  * Makefile.in (ALL_64_TARGET_OBS): Replace aarch64-insn.o with
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 1b15adc..fa2889b 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -15234,6 +15234,7 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
   struct type *type;
   struct attribute *attr;
   int encoding = 0, bits = 0;
+  int endianity = 0;
   const char *name;

   attr = dwarf2_attr (die, DW_AT_encoding, cu);
@@ -15330,6 +15331,21 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
   if (name && strcmp (name, "char") == 0)
     TYPE_NOSIGN (type) = 1;

+  attr = dwarf2_attr (die, DW_AT_endianity, cu);
+  if ( attr )
+    {
+      endianity = DW_UNSND (attr);
+      switch (endianity)
+        {
+           case DW_END_big:
+              TYPE_ENDIANITY_BIG (type) = 1;
+              break;
+           case DW_END_little:
+              TYPE_ENDIANITY_LITTLE (type) = 1;
+              break;
+        }
+    }
+
   return set_die_type (die, type, cu);
 }

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 73d4453..43f553b 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -3423,6 +3423,8 @@ check_types_equal (struct type *type1, struct type *type2,
       || TYPE_LENGTH (type1) != TYPE_LENGTH (type2)
       || TYPE_UNSIGNED (type1) != TYPE_UNSIGNED (type2)
       || TYPE_NOSIGN (type1) != TYPE_NOSIGN (type2)
+      || TYPE_ENDIANITY_BIG (type1) != TYPE_ENDIANITY_BIG (type2)
+      || TYPE_ENDIANITY_LITTLE (type1) != TYPE_ENDIANITY_LITTLE (type2)
       || TYPE_VARARGS (type1) != TYPE_VARARGS (type2)
       || TYPE_VECTOR (type1) != TYPE_VECTOR (type2)
       || TYPE_NOTTEXT (type1) != TYPE_NOTTEXT (type2)
@@ -4460,6 +4462,14 @@ recursive_dump_type (struct type *type, int spaces)
     {
       puts_filtered (" TYPE_NOSIGN");
     }
+  if (TYPE_ENDIANITY_BIG (type))
+    {
+      puts_filtered (" TYPE_ENDIANITY_BIG");
+    }
+  if (TYPE_ENDIANITY_LITTLE (type))
+    {
+      puts_filtered (" TYPE_ENDIANITY_LITTLE");
+    }
   if (TYPE_STUB (type))
     {
       puts_filtered (" TYPE_STUB");
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 009cea9..074aa2c 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -210,6 +210,16 @@ DEF_ENUM_FLAGS_TYPE (enum type_instance_flag_value, type_instance_flags);

 #define TYPE_NOSIGN(t) (TYPE_MAIN_TYPE (t)->flag_nosign)

+/* * Mixed endian archetectures can supply dwarf instrumentation
+ * that indicates the desired endian interpretation of the variable.
+ * This indicates that the interpretation should be big-endian
+ * even if the cpu is running in little endian mode. */
+#define TYPE_ENDIANITY_BIG(t) (TYPE_MAIN_TYPE (t)->flag_endianity_big)
+
+/* * The type has a little endian interpretation even if the cpu
+ * is running in big endian mode. */
+#define TYPE_ENDIANITY_LITTLE(t) (TYPE_MAIN_TYPE (t)->flag_endianity_little)
+
 /* * This appears in a type's flags word if it is a stub type (e.g.,
    if someone referenced a type that wasn't defined in a source file
    via (struct sir_not_appearing_in_this_film *)).  */
@@ -616,6 +626,8 @@ struct main_type
   unsigned int flag_gnu_ifunc : 1;
   unsigned int flag_fixed_instance : 1;
   unsigned int flag_objfile_owned : 1;
+  unsigned int flag_endianity_big : 1;
+  unsigned int flag_endianity_little : 1;

   /* * True if this type was declared with "class" rather than
      "struct".  */
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index a8743f1..e714283 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -356,6 +356,15 @@ print_scalar_formatted (const gdb_byte *valaddr, struct type *type,
   unsigned int len = TYPE_LENGTH (type);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

+  if ( TYPE_ENDIANITY_BIG (type) )
+  {
+    byte_order = BFD_ENDIAN_BIG;
+  }
+  if ( TYPE_ENDIANITY_LITTLE (type) )
+  {
+    byte_order = BFD_ENDIAN_LITTLE;
+  }
+
   /* String printing should go through val_print_scalar_formatted.  */
   gdb_assert (options->format != 's');



On behalf of my employer (LzLabs), I would like to contribute this change to to the gdb project.  For a small change like this, it is not clear FSF copyright assignment is required, as I see the following in your CONTRIBUTIONS document: "Small changes can be accepted without a copyright assignment form on file.".  What counts as small?


If assignment is required, I have approval from company management and legal to formally go through that process.  Before figuring out how that assignment process works, I wanted to first ensure the changes I've made would be acceptable for contribution, and make any review driven updates required.


--

Peeter