Re: [pushed] Support -prompt and -lbl in gdb_test (Re: [PATCH 5/5] Make gdb_test's question non-optional if specified)

[Pedro Alves <pedro@palves.net>]

On 2022-05-18 13:36, Pedro Alves wrote:
> On 2022-05-18 13:15, Tom de Vries wrote:
>> On 5/18/22 13:01, Pedro Alves wrote:
>>> -    if [llength $args]>2 then {
>>> -    set message [lindex $args 2]
>>> -    } else {
>>> -    set message [lindex $args 0]
>>> +    if { $message == "" } {
>>> +    set message $command
>>>       }
>>
>> This seems to cause:
>> ...
>> PATH: gdb.ada/exec_changed.exp: shell mv /home/vries/gdb_versions/devel/build/gdb/testsuite/outputs/gdb.ada/exec_changed/first /home/vries/gdb_versions/devel/build/gdb/testsuite/outputs/gdb.ada/exec_changed/common
>> PATH: gdb.ada/exec_changed.exp: shell mv /home/vries/gdb_versions/devel/build/gdb/testsuite/outputs/gdb.ada/exec_changed/common /home/vries/gdb_versions/devel/build/gdb/testsuite/outputs/gdb.ada/exec_changed/first
>> PATH: gdb.ada/exec_changed.exp: shell mv /home/vries/gdb_versions/devel/build/gdb/testsuite/outputs/gdb.ada/exec_changed/second /home/vries/gdb_versions/devel/build/gdb/testsuite/outputs/gdb.ada/exec_changed/common
>> PATH: gdb.ada/exec_changed.exp: shell touch /home/vries/gdb_versions/devel/build/gdb/testsuite/outputs/gdb.ada/exec_changed/common
>> PATH: gdb.ada/exec_changed.exp: shell touch /home/vries/gdb_versions/devel/build/gdb/testsuite/outputs/gdb.ada/exec_changed/first
>> ...
>> because the interpretation of message "" changed:
>> ...
>> gdb_test "shell mv ${binfile} ${common_binfile}" ".*" ""
>> ...
> 
> Hmm...  Yeah...  We could revert most of the changes to gdb_test and just keep the parse_args
> part.  However, IMO the old behavior is a misfeature, though.  I think tests should
> always have a name.  E.g., if such a test hits an internal error, what message would
> be used?
> 
> The documentation of the option even says that if message is omitted, use the command
> string as message:
> 
>  # MESSAGE is an optional message to be printed.  If this is
>  #   omitted, then the pass/fail messages use the command string as the
>  #   message.  (If this is the empty string, then sometimes we don't
>  #   call pass or fail at all; I don't understand this at all.)
> 
> Also, gdb_test_multiple doesn't a distinction between explicit "" message and
> not specified message, the only way to end up with an empty message is if command
> is empty as well.  So AFAICS, this change (inadvertently) made gdb_test and
> gdb_test_multiple behave the same in this respect.
> 
> So how about we just fix the affected gdb_test invocations?

So I'm diffing testruns from before the patch vs after, and I think the vast
majority of cases that weren't issuing a pass should do so.  With the new
messages, we now get a ton of DUPLICATEs, which I'm fixing.

However, there are a few spots here and there where we really would prefer to
not issue a pass, such as when we're testing something in a loop, and we don't
know how many iterations there will be.

Instead of going back to how it used to be, I'm thinking of adding a new
option to gdb_test, "gdb_test -nopass".  The advantage of this approach is
that we always have a message for the FAIL case this way, and, it's more
explicit.  We could fix the "no message for the FAIL case" in the old implementation
by delaying defaulting message to the command until after the pass case is added
to user_code.  But the explict option still seems better to me, as it let's you
specify a message different from the command, and only print it on FAIL.  The other
approach would force the message to be the same as the command.