Re: review request: implementing DW_AT_endianity

[Simon Marchi <simark at simark dot ca>]

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

Ok, I see this is binutils code, and I hadn't see that the surrounding code did like that.  You
can leave it as is, and the binutils people will tell you if that's not ok.

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

I suggest naming this function type_byte_order.  Functions named "gdbarch_*" are usually
those part of the gdbarch interface (defined in gdbarch.sh/.h/.c).

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

Nice.  Assginment of fields by GDB would be a good thing to check in the test.

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

Ah indeed.  Do you report the gcc bugs you find to them?

> 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.
Great, thanks,

Simon