Re: [PATCH] add -s option to make -break-insert support dprintf

Pedro Alves
2013-04-09T17:50:29Z

Hi Hui,

Thanks for the patch.

New MI features need a NEWS entry.

On 03/29/2013 08:01 AM, Hui Zhu wrote:

> + if (hardware && dprintf)
> + error (_("-break-insert: -h and -s cannot be use together"));

"cannot be used"

> @@ -180,11 +189,14 @@ mi_cmd_break_insert (char *command, char
> regular non-jump based tracepoints. */
> type_wanted = (tracepoint
> ? (hardware ? bp_fast_tracepoint : bp_tracepoint)
> - : (hardware ? bp_hardware_breakpoint : bp_breakpoint));
> - ops = tracepoint ? &tracepoint_breakpoint_ops : &bkpt_breakpoint_ops;
> + : (hardware ? bp_hardware_breakpoint
> + : (dprintf ? bp_dprintf : bp_breakpoint)));
> + ops = tracepoint ? &tracepoint_breakpoint_ops
> + : (dprintf ? &dprintf_breakpoint_ops
> + : &bkpt_breakpoint_ops);

This is getting unnecessarily hard for humans to grok. Write
instead as (untested):

if (tracepoint)
{
/* move existing comment on fast tracepoints here */
type_wanted = hardware ? bp_fast_tracepoint : bp_tracepoint;
ops = &tracepoint_breakpoint_ops;
}
else if (dprintf)
{
type_wanted = bp_dprintf;
ops = &dprintf_breakpoint_ops;
}
else
{
type_wanted = hardware ? bp_hardware_breakpoint : bp_breakpoint;
ops = &bkpt_breakpoint_ops;
}

> +@item -s "@var{template},@var{expression}[,@var{expression}@dots{}]"
> +Set a dynamic printf breakpoint, described in @ref{Dynamic Printf}.
> +The @var{location}, @var{template} and @var{expression} should be
> +within double quotations and be escaped by being preceded with a backslash.

Please remove the "location" mention here. It's stale.

I think you either say "double quotation marks" or "double quotes",
never "double quotation".

>
> 2013-03-29 Hui Zhu <hui@codesourcery.com>
>
> * breakpoint.c (dprintf_breakpoint_ops): Remove its static.
> * breakpoint.h (dprintf_breakpoint_ops): Add extern.
> * mi/mi-cmd-break.c (mi_cmd_break_insert): Describe the "-s" option.

Not really describing... "Handle"?

> 2013-03-29 Hui Zhu <hui@codesourcery.com>
>
> * gdb.mi/Makefile.in (PROGS): Add "mi-dprintf".
> * gdb.mi/mi-dprintf.c, gdb.mi/mi-dprintf.h: New.

Missing reference to mi-dprintf.exp.

> + foo (loc++);
> + foo (loc++);
> + foo (loc++);
> + return g;
> +}
> +
> +#include <stdlib.h>

Headers at the top, please.

> +/* Make sure function 'malloc' is linked into program. On some bare-metal
> + port, if we don't use 'malloc', it will not be linked in program. 'malloc'
> + is needed, otherwise we'll see such error message
> +

> +standard_testfile .c

.c is the default.

> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> + untested mi-dprintf.exp

http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#A.22untested.22_calls

> +set target_can_dprintf 1

This should start out as 0. More below.

> +set msg "Set dprintf style to agent"
> +mi_gdb_test "1set dprintf-style agent" "\[^\n\]*\r\ndone"
> +gdb_expect {
> + -re "\\^done" {
> + pass "$msg - can do"

and be set to 1 here.

Should expect ${mi_gdb_prompt} too.

> + }
> + -re ".*" {

Should expect ${mi_gdb_prompt} too.
But what this actually expecting? Is it:

"warning: Target cannot run dprintf commands, falling back to GDB printf"

? Please adjust the -re accordingly.

> + set target_can_dprintf 0
> + pass "$msg - cannot do"

> + }
> + timeout {
> + fail "resume all, waiting for program exit (timeout)"

Certainly "resume all" is a pasto here.

Related to the comment to "set target_can_dprintf 1" above,
e.g., this failure path didn't set target_can_dprintf to 0.

> + }
> +}
> +
> +if $target_can_dprintf {

Why do I get:

PASS: gdb.mi/mi-dprintf.exp: Set dprintf style to agent - cannot do

with gdbserver?

(The test tries the same thing with a few different options. I suspect
it could be simplified with loops and with_test_prefix.)

--
Pedro Alves