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

[Pedro Alves <palves at redhat dot com>]

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