Re: review request: implementing DW_AT_endianity

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

Hi Simon,


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

Thanks for the new-developer tips.


> > --- a/binutils/dwarf.c

> > +++ b/binutils/dwarf.c

...

> Put each statement on its own line:

>

> case DW_END_default:

>   printf ("(default)");

>   break;



Note that if I did this, the addition wouldn't match any of the case statements in the surrounding code (read_and_display_attr_value).  There are case statements matching your suggested style earlier in this function, but others that do not (like all the ones immediately surrounding my addition).  I've temporarily adjusted my addition to match the immediately surrounding code (i.e. using tabs instead of spaces, which I hadn't initially noticed).  Shall I adjust all of that surrounding "non-standard formatting" so that each statement is on its own line, or be consistent locally with the conventions used in this file?


I've fixed up all the other formatting issues that you've pointed out, and will submit a new patch with git send-email after running the test suite, and adding my own new test.  The next patch I send will have an additional mechanical change, altering blocks of code from:


gdbarch_byte_order (get_type_arch (type1))


to:


gdbarch_byte_order_for_type (NULL, type1)


where the following helper function has been added to gdbtypes.c:


enum bfd_endian

gdbarch_byte_order_for_type (struct gdbarch * gdbarch, struct type *type)

{

  if (TYPE_ENDIANITY_BIG (type))

    return BFD_ENDIAN_BIG;

  else if (TYPE_ENDIANITY_LITTLE (type))

    return BFD_ENDIAN_LITTLE;

  if (!gdbarch)

    gdbarch = get_type_arch (type);

  return gdbarch_byte_order (gdbarch);

}


When the caller of this already had a gdbarch variable handy, I've used that, instead of passing NULL.  This transformation is enough to make gdb assignment to a mixed endian field, as in:


(gdb) p b.v = 4


work properly, storing the value in big-endian format.  This is the using the same example from my original review request email, assuming a little endian target and a structure with a big-endian attribute.


In the process of doing this debug testing, I've noticed that gcc appears to have some bugs with respect to its dwarf endianity attribute tagging.  For example:


#include <stdio.h>

#include <string.h>


typedef int int32be_t;

typedef short int16be_t;


struct big {

    int32be_t v;

    int16be_t a[4];

} __attribute__( ( scalar_storage_order( "big-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}};


    printf( "%d\n", b.v );

    printf( "%d\n", n.v );


    return 0;

}


A version of this code that does not use typedefs for the field types does get tagged with DW_AT_endianity/DW_END_big, but when typedefs are used as above, the object code ends up with no endianity attributes at all (although the compiler does insert the desired bswap operations).  It appears that it may take a few iterations of compiler/debugger enhancements to really get this working nicely together.


There is a bigger issue of the DW_AT_name that gets emitted for a field with non-native endianity.  I think it would be best for the compiler to choose a different DW_AT_name than the default (int.be for example).  If that is not done it looks like it breaks gdb's assumption that there is one set of attributes for each type in question.  This could be considered a gdb bug, but I think it would be better for the compiler to cater to gdb in this case.  I'm curious what other developers (especially anybody that works on both gcc and gdb) think about this.


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


Okay.  I've contacted the FSF assignment representative to start this process.


Peeter