[RFC,gdb/testsuite] Add -lbl option in gdb_test_multiple

  [ was: Re: [RFC][gdb/testsuite] Handle -line and -non-empty-line in
gdb_test_multiple ]
On 20-02-2020 14:27, Pedro Alves wrote:
> On 2/19/20 9:30 PM, Tom de Vries wrote:
>> [ was: Re: [PATCH][gdb/testsuite] Fix corefile-buildid.exp with
> 
>> Committed.
>>
>>> If this pattern appears in more places it may be worth it to
>>> think about some abstraction to make it easier to write.
>>> Like e.g., a new "-lbl" (line-by-line) option switch to
>>> gdb_test_multiple that auto-appends the "match one line" regexp.
>>
>> How about this?
> 
> Oh, by "pattern", I meant the design pattern, the higher-level
> thing of expecting a string while at the same time consuming
> input a line at a time.

Right, I sort of got that, but went for a simpler form of abstraction.

> Something like:
> 
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> diff --git c/gdb/testsuite/lib/gdb.exp w/gdb/testsuite/lib/gdb.exp
> index d8ebddf63ce..6eaebc7710d 100644
> --- c/gdb/testsuite/lib/gdb.exp
> +++ w/gdb/testsuite/lib/gdb.exp
> @@ -858,6 +858,7 @@ proc gdb_test_multiple { command message user_code { prompt_regexp "" } } {
>      set expecting_action 0
>      set expecting_arg 0
>      set wrap_pattern 0
> +    set line_by_line 0
>      foreach item $user_code subst_item $subst_code {
>         if { $item == "-n" || $item == "-notransfer" || $item == "-nocase" } {
>             lappend $current_list $item
> @@ -880,6 +881,10 @@ proc gdb_test_multiple { command message user_code { prompt_regexp "" } } {
>             set wrap_pattern 1
>             continue
>         }
> +       if {$item == "-lbl"} {
> +           set line_by_line 1
> +           continue
> +       }
>         if { $expecting_arg } {
>             set expecting_arg 0
>             lappend $current_list $subst_item
> @@ -1070,6 +1075,14 @@ proc gdb_test_multiple { command message user_code { prompt_regexp "" } } {
>         }
>      }
>  
> +    if {$line_by_line} {
> +       append code {
> +           -re "^\[^\r\n\]*\r\n" {
> +               exp_continue
> +           }
> +       }
> +    }
> +
>      # Now patterns that apply to any spawn id specified.
>      append code {
>         -i $any_spawn_id
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> That should mean we could drop the $gdb_prompt matches too.
> But maybe a new gdb_test_lbl procedure that wraps gdb_test_multiple
> would be preferred.
> 

I think we don't want to integrate -lbl in the user_code parsing, because:
...
gdb_test_multiple "command" "testname" {
	-re "bla" {
	}
	-lbl
}
...
and:
...
gdb_test_multiple "command" "testname" {
	-lbl
	-re "bla" {
	}
}
...
will have the same effect, which is likely to cause confusion.

So I've added it as option to gdb_test_multiple. [ Which caused me to
rewrite the optional $prompt_regexp argument to -prompt $prompt_regexp. ]

I had to rewrite the -lbl regexp to "\r\n" at start-of-line rather than
end-of-line, because it didn't work well otherwise with the implicit
regexps that are listed before it.  Consequently, I had to rewrite the
regexps in corefile-buildid.exp in a similar way.

> The way you have it doesn't look very different from
> creating a couple globals, like:
> 
>  set line_re "^\[^\r\n\]*\r\n"
>  set non_empty_line_re "^\[^\r\n\]+\r\n"
> 
> and the using them like:
> 
>  -re "$line_re" {
> 
> Maybe removing the leading "^" anchor from the variables
> would make them usable in more places.
> 

Ack, that all makes sense.

> Anyway, I don't object to your patch if you like it.  It just
> wasn't what I was suggesting.

I'll drop that one for now, maybe rewrite it at some point to use some
global variables instead.

So how about this -lbl implementation? Any comments (other than missing
documentation in gdb_test_multiple proc header)?

Reg-tested on x86_64-linux.

Tested corefile-buildid.exp with check-read1.

Thanks,
- Tom
  

On 2/21/20 3:35 PM, Tom de Vries wrote:

> I think we don't want to integrate -lbl in the user_code parsing, because:
> ...
> gdb_test_multiple "command" "testname" {
> 	-re "bla" {
> 	}
> 	-lbl
> }
> ...
> and:
> ...
> gdb_test_multiple "command" "testname" {
> 	-lbl
> 	-re "bla" {
> 	}
> }
> ...
> will have the same effect, which is likely to cause confusion.

Well:

 gdb_test_multiple "command" "testname" {
	-re "bar" {
	}
	-timeout 60
 }

and:

 gdb_test_multiple "command" "testname" {
	-timeout 60
	-re "bla" {
	}
 }

also have the same effect.  So there's precedent and it goes
all the way back to expect.

> [gdb/testsuite] Add -lbl option in gdb_test_multiple
> 
> Add gdb_test_multiple option -lbl, that adds a regexp after the user code that
> reads one line, and discards it:
> ...
>            -re "\r\n\[^\r\n\]*(?=\r\n)" {
>                exp_continue
>            }
> ...
> 
> In order to be able to write:
> ...
> gdb_test_multiple "command" "testname" {
>   ...
> } -lbl
> ...
> as opposed to having to insert the "" for the prompt_regexp arguments:
> ...
> gdb_test_multiple "command" "testname" {
>   ...
> } "" -lbl
> ...
> rewrite the promp_regexp argument usage into:
> ...
> gdb_test_multiple "command" "testname" {
>   ...
> } -prompt $prompt_regexp
> ...

Honestly, to me, this:

 gdb_test_multiple "command" "testname" {
   ...
 } -lbl -prompt $prompt_regexp

looks a bit harder to grok than:

 gdb_test_multiple "command" "testname" {
   -lbl
   -prompt $prompt_regexp
   ...
 }

... because those options appear at the end, after the regexes.

Alternatively, if you don't like the -lbl within the {} block, and if
we're going to use "-" options, how about supporting them before
the {} user code block, so that the user code block is always
at the end?  Like:

 gdb_test_multiple "command" "testname" -lbl {
   ...
 }

 gdb_test_multiple "command" "testname" -prompt $prompt_regexp {
   ...
 }

That should be doable with:

 -proc gdb_test_multiple { command message user_code { prompt_regexp "" } } {
 +proc gdb_test_multiple { command message args } {

and then walking the $args list, processing "-" arguments, and interpreting
the first non-"-" argument as the user code block (and erroring out if there
are more arguments).  I think the gdb_test_multiple code would look quite similar
to your patch, except that the user_code parameter would no longer be a parameter,
instead it would be a local variable set to the first non-"-" element of $args.

Thanks,
Pedro Alves