Re: [PATCH 5/6] Test tracepoints are installed or not

[Pedro Alves <palves at redhat dot com>]

On 12/12/2012 02:59 AM, Yao Qi wrote:
> On 12/12/2012 01:53 AM, Pedro Alves wrote:
>>
>> this use of pass+exp_continue is fragile.  Say a GDB bug is
>> introduced, and the second =breakpoint-modified fails to be output.  In
>> that case, this will result in:
>>
>>       PASS: tracepoint on pendfunc2 resolved
>>       FAIL: tracepoint on pendfunc2 resolved
>>
>> instead of
>>       PASS: tracepoint on pendfunc2 resolved
>>       FAIL: tracepoint on pendfunc2 installed
>>
> 
> My intention of using exp_coutine here is to handle the situation that
> notifications may arrive in different orders.  The fail message is not
> clear, but not a big deal, to me.  It is important to catch a fail and
> then we can check gdb.log and test case to fix it.

The text of the fail message isn't that important, I agree, but
ending up with the _same_ text for both the PASS and the FAIL (the whole
duplicate messages issue, PR13443) is something we're actively
trying to avoid.

> 
>> You could fix that by doing:
>>
>>   +	    # Pending tracepoint is resolved.
>>    	    pass "$test"
>>   +	    set test "tracepoint on pendfunc2 installed"
>>   +	    exp_continue
>>
>> But then, if the order of the notifications changes (IOW, due to a bug,
>> we end up with the tracepoint not installed), this won't catch it.
> 
> Looks the fix is worse than the original one, at least no fail is
> missing in the original one.

Not sure what you mean.  The only change compared to the original one
would be the new

	    set test "tracepoint on pendfunc2 installed"

line.  Sorry if that wasn't clear.  (you'd probably change the other pass
to be 'pass "$test"' too then.)

> 
>> It seems best to me to only use exp_continue in cases we won't to
>> consume/skip output, and in the case of this patch, split the two
>> tests into two consecutive gdb_expects.
> 
> If this way, we can't handle that two notifications arrive in a
> reversed order (which is also correct).  

This seems to be crux of the issue here.  I don't understand how
reverse order would be correct.  This:

 =breakpoint-modified,bkpt={number="1",type="tracepoint".*.*times=\"0\".*installed="n"
 =breakpoint-modified,bkpt={number="1",type="tracepoint".*.*times=\"0\".*installed="y"

means the frontend ends up thinking the tracepoint is installed, while this:

 =breakpoint-modified,bkpt={number="1",type="tracepoint".*.*times=\"0\".*installed="y"
 =breakpoint-modified,bkpt={number="1",type="tracepoint".*.*times=\"0\".*installed="n"

means the frontend ends up thinking the tracepoint is not installed, which
I'd think is not what we want in this test.

> If we don't have to worry about the order 

Confused, because I'm suggesting to worry about the order.  :-)

> (after all, the order of MI notifications are still
> deterministic nowadays), I am OK with your suggestion.

> 2012-12-12  Yao Qi  <yao@codesourcery.com>
>
> 	* gdb.trace/mi-tracepoint-changed.exp (test_pending_resolved): Check
> 	'installed' field in '=breakpoint-modified'.
> 	(test_reconnect): Check 'installed' field in
> 	'=breakpoint-modified' and '=breakpoint-created'.

This version looks good to me.  Thanks,

-- 
Pedro Alves