Re: [PATCH] gdb.fortran/lbound-ubound.exp: read expected lbound and ubound from function parameters (PR 30414)

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

> On 5/12/23 13:02, Andrew Burgess via Gdb-patches wrote:
>>> @@ -96,6 +68,17 @@ while { $test_count < 500 } {
>>>  	    break
>>>  	}
>>>  
>>> +	set expected_lbound [get_valueof "" "lb" "get expected lbound"]
>> 
>> I don't think this is doing what you expect (given the lines below).
>> The default value is going to be 'get expected lbound', while the
>> testname will be generated by the get_valueof call.  You can just do
>> this:
>> 
>>   set expected_lbound [get_integer_valueof "lb" "" "get expected lbound"]
>
> You're right, I'm missing a parameter to get_valueof.  It should have
> been:
>
> 	set expected_lbound [get_valueof "" "lb" "" "get expected lbound"]
> 	set expected_ubound [get_valueof "" "ub" "" "get expected ubound"]
>
> Note that I don't want to use get_integer_valueof here, because that
> proc specifically expects one integer, whereas I get an array of
> integers:
>
>     print lb^M
>     $1 = (-8, -10)^M
>     (gdb) PASS: gdb.fortran/lbound-ubound.exp: test 0: get expected lbound
>     print ub^M
>     $2 = (-1, -2)^M
>     (gdb) PASS: gdb.fortran/lbound-ubound.exp: test 0: get expected ubound
>
> Luckily, GDB outputs them in the same format as the inferior outputs
> them before my patch.  So the rest of the test doesn't see the
> difference.
>
>>> +	set expected_ubound [get_valueof "" "ub" "get expected ubound"]
>>> +
>>> +	if { $expected_lbound == "" } {
>>> +	    error "failed to extract expected results for lbound"
>>> +	}
>>> +
>>> +	if { $expected_ubound == "" } {
>>> +	    error "failed to extract expected results for ubound"
>>> +	}
>> 
>> I was originally going to suggest dropping these checks -- having the
>> default expected result be the empty string (or maybe use some other
>> invalid string, e.g. '*INVALID*'?) will cause the following tests to
>> fail anyway, so the check doesn't seem to add much.
>> 
>> But I see all you really did was move the checks I already had ... so I
>> guess that's fine.  I don't think I'd write this test in this way today
>> though.
>
> I am fine with dropping the checks.
>
>> I do wonder why the switch from perror to error though?  The original
>> perror would print the error but then continue to try the next test,
>> while error aborts the whole test script.  I guess if one goes wrong
>> then they all go wrong...  I guess it doesn't really matter much.
>
> I'm not a big fan of perror in the first place, I find it's really easy
> to mis-use.  Like here, the test calls perror, but then return.  So
> what is the next test that it intends to mark as unresolved?  Is it
> going to affect the next .exp to be executed?  I feel like something
> like that happened to me before and I chased the source of random
> unresolveds for a long time.
>
> I just re-read the documentation, and noticed:
>
>     If the optional numeric value is 0, then there are no further side
>     effects to calling this function, and the following test outcome
>     doesn’t become UNRESOLVED. This can be used for errors with no known
>     side effects.
>
> So I suppose that calling perror with 0 would be fine here.  Except
> that I just tried it and:
>
>     Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.fortran/lbound-ubound.exp ...
>     ERROR: hello
>     ...
>     # of expected passes            5
>     ...
>     $ echo $?
>     0
>
> That error doesn't leave a trace in the summary and the exit status is
> 0, so it seems very easy to miss.
>
> So, my preference in that case is to use "error", it's impossible to
> miss.  It means we abort the whole test, but that seems fine in this
> situation where something just unrecoverable happened.

Honestly, I wasn't even aware of perror changing the next test to
unresolved.  So, that's my one thing learned for today :)

I'm happy with using error, or no checks and just collecting the FAILs,
whatever you prefer.

Thanks for getting this test fixed up.

Andrew


>
> But again, I'm fine with not having checks at all, it means we would get
> a bunch of FAILs in that case, which is also impossible to miss.
>
> Simon