This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA 2/6] Handle alignof and _Alignof


Hi Tom,

What's in the patch looks good to me.  I have comments on the
tests -- I think it'd be good to extend them a bit more.

On 04/24/2018 04:22 PM, Tom Tromey wrote:

> +
> +# The types we're going to test.
> +
> +set typelist {
> +    char {unsigned char}
> +    short {unsigned short}
> +    int {unsigned int}
> +    long {unsigned long}
> +    {long long} {unsigned long long}
> +    float
> +    double

Shouldn't we test "long double"?  Patch #1 handles it.
Not sure all GCC ports support it, may require separate compilation.

Also, I'm wondering about "__int128" if the target
supports it.

In C++, do we get the alignment of non-standard layout classes right?
E.g., structs with references.  And structs with virtual methods, like:

 struct S
 {
   virtual ~S ();
   char c;
 };

This should print 8 instead of 1 on x86-64, due to the vtable pointer.

I think it'd be good to cover those things in the tests too.

Likewise arrays, bitfields and typedefs?

I didn't spot any test for the
 "could not determine alignment of type"
case to make that that works gracefully without crashing.  

What do we do with _Alignof(void)?  We treat sizeof(void) == 1,
like gcc, so I assume the _Alignof will return 1 too instead
of erroring out.

Finally, for completeness, GCC allows _Alignof applied to
expressions, so I guess we should to.  Does the series allow that?
I.e., can we do _Alignof(1 + 1)?  Does the parser handle that?

Shouldn't we test _Alignof applied to the structure fields too?

There was a snippet in the patch that made me wonder if the patch
handles alignof of a no-debug-info variable and and the return-type
of a no-debug-info function correctly (instead of e.g., crashing).
I'd be nice to add a couples test to gdb.base/nodebug.exp
to make sure.  E.g.:
   p _Alignof (dataglobal64_1)
   p _Alignof (middle())"

Also, please add intro comments to the testcase .exp files, so
that later on people can find out what the testcase is
about easily.

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]