Re: [PATCH] Add breakpoint_created observer to update tracepoint_count.

[Pedro Alves <palves@redhat.com>]

On 11/09/2012 01:09 AM, Yao Qi wrote:
> On 11/08/2012 02:13 AM, Pedro Alves wrote:
>> > No need to install an observer for a notification that is emitted in the same
>> > module the new observer is in.  This is internals of the breakpoints module.
>> > All set_breakpoint_count's calls are centralized in install_breakpoint, through
>> > set_breakpoint_number.  All but break-range's, that is.  I don't recall why
>> > that doesn't use install_breakpoint.  Maybe it should.
>> > 
> My original thought is to move tracepoint-related code from
> breakpoint.c to tracepoint.c, so register a breakpoint_created observer
> in tracepoint.c to update 'tracepoint_count'.  Unfortunately,
> tracepoint commands use some breakpoint internal macros that prevent
> moving them to tracepoint.c, so I move the observer register code back
> to breakpoint.c.

I see.

> 
> Yes, it is not necessary to register a observer to update some state in
> the same module.
> 
>> > We should be able to put 'if (is_tracepoint) set_tracepoint_count()' in
>> > install_breakpoint too.
> Hmmm, that sounds the right place to update 'tracepoint_count'.  How
> about patch below?  A new test is added, without this fix, this test
> fails.

Looks good.  Further comments below.

> 2012-11-09  Yao Qi  <yao@codesourcery.com>
>
> 	* gdb.mi/mi-break.exp (test_abreak_creation): New.

I suggest:

 	* gdb.mi/mi-break.exp (test_abreak_creation): New procedure.
	(top level): Call it.

And here ...

> +proc test_abreak_creation {} {
> +    mi_gdb_test "522-break-insert -a main" \
> +	"522\\^done,bkpt=\{number=\"10\",type=\"tracepoint\".*\"\}" \
> +	"break-insert -a operation"
> +
> +    mi_gdb_test "p \$tpnum" ".* = 10.*" "print \$tpnum"
> +}
> +

... I suggest checking tpnum before creating the tracepoint too

BTW, do we need "10.*", or is "10" good enough?  "10.*" matches 100
or other bogus numbers too.  And usually ".*" at the start of the
regex is not necessary.

So all in all (untested):

    mi_gdb_test "p \$tpnum" " = void" "\$tpnum before tracepoint"

    mi_gdb_test "522-break-insert -a main" \
	"522\\^done,bkpt=\{number=\"10\",type=\"tracepoint\".*\"\}" \
	"break-insert -a operation"

    mi_gdb_test "p \$tpnum" " = 10" "\$tpnum after tracepoint"