[PATCH][gdb/testsuite] Introduce gdb_test_ext

[Tom de Vries]

On 19-09-19 21:24, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2019-09-19 21:01:01 +0200]:
> 
>> On 19-09-19 18:18, Andrew Burgess wrote:
>>> * Tom de Vries <tdevries@suse.de> [2019-09-19 13:13:23 +0200]:
>>>
>>>> Hi,
>>>>
>>>> In commit 25e5c20918 "[gdb/testsuite] Allow some tests in gdb.base/store.exp
>>>> to be unsupported" we replace a gdb_test:
>>>> ...
>>>>     gdb_test "print l" " = ${l}" \
>>>>        "${prefix}; print old l, expecting ${l}"
>>>> ...
>>>> with a gdb_test_multiple:
>>>> ...
>>>>     set supported 1
>>>>     set test "${prefix}; print old l, expecting ${l}"
>>>>     gdb_test_multiple "print l" "$test"  {
>>>>        -re " = <optimized out>\r\n$gdb_prompt $" {
>>>>            unsupported $test
>>>>            set supported 0
>>>>        }
>>>>        -re " = ${l}\r\n$gdb_prompt $" {
>>>>            pass $test
>>>>        }
>>>>     }
>>>> ...
>>>> in order to handle the UNSUPPORTED case.
>>>>
>>>> This has the drawback that we have to be explicit about the gdb_prompt, and
>>>> move the gdb_test arguments around to fit the gdb_test_multiple format.
>>>>
>>>> Introduce a new proc gdb_test_ext that behaves as gdb_test, but also allows
>>>> extension, allowing us to rewrite the gdb_test_multiple above in a form
>>>> resembling the original gdb_test:
>>>> ...
>>>>     set supported 1
>>>>     gdb_test_ext "print l" " = ${l}" \
>>>>        "${prefix}; print old l, expecting ${l}" \
>>>>        -- [list "unsupported" " = <optimized out>" "set supported 0"]
>>>
>>> I've also thought about this sort of problem in the past, and would
>>> like to propose a similar, but slightly different solution.
>>>
>>> My idea is more like a trimmed down gdb_test_multiple, so for your
>>> example above you would write this:
>>>
>>>     gdb_test_ext "print l" "${prefix}; print old l, expecting ${l}" {
>>> 	" = ${l}" {
>>> 	    pass $gdb_test_ext_name
>>> 	}
>>> 	" = <optimized out>" {
>>> 	    unsupported $gdb_test_ext_name
>>> 	    set supported 0
>>> 	}
>>>     }
>>>
>>> You don't put '-re' before each pattern, this is because they aren't
>>> full patterns, gdb_test_ext will be extending them.
>>>
>>> Unlike your solution the 'pass' case is not created automatically, you
>>> need to write it yourself, so maybe that's a negative.  The advantages
>>> I see of this approach is that there's not special handling for
>>> different "types" of alternatives as in your original code, the action
>>> block can contain anything 'unsupported', 'fail', etc.  Plus it's
>>> formatted as a code body, which I like.
>>>
>>
>> The solution as I proposed it doesn't limit itself to require each
>> alternative to be handled as either supported or pass or fail or
>> somesuch.  It just adds a means to extend gdb_test using a keyword that
>> determines how the keyword arguments are handled.
> 
> I don't really understand what you're trying to say here, sorry.
> Looking at the code it still appears that each case would need to be
> added specifically, so if tomorrow I want a 'fail' case, I'd need to
> add it to lib/gdb.exp - or am I not understanding?
> 

I thought you were arguing that it was not possible handle things
generically with my proposed solution (which is not the case, as I tried
to demonstrate by adding the "generic" method), so I tried to argue
against that. But I guess I misunderstood you there.

So confirmed, adding a fail case requires adding to gdb_test_ext in
gdb.exp. [ Although in the latest version you could handle it using the
"generic" method, but I'd prefer to add a "fail" method instead. ]

>>
>> So, I've added the style you propose here as "generic", and rewrote one
>> of the two places I update in store.exp using the "generic" style for
>> demonstration purposes.
>>
>> I envision the usage like this: you'd usually use "unsupported" or
>> similar to skip all the repetitive code and focus on the bits that
>> actually contain content, and for special cases where that doesn't fit
>> you'd use "generic". You can go further and add a "freeform" or some
>> such where you'd have to write out the entire regexp.
>>
>> The nice thing is that you can add keywords and corresponding handling
>> as you go, whereas the gdb_test_multiple-like solution you propose only
>> has one way of handling its arguments, which of course does makes things
>> consistent and clear, but is not very extensible.
> 
> I'm still not seeing which arguments can only be handled in one way.
> Could you give an example maybe?

Well, you gave this example:
...
gdb_test_ext "print l" "${prefix}; print old l, expecting ${l}" {
  " = ${l}" {
    pass $gdb_test_ext_name
  }
  " = <optimized out>" {
    unsupported $gdb_test_ext_name
    set supported 0
  }
}
...
It has (ignoring the first two) two arguments, one containing the 'pass'
and one containing the 'unsupported'. Each is handled in the exact same way.

> I do see that you're suggestion
> improves the existing test - removing the need to set an extra 'test'
> variable, and not having to add $gdb_prompt, but when all is said and
> done, the pattern is still just a pattern, and the code is still just
> code - how is this any different to gdb_test_multiple?
> 

It's much less verbose, and eliminates as much as possible code that is
commonly occurring, in order to avoid errors and omissions there.

>>
>>> One other thing which I've wanted for _ages_ is to avoid having to set
>>> the test name into a separate variable, which your solution offers
>>> too.  The solution I offer is '$gdb_test_ext_name', this variable is
>>> set auto-magically by the call to 'gdb_test_ext, and is available in
>>> all of the action bodies for calls to pass/fail/unsupported/etc.
>>>
>>
>> Nice trick, I've copied that for usage in the "generic" method.
>>
>> So, WDYT?
> 
> I'm still not convinced.
> 
> On further thought, I actually think there's no need for an extra
> function at all, we can get all the benefit (as I see it) by possibly
> updating gdb_test_multiple.  I'm travelling right now so can't code
> this up, but I think a solution that does something like this:
> 
>      gdb_test_multiple "command" "test name" {
>        -re "full regexp here$gdb_prompt" {
>          pass $gdb_test_multiple_name
>        }
>        -output "pattern without prompt" {
>          fail $gdb_test_multiple_name
>        }
>      }
> 
> So using '-re' and '-output' to specialise the behaviour of
> gdb_test_multiple, and adding in the $gdb_test_multiple_name variable.
> 
> When I get back to my desk I'll try to code this up.
> 

Agreed, I think it makes sense to fold that into gdb_test_multiple.

> I'm know the above isn't going to satisfy you though - it's basically
> an iteration on what I already proposed

Yeah, I think we're looking for different things here: less verbosity at
the cost of specialization vs. generality at the cost of more verbosity.

> - maybe you could expand on
> the benefits of you solution a bit more.
> 

Sure. The benefits of my solution as I see them:
- uses gdb_test semantics for args before '--', minimizing the effort to
  rewrite from gdb_test to gdb_test_ext
- maximally eliminates repetitive code, to prevent error and omissions
  in there and reduce verbosity
- extensible. Each method is handled completely independent, and new
  methods can be added as needed.

Thanks,
- Tom