Re: [PATCH][gdb/testsuite] Fix gdb.base/endianity.exp with gcc-4.8

[Simon Marchi <simark@simark.ca>]

On 2020-12-13 11:23 a.m., Tom de Vries wrote:
> On 12/13/20 2:56 PM, Joel Brobecker wrote:
>> Hi Tom,
>>
>>> When running test-case gdb.base/endianity.exp using gcc-4.8, we get:
>>> ...
>>> (gdb) x/x &o.v^M
>>> 0x7fffffffd120: 0x00000004^M
>>> (gdb) XFAIL: gdb.base/endianity.exp: x/x &o.v
>>> x/xh &o.w^M
>>> 0x7fffffffd124: 0x0003^M
>>> (gdb) FAIL: gdb.base/endianity.exp: x/xh &o.w
>>> ...
>>>
>>> The gcc 4.8 compiler does not support the scalar_storage_order attribute, so
>>> the testcase is compiled without that attribute, and the expected results are
>>> different.
>>>
>>> This is why there's the first XFAIL, and we could xfail the second FAIL for the
>>> same reason.
>>>
>>> Instead, fix this by adapting the expected values based on whether the attribute
>>> has been used in endianity.c.
>>>
>>> Also, remove hard-coding of the byte order in the expected memory printing.
>>>
>>> Tested on x86_64-linux, with gcc-4.8, gcc-7, and clang-10.
>>>
>>>
>>> Any comments?
>>
>> For me, the whole point of this testcase is to test SSO, so if
>> the compiler doesn't support it, the testcase loses its value
>> entirely (to my eyes anyway). As a result of this, I dont' think
>> bringing the extra complexity that you are suggesting is bringing
>> any value -- I might argue that it's now hard to read the testcase
>> an understand what we're trying to do (sorry!).
>>
>
> Np, that's also good feedback, thanks.
>
>> In my opinion, rather than an XFAIL, we should just only do
>> the second half of the testcase if the compiler supports it,
>> than xfailing the tests. So I would do:
>>
>> if { ([test_compiler_info {gcc-[0-5]-*}] || ![test_compiler_info gcc*]) } {
>>     # The rest of the testcase requires Scalar Storage Order support.
>>     # This compiler does not support it, so skip the rest.
>>     return
>> }
>>
>
> Ack, committed as below.
>
> Thanks,
> - Tom
>

Just a small nit, it would be nice to have an "unsupported" call in the
if to record in the sum file that part of this file was skipped and why.

Also, I don't like the (pre-existing) check:

  if { ([test_compiler_info {gcc-[0-5]-*}] || ![test_compiler_info gcc*]) } {

That makes it so testing with any other compiler than gcc will result in
this part being skipped.  What if clang gains support for this feature?

Would it make sense to have a small
"supports_scalar_storage_order_attribute" util proc in lib/gdb.exp?

Simon