Re: [PATCHv3] gdb/testsuite: modernize gdb.base/maint.exp

[Bruno Larsen <blarsen@redhat.com>]

On 6/29/22 07:33, Andrew Burgess wrote:
> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> gdb.base/maint.exp was using several gdb_expect statements, probably
>> because this test case predates the existance of gdb_test_multiple. This
>> commit updates the test case to use gdb_test_multiple, making it more
>> resilient to internal errors and such.
>>
>> The only gdb_expect left in the testcase is one that specifically looks
>> for an internal error being triggered as a PASS.
>> ---
>>
>> Changes for version 3:
>>      * changed patterns when testing psymbols, now using trailing
>> parenthesis
>>      * used -lbl when running "check psymtabs"
>>      * made use of boolean variables and gdb_assert when testing
>> psymtabs
>>      * Simplified msymbols and psymbosl test to 3 tests, instead of
>> nested tests
>>
>> Changes for v2:
>>   - Addressed Andrew's comments
>>   - rebased on current master.
>>
>> ---
>>   gdb/testsuite/gdb.base/maint.exp | 142 ++++++++++---------------------
>>   1 file changed, 43 insertions(+), 99 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.base/maint.exp b/gdb/testsuite/gdb.base/maint.exp
>> index 2817c6eafb9..aea8a10df0c 100644
>> --- a/gdb/testsuite/gdb.base/maint.exp
>> +++ b/gdb/testsuite/gdb.base/maint.exp
>> @@ -151,22 +151,20 @@ set have_psyms [expr ! ( $have_gdb_index || $readnow_p )]
>>   # unless there is some problem with the symtabs and psymtabs
>>   # so that branch will really never be covered in this tests here!!
>>   #
>> +# When there is a problem, there may be loads of output, which can
>> +# overwhelm the expect buffer. Splitting it seems to fix those
>> +# issues.
>> +
>> +set seen_command false
>> +gdb_test_multiple "maint check-psymtabs" "" -lbl {
> 
> Bruno,
> 
> Thanks for continuing to work on this...
> 
> Sorry to be a pain, but I don't think -lbl will actually do what you
> want here.  To me the implementation of -lbl just looks weird, and I'm
> not sure how it is expected to help.

Don't worry. This is why reviews are useful!

> 
> Your patterns are:
> 
>      -re "^maint check-psymtabs\r\n" {
> 	set seen_command true
> 	exp_continue
>      }
> 
>      -re "^$gdb_prompt $" {
> 	gdb_assert { $seen_command } $gdb_test_name
>      }
> 
> And -lbl adds:
> 
>      -re "\r\n\[^\r\n\]*(?=\r\n)" {
>          exp_continue
>      }
> 
> The problem I think exists here is that your first pattern runs from the
> star of the buffer and consumes the trailing \r\n.  Your prompt pattern
> also expects to start from the start of the buffer.
> 
> If the output is:
> 
>    maint check-psymtabs\r\n
>    (gdb)
> 
> Then these patterns will work fine.  But if your output is:
> 
>    maint check-psymtabs\r\n
>    Some other line here\r\n
>    (gdb)
> 
> Then your first pattern will consume the first line, including the
> trailing \r\n.  The -lbl pattern will then fail to match the second line
> as the -lbl pattern expects a leading \r\n.  And now your pattern will
> fail to match the third line as the second line is still in the buffer.
> 
> The -lbl pattern always leaves a trailing \r\n in the expect buffer,
> which will cause your prompt pattern to fail.
> 
> Personally, I'd just drop the use of -lbl and add what I think is the
> more correct pattern:
> 
>    -re "^\[^\r\n\]+\r\n" {
>      exp_continue
>    }
> 
> which is what you originally proposed.  But if you really want to make
> use of -lbl then you can change your patterns to:
> 
>      -re "^maint check-psymtabs(?=\r\n)" {
> 	set seen_command true
> 	exp_continue
>      }
> 
>      -re "\r\n$gdb_prompt $" {
> 	gdb_assert { $seen_command } $gdb_test_name
>      }
> 
> I don't think that's an improvement, but I think that should work.

This makes sense. I didn't really look at how -lbl worked because I wanted
to reduce complexity on the gdb_test_multiple call, but if I need to check
end of line without consuming, I don't think we're really reducing the
complexity of anything. I'll revert this to how it was on v2.

> 
>> +    -re "^maint check-psymtabs\r\n" {
>> +	set seen_command true
>> +	exp_continue
>> +    }
>>   
>> -# guo: on linux this command output is huge.  for some reason splitting up
>> -# the regexp checks works.
>> -#
>> -send_gdb "maint check-psymtabs\n"
>> -gdb_expect  {
>> -    -re "^maint check-psymtabs" {
>> -	gdb_expect {
>> -	    -re "$gdb_prompt $" {
>> -		pass "maint check-psymtabs"
>> -	    }
>> -	    timeout { fail "(timeout) maint check-psymtabs" }
>> -	}
>> +    -re "^$gdb_prompt $" {
>> +	gdb_assert { $seen_command } $gdb_test_name
>>       }
>> -    -re ".*$gdb_prompt $"     { fail "maint check-psymtabs" }
>> -    timeout         { fail "(timeout) maint check-psymtabs" }
>>   }
>>   
>>   # This command does not produce any output unless there is some problem
>> @@ -272,63 +270,33 @@ if { $have_psyms } {
>>   		       "maint print psymbols -pc" \
>>   		       "maint print psymbols -pc main $psymbols_output"]
>>       foreach { test_name command } $test_list {
>> -	send_gdb "$command\n"
>> -	    gdb_expect  {
>> -		-re "^maint print psymbols \[^\n\]*\r\n$gdb_prompt $" {
>> -		    send_gdb "shell ls $psymbols_output\n"
>> -		    gdb_expect {
>> -			-re "$psymbols_output_re\r\n$gdb_prompt $" {
>> -			    # We want this grep to be as specific as possible,
>> -			    # so it's less likely to match symbol file names in
>> -			    # psymbols_output.  Yes, this actually happened;
>> -			    # poor expect got tons of output, and timed out
>> -			    # trying to match it.   --- Jim Blandy <jimb@cygnus.com>
>> -			    send_gdb "shell grep 'main.*function' $psymbols_output\n"
>> -			    gdb_expect {
>> -				-re ".main., function, $hex.*$gdb_prompt $" {
>> -				    pass "$test_name 1"
>> -				}
>> -				-re ".*main.  .., function, $hex.*$gdb_prompt $" {
>> -				    pass "$test_name 2"
>> -				}
>> -				-re ".*$gdb_prompt $" { fail "$test_name" }
>> -				timeout { fail "$test_name (timeout)" }
>> -			    }
>> -			    gdb_test "shell rm -f $psymbols_output" ".*" \
>> -				"${test_name}: shell rm -f psymbols_output"
>> +	gdb_test_multiple "$command" "$test_name" {
>> +	    -re  -wrap "^maint print psymbols \[^\n\]*" {
>> +		gdb_test_multiple "shell grep 'main.*function' $psymbols_output" \
>> +		    "$test_name internal" {
>> +			-re -wrap ".main., function, $hex.*" {
>> +			    pass "$test_name (pattern 1)"
>> +			}
>> +			-re -wrap ".*main.  .., function, $hex.*" {
>> +			    pass "$test_name (pattern 2)"
>>   			}
>> -			-re ".*$gdb_prompt $" { fail "$test_name" }
>> -			timeout { fail "$test_name (timeout)" }
>> -		    }
>>   		}
>> -		-re ".*$gdb_prompt $" { fail "$test_name" }
>> -		timeout { fail "$test_name (timeout)" }
>>   	    }
>> +	}
>> +	gdb_test "shell rm -f $psymbols_output" ".*" \
>> +	    "${test_name}: shell rm -f psymbols_output"
>>       }
> 
> Again, apologies for only just spotting this, but I think this test can
> be further simplified to just:
> 
>      foreach { test_name command } $test_list {
>          with_test_prefix "$test_name" {
>            gdb_test_no_output "$command" "collect output"
>            gdb_test_multiple "shell grep 'main.*function' $psymbols_output" \
>              "$analyse output" {
>    	        -re -wrap ".main., function, $hex.*" {
>    	            pass "$gdb_test_name (pattern 1)"
>    	        }
>    	        -re -wrap ".*main.  .., function, $hex.*" {
>    	            pass "$gdb_test_name (pattern 2)"
>                  }
>            }
>    	  gdb_test "shell rm -f $psymbols_output" ".*" \
>    	      "shell rm -f psymbols_output"
>          }
>      }
> 
> I think we already made a similar change later on in this script, so
> this just brings this test into line with the later ones.

Alright, makes sense too. v4 coming soon!

Cheers!
Bruno Larsen

> 
> Thanks,
> Andrew
> 
>>   }
>>   
>>   
>>   set msymbols_output [standard_output_file msymbols_output]
>>   set msymbols_output_re [string_to_regexp $msymbols_output]
>> -send_gdb "maint print msymbols -objfile ${binfile} $msymbols_output\n"
>> -gdb_expect  {
>> -    -re "^maint print msymbols \[^\n\]*\r\n$gdb_prompt $" {
>> -	send_gdb "shell ls $msymbols_output\n"
>> -	gdb_expect {
>> -	    -re "$msymbols_output_re\r\n$gdb_prompt $" {
>> -		gdb_test "shell grep factorial $msymbols_output" \
>> -		    "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex \\.?factorial.*" \
>> -		    "maint print msymbols, absolute pathname"
>> -		gdb_test "shell rm -f $msymbols_output" ".*" \
>> -		    "shell rm -f msymbols_output"
>> -	    }
>> -	    -re ".*$gdb_prompt $" { fail "maint print msymbols" }
>> -	    timeout { fail "maint print msymbols (timeout)" }
>> -	}
>> -    }
>> -    -re ".*$gdb_prompt $" { fail "maint print msymbols" }
>> -    timeout { fail "maint print msymbols (timeout)" }
>> -}
>> +gdb_test_no_output "maint print msymbols -objfile ${binfile} $msymbols_output" \
>> +    "print msymbols to file, with absolute path"
>> +gdb_test "shell grep factorial $msymbols_output" \
>> +    "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex \\.?factorial.*" \
>> +    "maint print msymbols, absolute pathname"
>> +gdb_test "shell rm -f $msymbols_output" ".*" "remove absolute path msymbols"
>>   
>>   # Check that maint print msymbols allows relative pathnames
>>   set mydir [pwd]
>> @@ -336,18 +304,13 @@ gdb_test "cd [standard_output_file {}]" \
>>       "Working directory .*\..*" \
>>       "cd to objdir"
>>   
>> -gdb_test_multiple "maint print msymbols -objfile ${testfile} msymbols_output2" "maint print msymbols" {
>> -    -re "^maint print msymbols \[^\n\]*\r\n$gdb_prompt $" {
>> -    	gdb_test_multiple "shell ls msymbols_output2" "maint print msymbols" {
>> -	    -re "msymbols_output2\r\n$gdb_prompt $" {
>> -		gdb_test "shell grep factorial msymbols_output2" \
>> -		    "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex \\.?factorial.*" \
>> -		    "maint print msymbols, relative pathname"
>> -		gdb_test "shell rm -f msymbols_output2" ".*"
>> -	    }
>> -	}
>> -    }
>> -}
>> +gdb_test_no_output "maint print msymbols -objfile ${testfile} $msymbols_output"\
>> +    "print msymbols to file, with relative path"
>> +gdb_test "shell grep factorial $msymbols_output" \
>> +    "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex \\.?factorial.*" \
>> +    "maint print msymbols, relative pathname"
>> +gdb_test "shell rm -f msymbols_output" ".*" "remove relative path msymbols"
>> +
>>   gdb_test "cd ${mydir}" \
>>       "Working directory [string_to_regexp ${mydir}]\..*" \
>>       "cd to mydir"
>> @@ -365,31 +328,12 @@ set test_list [list \
>>   		   "maint print symbols -pc" \
>>   		   "maint print symbols -pc main $symbols_output"]
>>   foreach { test_name command } $test_list {
>> -    send_gdb "$command\n"
>> -    gdb_expect {
>> -	-re "^maint print symbols \[^\n\]*\r\n$gdb_prompt $" {
>> -	    send_gdb "shell ls $symbols_output\n"
>> -	    gdb_expect {
>> -		-re "$symbols_output_re\r\n$gdb_prompt $" {
>> -		    # See comments for `maint print psymbols'.
>> -		    send_gdb "shell grep 'main(.*block' $symbols_output\n"
>> -		    gdb_expect {
>> -			-re "int main\\(int, char \\*\\*, char \\*\\*\\); block.*$gdb_prompt $" {
>> -			    pass "$test_name"
>> -			}
>> -			-re ".*$gdb_prompt $" { fail "$test_name" }
>> -			timeout { fail "$test_name (timeout)" }
>> -		    }
>> -		    gdb_test "shell rm -f $symbols_output" ".*" \
>> -			"$test_name: shell rm -f symbols_output"
>> -		}
>> -		-re ".*$gdb_prompt $" { fail "$test_name" }
>> -		timeout { fail "$test_name (timeout)" }
>> -	    }
>> -	}
>> -	-re ".*$gdb_prompt $" { fail "$test_name" }
>> -	timeout { fail "$test_name (timeout)" }
>> -    }
>> +    gdb_test_no_output "$command" "$test_name generate"
>> +    gdb_test "shell grep 'main(.*block' $symbols_output"\
>> +	"int main\\(int, char \\*\\*, char \\*\\*\\); block.*"\
>> +	"$test_name read"
>> +    gdb_test "shell rm -f $symbols_output" ".*" \
>> +	"$test_name: shell rm -f symbols_output"
>>   }
>>   
>>   set msg "maint print type"
>> -- 
>> 2.31.1
>