This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
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