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

[Tom de Vries <tdevries@suse.de>]

On 1/28/21 7:20 PM, Simon Marchi wrote:
> 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.

Right, nevertheless I think it was good to go over this and try to make
thing more readable.  So, thanks for the review :)

Committed with the state machine part removed.

Thanks,
- Tom