Re: [PATCH][gdb/testsuite] Fix gdb.opt/solib-intra-step.exp with -m32 and gcc-10

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-01-28 1:15 p.m., Tom de Vries wrote:> On 1/28/21 7:04 PM, Simon Marchi wrote:
>>>>> @@ -89,7 +89,7 @@ gdb_test_multiple "step" $test {
>>>>>  	exp_continue
>>>>>      }
>>>>>      -re -wrap "get_pc_thunk.*" {
>>>>> -	if  { $state != 1 } {
>>>>> +	if  { $state != 0 && $state != 1 } {
>>>>>  	    set state -1
>>>>>  	} else {
>>>>>  	    set state 2
>>>>>
>>>>
>>>> I don't really understand what happens here, what state value means what.
>>>>
>>>> A bit of commenting would help.
>>>
>>> I tried to add comments but didn't manage to come up with something
>>> sensible.
>>>
>>> Instead, I simplified gdb_test_multiple to just track the order of
>>> events, and then added a few asserts about order of events.
>>>
>>> I hope this clarifies what the test is trying to do.  WDYT?
>>
>> Hmm, it's still not clear to me what the intention of the test is.  It's
>> not clear what kind of good or bad behavior from GDB we are looking for.
>> That intention needs to be recorded in a comment, otherwise, I can't
>> tell if the code matches what we want (since I don't know what we want).
>> I kind of understand now that we do a step, we want to get until the
>> "first-hit" line (or "second-hit" in the second case), but it's possible
>> that we land on intermediary states, which are acceptable.  But there
>> also seems to be an ordering component?  Why is that important?  Why
>> don't we simply "exp_continue" when seeing "retry" or "get_pc_thunk",
>> why bother recording anything?
> 
> Ah, I see.
> 
> Well, it's an attempt to be precise about what we accept in the test.
> Much in the same way that two subsequent gdb_test do that.
> 
> But yeah, I don't think it's really important, so I can drop that part.

Thanks, and to be clear I don't have anything against what you suggest,
the test was lacking proper documentation before you touched it.

Simon