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

[Hui Zhu <teawater at gmail dot com>]

Hi Pedro,

Thanks for your review.

On Wed, Apr 10, 2013 at 1:50 AM, Pedro Alves <palves@redhat.com> wrote:
> Hi Hui,
>
> Thanks for the patch.
>
> New MI features need a NEWS entry.

This is for NEWS:
  ** The -s of MI command -break-insert can set a dynamic printf.

>
> 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"

Fixed.

>
>> @@ -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;
>     }
>

Fixed.

>
>> +@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".

Fixed.

>
>>
>> 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"?

Fixed.

>
>> 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.

Fixed.

>
>
>> +  foo (loc++);
>> +  foo (loc++);
>> +  foo (loc++);
>> +  return g;
>> +}
>> +
>> +#include <stdlib.h>
>
> Headers at the top, please.

Fixed.

>
>> +/* 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.

Fixed.

>
>> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
>> +     untested mi-dprintf.exp
>
> http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#A.22untested.22_calls
>

Looks test doesn't need it now.  So I removed it.

>
>
>
>> +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.

pasto?

>
> 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?

Set dprintf style to agent need test with gdbserver.
I update this pass to unsupported.

And also update this part to make it test OK 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.)

I moved them to a proc and add with_test_prefix.

I post a new version according to your comments.  Please help me review it.

Best,
Hui

>
> --
> Pedro Alves


2013-04-11  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): Handle the "-s" option.

2013-04-11  Hui Zhu  <hui@codesourcery.com>

* gdb.texinfo (GDB/MI Breakpoint Commands): Describe the "-s" option.

2013-04-11  Hui Zhu  <hui@codesourcery.com>

* gdb.mi/Makefile.in (PROGS): Add "mi-dprintf".
* gdb.mi/mi-dprintf.exp, gdb.mi/mi-dprintf.c: New.

Attachment: mi-dprintf.txt
Description: Text document

Attachment: mi-dprintf-doc.txt
Description: Text document

Attachment: mi-dprintf-test.txt
Description: Text document