On 12/14/20 5:55 PM, Simon Marchi wrote:
> 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?

How about this?

Thanks,
- Tom