Re: review request: implementing DW_AT_endianity

[Simon Marchi <simark at simark dot ca>]

On 2017-10-06 11:05 AM, Peeter Joot wrote:
> 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.

Hi Peeter,

Thanks for the patch.  Since the compiler supports it, it makes sense for the debugger
to support it correctly.

Please read the Contribution Checklist:

  https://sourceware.org/gdb/wiki/ContributionChecklist

Most importantly, make sure your code uses the GNU style, and use "git send-email" to
send your patch.  It will make it easier for reviewers to apply and look at your patch.

Changes to binutils should be sent the binutils mailing list (binutils@sourceware.org), so
make a separate patch for binutils/dwarf.c and send it there.

The best way to show that your contribution works is to add a test for it.  You can add it
to testsuite/gdb.base.  Copy an existing one (e.g. wchar.c/wchar.exp) and modify as needed.

You should also run the testsuite to see if your patch causes any regression:

  https://sourceware.org/gdb/wiki/TestingGDB

Note that there are many existing failures in the testsuite, so what you should do is run the
testsuite without and with your patch, and diff the before/after testsuite/gdb.sum file.

I just noted a few formatting comments below, I'll look at the code more in depth once you
send an updated version that's easier to apply.

> 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;

Put each statement on its own line:

case DW_END_default:
  printf ("(default)");
  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) )

Don't put spaces inside parentheses.

> +  {
> +    byte_order = BFD_ENDIAN_LITTLE;
> +  }

Don't use curly braces for a single statement.

> +
>    /* 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.

I would consider a change like that to be significant enough to require an assignment.

Thanks!

Simon