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

[Andrew Burgess <aburgess@redhat.com>]

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.
> ---
>  gdb/testsuite/gdb.base/maint.exp | 109 ++++++++-----------------------
>  1 file changed, 29 insertions(+), 80 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/maint.exp b/gdb/testsuite/gdb.base/maint.exp
> index 45ccdc6584e..2efdda9aed7 100644
> --- a/gdb/testsuite/gdb.base/maint.exp
> +++ b/gdb/testsuite/gdb.base/maint.exp
> @@ -155,18 +155,10 @@ set have_psyms [expr ! ( $have_gdb_index || $readnow_p )]
>  # guo: on linux this command output is huge.  for some reason splitting up
>  # the regexp checks works.

I think this comment should either be deleted, or rewritten.

Plus, this comment seems to directly contradict the preceeding comment
that says:

  # this command does not produce any output
  # unless there is some problem with the symtabs and psymtabs
  # so that branch will really never be covered in this tests here!!

Which is it?  The command produces no output, or it produces a huge
amount of output?

Turns out, the first comment is correct, there's no output, at least on
my Linux machine.

Looking at maintenance_check_psymtabs, I guess there could, potentially,
be lots of output, if there is a lot of problems with the generated file
that we're debugging for this test, so maybe when the code below, that
you're changing, was added, some targets were seeing such problems?

If we assume that there are still targets out there that do have errors
then we should probably write a comment that's not so contradictory...

>  #
> -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" }
> -	}
> +gdb_test_multiple "maint check-psymtabs" "maint check-psymtabs" {

You can just do:

  gdb_test_multiple "maint check-psymtabs" "" {

the message will default to the command.

> +    -re -wrap "^maint check-psymtabs.*" {

I suspect that this only works because for you, like me, 'maint
check-psymtabs' doesn't produce any output.

The original checks didn't require the entire command output to be
loaded into the expect buffer, while your updated pattern does.  The
"for some reason" that is mentioned in the 'guo' comment is almost
certainly that the expect buffer was getting full.

The patterns should probably be:

    -re "^maint check-psymtabs\r\n" {
	exp_continue
    }

    -re "^$gdb_prompt $" {
	pass $gdb_test_name
    }

    -re "^\[^\r\n\]+\r\n" {
	exp_continue
    }

This will allow the command to run, and discard any lines of output that
are not the prompt.

We could make this test stricter by changing the last pattern to match
all possible lines that maintenance_check_psymtabs can produce (I think
there's only 4 or 5).

We could also make the test stricter by ensuring that the initial 'maint
check-psymtabs' line is seen.

> +	pass "maint check-psymtabs"

You can do:

  pass $gdb_test_name

the '$gdb_test_name' is automatically created based on the value of the
message by gdb_test_multiple.

>      }
> -    -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
> @@ -270,62 +262,38 @@ 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_multiple "$command" "$test_name" {
> +	    -re  -wrap "^maint print psymbols \[^\n\]*" {
> +		gdb_test_multiple "shell grep 'main.*function' $psymbols_output"\
> +		    "$test_name internal"{

I think there should be a space before the `\` and `{` at the end of the
two previous lines.  Not required, that just seems to be the GDB style.

> +			-re -wrap ".main., function, $hex.*" {
> +			    gdb_test "shell rm -f $psymbols_output" ".*" \
> +				"${test_name}: shell rm -f psymbols_output"

I wonder why the 'rm' needed to move into both of the pass blocks?  It
seemed like having it after the gdb_test_multiple would be better.

> +			    pass "$test_name 1"

You could use:

    pass "$gdb_test_name, pattern 1"

here, and similar, with ', pattern 2' for the next 'pass' call.

> +			}
> +			-re -wrap ".*main.  .., function, $hex.*" {
>  			    gdb_test "shell rm -f $psymbols_output" ".*" \
>  				"${test_name}: shell rm -f psymbols_output"
> +			    pass "$test_name 2"
>  			}
> -			-re ".*$gdb_prompt $" { fail "$test_name" }
> -			timeout { fail "$test_name (timeout)" }
> -		    }
>  		}
> -		-re ".*$gdb_prompt $" { fail "$test_name" }
> -		timeout { fail "$test_name (timeout)" }
>  	    }
> +	}
>      }
>  }
>  
>  
>  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)" }
> -	}
> +gdb_test_multiple "maint print msymbols -objfile ${binfile} $msymbols_output" \
> +"printing msymbols" {

I think this line should be indented.

> +    -re -wrap "^maint print msymbols \[^\n\]*" {
> +	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"
>      }

Couldn't these tests just be serialised, rather than being nested? e.g.:

   gdb_test_no_output "maint print msymbols -objfile ${binfile} $msymbols_output" \
       "printing msymbols"

   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"

I haven't tested this, so maybe there's a reason why this wouldn't work.

Thanks,
Andrew



> -    -re ".*$gdb_prompt $" { fail "maint print msymbols" }
> -    timeout { fail "maint print msymbols (timeout)" }
>  }
>  
>  # Check that maint print msymbols allows relative pathnames
> @@ -363,31 +331,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