[v3,1/3] gdb/testsuite: fix XPASSes when testing with clang

  Clang used to not add access specifiers for typedefs, and the tests
gdb.cp/ptype-flags.exp and gdb.cp/classes.exp both set XFAILs if clang
is being used to test. This was fixed on the clang 16.0.0 release,
generating many XPASSes. This current commit checks the clang version
used for testing, and only emits XFAIL on when a bad clang version is
used.

The testsuite didn't have a good way to check if an arbitrary compiler
is newer than a given release, so this commit also adds a TCL proc to do
just that. The new proc must receive the desired version as a list, then
optionally the compiler and language.
---
 gdb/testsuite/gdb.cp/classes.exp     | 16 +++++++++-------
 gdb/testsuite/gdb.cp/ptype-flags.exp | 11 +++++------
 gdb/testsuite/lib/gdb.exp            | 21 +++++++++++++++++++++
 3 files changed, 35 insertions(+), 13 deletions(-)
  

Hi!

On 10/17/24 10:56 AM, Guinevere Larsen wrote:

> The testsuite didn't have a good way to check if an arbitrary compiler
> is newer than a given release, so this commit also adds a TCL proc to do
> just that. The new proc must receive the desired version as a list, then
> optionally the compiler and language.

I only have one trivial nit -- feel free to ignore.

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index f0a89394b87..0335956c64e 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -5392,6 +5392,27 @@ proc gcc_major_version { {compiler "gcc-*"} {language "c"} } {
>       return $major.$minor
>   }
>   
> +# Calculate if the compiler version is higer than VERSION.
> +# If the proc can't tell or the provided compiler is different
> +# to what is being used to run the tests, it returns false.
> +# VERSION must be a list like "$major $minor".
> +
> +proc compiler_version_lower_than { version {compiler "gcc"} \
> +				      {language "c"} } {

I would (very slightly) prefer that this be kept generic/parallel to
version_compare by naming the proc a bit different and passing in the
operator to pass to version_compare. [To be clear, I do like this as a
convenience function.]

proc compiler_version_compare {version {compiler "gcc"} \
                                {language "c"} op} {
   # ...

In any case, I've retested on RHEL7 w/DTS12, and that is good. Thank
you for addressing that.

And for lack of sufficient caffeine this morning to come up with
anything more clever to say: LGTM (whether you choose to change
anything or not).

Reviewed-by: Keith Seitz <keiths@redhat.com>

Keith

> +    global decimal
> +
> +    set res [regexp $compiler-($decimal)-($decimal)-($decimal) \
> +		[test_compiler_info "" $language] dummy major minor point]
> +
> +    # We either can't determine the compiler, or the compiler is
> +    # different than provided
> +    if { $res != 1} {
> +	return false
> +    }
> +
> +    return [version_compare [list $major $minor $point] "<" $version]
> +}
> +
>   proc current_target_name { } {
>       global target_info
>       if [info exists target_info(target,name)] {
  

On 10/21/24 1:03 PM, Keith Seitz wrote:
> Hi!
>
> On 10/17/24 10:56 AM, Guinevere Larsen wrote:
>
>> The testsuite didn't have a good way to check if an arbitrary compiler
>> is newer than a given release, so this commit also adds a TCL proc to do
>> just that. The new proc must receive the desired version as a list, then
>> optionally the compiler and language.
>
> I only have one trivial nit -- feel free to ignore.
>
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index f0a89394b87..0335956c64e 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -5392,6 +5392,27 @@ proc gcc_major_version { {compiler "gcc-*"} 
>> {language "c"} } {
>>       return $major.$minor
>>   }
>>   +# Calculate if the compiler version is higer than VERSION.
>> +# If the proc can't tell or the provided compiler is different
>> +# to what is being used to run the tests, it returns false.
>> +# VERSION must be a list like "$major $minor".
>> +
>> +proc compiler_version_lower_than { version {compiler "gcc"} \
>> +                      {language "c"} } {
>
> I would (very slightly) prefer that this be kept generic/parallel to
> version_compare by naming the proc a bit different and passing in the
> operator to pass to version_compare. [To be clear, I do like this as a
> convenience function.]
>
> proc compiler_version_compare {version {compiler "gcc"} \
>                                {language "c"} op} {
>   # ...
>
Does TCL play nice with optional arguments before mandatory arguments?

I would ideally want to have "op" in the middle of the arguments, like 
version_compare does, so to keep things readable, I'd like it to be

proc compiler_version_compare { {compiler "gcc"} {language "c"} op version }


Or I could make everything mandatory now that i think about it. Do you 
have any preferences? IMO having version first means the arguments are 
passed in the reverse order to how I naturally read the proc's name.
  

On 10/23/24 1:12 PM, Guinevere Larsen wrote:
> On 10/21/24 1:03 PM, Keith Seitz wrote:
>>
>> I only have one trivial nit -- feel free to ignore.

This still applies. :-)

>>
>>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>>> index f0a89394b87..0335956c64e 100644
>>> --- a/gdb/testsuite/lib/gdb.exp
>>> +++ b/gdb/testsuite/lib/gdb.exp
>>> @@ -5392,6 +5392,27 @@ proc gcc_major_version { {compiler "gcc-*"} 
>>> {language "c"} } {
>>>       return $major.$minor
>>>   }
>>>   +# Calculate if the compiler version is higer than VERSION.
>>> +# If the proc can't tell or the provided compiler is different
>>> +# to what is being used to run the tests, it returns false.
>>> +# VERSION must be a list like "$major $minor".
>>> +
>>> +proc compiler_version_lower_than { version {compiler "gcc"} \
>>> +                      {language "c"} } {
>>
>> I would (very slightly) prefer that this be kept generic/parallel to
>> version_compare by naming the proc a bit different and passing in the
>> operator to pass to version_compare. [To be clear, I do like this as a
>> convenience function.]
>>
>> proc compiler_version_compare {version {compiler "gcc"} \
>>                                {language "c"} op} {
>>   # ...
>>
> Does TCL play nice with optional arguments before mandatory arguments?

No, that's not a thing, but you can use parse_args to add a lot more
flexibility. See documentation in testsuite/lib/gdb.exp ahead of
proc parse_list for an example. It won't get you to your desired
place, but it does offer options.

> I would ideally want to have "op" in the middle of the arguments, like 
> version_compare does, so to keep things readable, I'd like it to be
> 
> proc compiler_version_compare { {compiler "gcc"} {language "c"} op 
> version }

<rhetorical>Is there any language that would support this syntax?
</rhetorical>

> Or I could make everything mandatory now that i think about it. Do you 
> have any preferences? IMO having version first means the arguments are 
> passed in the reverse order to how I naturally read the proc's name.
> 

I don't mind all arguments being mandatory. Again, if this is too
distasteful, then just ignore it. I don't think this warrants
the time we've already spent discussing the possible semantics. If
needed, we can certainly expand this later.

Keith
  

On 10/25/24 12:22 PM, Keith Seitz wrote:
> On 10/23/24 1:12 PM, Guinevere Larsen wrote:
>> On 10/21/24 1:03 PM, Keith Seitz wrote:
>>>
>>> I only have one trivial nit -- feel free to ignore.
>
> This still applies. :-)
>
>>>
>>>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>>>> index f0a89394b87..0335956c64e 100644
>>>> --- a/gdb/testsuite/lib/gdb.exp
>>>> +++ b/gdb/testsuite/lib/gdb.exp
>>>> @@ -5392,6 +5392,27 @@ proc gcc_major_version { {compiler "gcc-*"} 
>>>> {language "c"} } {
>>>>       return $major.$minor
>>>>   }
>>>>   +# Calculate if the compiler version is higer than VERSION.
>>>> +# If the proc can't tell or the provided compiler is different
>>>> +# to what is being used to run the tests, it returns false.
>>>> +# VERSION must be a list like "$major $minor".
>>>> +
>>>> +proc compiler_version_lower_than { version {compiler "gcc"} \
>>>> +                      {language "c"} } {
>>>
>>> I would (very slightly) prefer that this be kept generic/parallel to
>>> version_compare by naming the proc a bit different and passing in the
>>> operator to pass to version_compare. [To be clear, I do like this as a
>>> convenience function.]
>>>
>>> proc compiler_version_compare {version {compiler "gcc"} \
>>>                                {language "c"} op} {
>>>   # ...
>>>
>> Does TCL play nice with optional arguments before mandatory arguments?
>
> No, that's not a thing, but you can use parse_args to add a lot more
> flexibility. See documentation in testsuite/lib/gdb.exp ahead of
> proc parse_list for an example. It won't get you to your desired
> place, but it does offer options.
yeah, parse_args is definitely over complicating this IMO.
>
>> I would ideally want to have "op" in the middle of the arguments, 
>> like version_compare does, so to keep things readable, I'd like it to be
>>
>> proc compiler_version_compare { {compiler "gcc"} {language "c"} op 
>> version }
>
> <rhetorical>Is there any language that would support this syntax?
> </rhetorical>

Yeah, I don't know of any, but TCL has surprised me before...

In fact, I tested the order: {version {compiler "gcc"} {language "c"} 
op} and it works with the existing tests (which define compiler and 
language). which is what prompted the original question...

>
>> Or I could make everything mandatory now that i think about it. Do 
>> you have any preferences? IMO having version first means the 
>> arguments are passed in the reverse order to how I naturally read the 
>> proc's name.
>>
>
> I don't mind all arguments being mandatory. Again, if this is too
> distasteful, then just ignore it. I don't think this warrants
> the time we've already spent discussing the possible semantics. If
> needed, we can certainly expand this later.
>
> Keith
>
I think I'll leave things as is, then, but if anyone else reviewing this 
shares your opinion, I'll make the change.