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

[Simon Marchi <simark@simark.ca>]

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.

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