[PATCH 1/2] [gdb/testsuite] Detect trailing-text-in-parentheses duplicates

[Tom de Vries]

On 7/22/24 17:52, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> When using a duplicate test name:
>> ...
>> fail foo
>> fail foo
>> ...
>> we get:
>> ...
>> FAIL: $exp: foo
>> FAIL: $exp: foo
>> DUPLICATE: $exp: foo
>> ...
>>
>> But when we do:
>> ...
>> fail foo
>> fail "foo (timeout)"
>> ...
>> we get only:
>> ...
>> FAIL: $exp: foo
>> FAIL: $exp: foo (timeout)
>> ...
>>
>> Trailing text between parentheses prefixed with a space is interpreted as
>> extra information, and not as part of the test name [1].
>>
>> Consequently, "foo" and "foo (timeout)" do count as duplicate test names,
>> which should have been detected.  This is PR testsuite/29772.
> 
> Thanks for fixing this.  I had one question, see below...
> 
>>
>> Fix this in CheckTestNames::_check_duplicates, such that we get:
>> ...
>> FAIL: $exp: foo
>> FAIL: $exp: foo (timeout)
>> DUPLICATE: $exp: foo (timeout)
>> ...
>>
>> [ One note on the implementation: I used the regexp { \([^()]*\)$}. I don't
>> know whether that covers all required cases, due to the fact that those are
>> not unambiguousely specified.  It might be possible to reverse-engineer that
>> information by reading or running the "regression analysis tools" mentioned on
>> the wiki page [1], but I haven't been able to.  Regardless, the current regexp
>> covers a large amount of cases, which IMO should be sufficient to be
>> acceptable. ]
>>
>> Doing so shows many new duplicates in the testsuite.
>>
>> A significant number of those is due to using a message which is a copy of the
>> command:
>> ...
>> gdb_test "print (1)"
>> ...
>>
>> Fix this by handling those cases using test names "gdb-command<print (1)>" and
>> "gdb-command<print (2)>.
>>
>> Fix the remaining duplicates manually (split off as follow-up patch for
>> readability of this patch).
>>
>> Tested on x86_64-linux and aarch64-linux.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29772
>>
>> [1] https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages
>> ---
>>   gdb/testsuite/lib/check-test-names.exp | 10 ++++++++
>>   gdb/testsuite/lib/gdb.exp              | 34 ++++++++++++++++++++++----
>>   2 files changed, 39 insertions(+), 5 deletions(-)
>>
>> diff --git a/gdb/testsuite/lib/check-test-names.exp b/gdb/testsuite/lib/check-test-names.exp
>> index 8f86f31aad4..11aed63ed11 100644
>> --- a/gdb/testsuite/lib/check-test-names.exp
>> +++ b/gdb/testsuite/lib/check-test-names.exp
>> @@ -64,6 +64,16 @@ namespace eval ::CheckTestNames {
>>       proc _check_duplicates { message } {
>>   	variable all_test_names
>>   
>> +	# Remove test-case prefix, including the space separator.
>> +	set prefix [string_to_regexp "$::subdir/$::gdb_test_file_name.exp: "]
>> +	set message [regsub ^$prefix $message ""]
>> +
>> +	# Remove the "extra information" part.
>> +	set message [regsub { \([^()]*\)$} $message ""]
>> +
>> +	# Add back the test-case prefix.
>> +	set message "${prefix}$message"
> 
> Maybe I missed it, but it's not clear to me why you're removing the
> filename prefix and then adding the regexp version of the filename back
> on again.  Could you explain this part bit more please.
> 

Hi Andrew,

thanks for the review.

You haven't missed it, I meant to make this more explicit, but seem to 
have forgotten about it.

This is related to test-case gdb.base/code-expr.exp.

If I comment out the bits you mention (removing / adding back file name 
prefix), I get:
...
PASS: gdb.base/code-expr.exp: (@code char)
PASS: gdb.base/code-expr.exp: (@code signed char)
DUPLICATE: gdb.base/code-expr.exp: (@code signed char)
...
because what is left of variable message after stripping the extra 
information regexp is "gdb.base/code-expr.exp:" for both passes.

By removing the file name prefix including the space separator, we 
prevent the regexp from triggering.

We could of course drop that bit and update the test-case to use 
different test names.   Please let me know if you have a preference, or 
an alternative idea.

Thanks,
- Tom

> Thanks,
> Andrew
> 
> 
> 
>> +
>>   	# Initialise a count, or increment the count for this test name.
>>   	if {![info exists all_test_names($message)]} {
>>   	    set all_test_names($message) 0
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index dfe19c9410d..e1ca9a6459f 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -965,6 +965,31 @@ proc fill_in_default_prompt {prompt_regexp with_anchor} {
>>       return $prompt_regexp
>>   }
>>   
>> +# Generate message from COMMAND.
>> +#
>> +# This is not trivial in the case that the command contains parentheses.
>> +# Trailing text between parentheses prefixed with a space is interpreted as
>> +# extra information, and not as part of the test name [1].  Consequently,
>> +# "PASS: print (1)" and "PASS: print (2)" count as duplicates.
>> +#
>> +# We fix this here by using "PASS: gdb-command<print (1)>" and
>> +# "PASS: gdb-command<print (2)>".
>> +#
>> +# A trivial way to fix this in a test-case is by using gdb_test "print(1)",
>> +# which produces the nicer-looking "PASS: print(1)".
>> +#
>> +# [1] https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages
>> +
>> +proc command_to_message { command } {
>> +    set message $command
>> +
>> +    if { [regexp { \(([^()]*)\)$} $message] } {
>> +	set message gdb-command<$message>
>> +    }
>> +
>> +    return $message
>> +}
>> +
>>   # gdb_test_multiple COMMAND MESSAGE [ -prompt PROMPT_REGEXP] [ -lbl ]
>>   #                   EXPECT_ARGUMENTS
>>   # Send a command to gdb; test the result.
>> @@ -1095,7 +1120,7 @@ proc gdb_test_multiple { command message args } {
>>       set prompt_regexp [fill_in_default_prompt $prompt_regexp true]
>>   
>>       if { $message == "" } {
>> -	set message $command
>> +	set message [command_to_message $command]
>>       }
>>   
>>       if [string match "*\[\r\n\]" $command] {
>> @@ -1471,7 +1496,6 @@ proc gdb_test_multiline { name args } {
>>       return 0
>>   }
>>   
>> -
>>   # gdb_test [-prompt PROMPT_REGEXP] [-lbl]
>>   #          COMMAND [PATTERN] [MESSAGE] [QUESTION RESPONSE]
>>   # Send a command to gdb; test the result.
>> @@ -1529,7 +1553,7 @@ proc gdb_test { args } {
>>       }
>>   
>>       if { $message == "" } {
>> -	set message $command
>> +	set message [command_to_message $command]
>>       }
>>   
>>       set prompt [fill_in_default_prompt $prompt [expr !${no-prompt-anchor}]]
>> @@ -1795,7 +1819,7 @@ proc gdb_test_lines { command message re args } {
>>       }
>>   
>>       if { $message == ""} {
>> -	set message $command
>> +	set message [command_to_message $command]
>>       }
>>   
>>       set lines ""
>> @@ -2012,7 +2036,7 @@ proc gdb_test_stdio {command inferior_pattern {gdb_pattern ""} {message ""}} {
>>       global gdb_prompt
>>   
>>       if {$message == ""} {
>> -	set message $command
>> +	set message [command_to_message $command]
>>       }
>>   
>>       set inferior_matched 0
>>
>> base-commit: a81f4e591fdb8530d832addc39beb31353b0ef2d
>> -- 
>> 2.35.3
>