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

[Hui Zhu <teawater@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5354 bytes --]

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 #2: mi-dprintf.txt --]
[-- Type: text/plain, Size: 3823 bytes --]

--- a/breakpoint.c
+++ b/breakpoint.c
@@ -305,7 +305,7 @@ struct breakpoint_ops bkpt_breakpoint_op
 static struct breakpoint_ops bkpt_probe_breakpoint_ops;
 
 /* Dynamic printf class type.  */
-static struct breakpoint_ops dprintf_breakpoint_ops;
+struct breakpoint_ops dprintf_breakpoint_ops;
 
 /* The style in which to perform a dynamic printf.  This is a user
    option because different output options have different tradeoffs;
--- a/breakpoint.h
+++ b/breakpoint.h
@@ -1212,6 +1212,7 @@ extern void tbreak_command (char *, int)
 extern struct breakpoint_ops base_breakpoint_ops;
 extern struct breakpoint_ops bkpt_breakpoint_ops;
 extern struct breakpoint_ops tracepoint_breakpoint_ops;
+extern struct breakpoint_ops dprintf_breakpoint_ops;
 
 extern void initialize_breakpoint_ops (void);
 
--- a/mi/mi-cmd-break.c
+++ b/mi/mi-cmd-break.c
@@ -99,15 +99,17 @@ mi_cmd_break_insert (char *command, char
   int pending = 0;
   int enabled = 1;
   int tracepoint = 0;
+  int dprintf = 0;
   struct cleanup *back_to;
   enum bptype type_wanted;
   struct breakpoint_ops *ops;
+  char *extra_string = NULL;
 
   enum opt
     {
       HARDWARE_OPT, TEMP_OPT, CONDITION_OPT,
       IGNORE_COUNT_OPT, THREAD_OPT, PENDING_OPT, DISABLE_OPT,
-      TRACEPOINT_OPT,
+      TRACEPOINT_OPT, DPRINTF_OPT,
     };
   static const struct mi_opt opts[] =
   {
@@ -119,6 +121,7 @@ mi_cmd_break_insert (char *command, char
     {"f", PENDING_OPT, 0},
     {"d", DISABLE_OPT, 0},
     {"a", TRACEPOINT_OPT, 0},
+    {"s", DPRINTF_OPT, 1},
     { 0, 0, 0 }
   };
 
@@ -159,9 +162,15 @@ mi_cmd_break_insert (char *command, char
 	case TRACEPOINT_OPT:
 	  tracepoint = 1;
 	  break;
+	case DPRINTF_OPT:
+	  extra_string = oarg;
+	  dprintf = 1;
+	  break;
 	}
     }
 
+  if (hardware && dprintf)
+    error (_("-break-insert: -h and -s cannot be used together"));
   if (oind >= argc)
     error (_("-break-insert: Missing <location>"));
   if (oind < argc - 1)
@@ -171,20 +180,31 @@ mi_cmd_break_insert (char *command, char
   /* Now we have what we need, let's insert the breakpoint!  */
   back_to = setup_breakpoint_reporting ();
 
-  /* Note that to request a fast tracepoint, the client uses the
-     "hardware" flag, although there's nothing of hardware related to
-     fast tracepoints -- one can implement slow tracepoints with
-     hardware breakpoints, but fast tracepoints are always software.
-     "fast" is a misnomer, actually, "jump" would be more appropriate.
-     A simulator or an emulator could conceivably implement fast
-     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;
+  if (tracepoint)
+    {
+      /* Note that to request a fast tracepoint, the client uses the
+	 "hardware" flag, although there's nothing of hardware related to
+	 fast tracepoints -- one can implement slow tracepoints with
+	 hardware breakpoints, but fast tracepoints are always software.
+	 "fast" is a misnomer, actually, "jump" would be more appropriate.
+	 A simulator or an emulator could conceivably implement fast
+	 regular non-jump based tracepoints.  */
+      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;
+    }
 
   create_breakpoint (get_current_arch (), address, condition, thread,
-		     NULL,
+		     extra_string,
 		     0 /* condition and thread are valid.  */,
 		     temp_p, type_wanted,
 		     ignore_count,

[-- Attachment #3: mi-dprintf-doc.txt --]
[-- Type: text/plain, Size: 1603 bytes --]

--- a/doc/gdb.texinfo
+++ b/doc/gdb.texinfo
@@ -28702,7 +28702,9 @@ N.A.
 @smallexample
  -break-insert [ -t ] [ -h ] [ -f ] [ -d ] [ -a ]
     [ -c @var{condition} ] [ -i @var{ignore-count} ]
-    [ -p @var{thread-id} ] [ @var{location} ]
+    [ -p @var{thread-id} ]
+    [ -s "@var{template},@var{expression}[,@var{expression}@dots{}]" ]
+    [ @var{location} ]
 @end smallexample
 
 @noindent
@@ -28742,6 +28744,29 @@ Make the breakpoint conditional on @var{
 Initialize the @var{ignore-count}.
 @item -p @var{thread-id}
 Restrict the breakpoint to the specified @var{thread-id}.
+@item -s "@var{template},@var{expression}[,@var{expression}@dots{}]"
+Set a dynamic printf breakpoint, described in @ref{Dynamic Printf}.
+The @var{template} and @var{expression} should be within double
+quotes and be escaped by being preceded with a backslash.
+
+@smallexample
+(gdb)
+4-break-insert -s "\"At foo entry\\n\"" foo
+4^done,bkpt=@{number="1",type="dprintf",disp="keep",enabled="y",
+addr="0x000000000040061b",func="foo",file="mi-dprintf.c",
+fullname="mi-dprintf.c",line="25",thread-groups=["i1"],
+times="0",script=@{"printf \"At foo entry\\n\"","continue"@},
+original-location="foo"@}
+(gdb)
+5-break-insert -s "\"arg=%d, g=%d\\n\", arg, g" 26
+5^done,bkpt=@{number="2",type="dprintf",disp="keep",enabled="y",
+addr="0x000000000040062a",func="foo",file="mi-dprintf.c",
+fullname="mi-dprintf.c",line="26",thread-groups=["i1"],
+times="0",script=@{"printf \"arg=%d, g=%d\\n\", arg, g","continue"@},
+original-location="mi-dprintf.c:26"@}
+(gdb)
+@end smallexample
+
 @end table
 
 @subsubheading Result

[-- Attachment #4: mi-dprintf-test.txt --]
[-- Type: text/plain, Size: 7179 bytes --]

--- a/testsuite/gdb.mi/Makefile.in
+++ b/testsuite/gdb.mi/Makefile.in
@@ -3,7 +3,7 @@ srcdir = @srcdir@
 
 PROGS = basics c_variable cpp_variable var-cmd dw2-ref-missing-frame	\
 	gdb669-pthreads gdb701 gdb792 mi-async mi-basics mi-break	\
-	mi-cli mi-console mi-disassemble mi-eval mi-file		\
+	mi-cli mi-console mi-disassemble mi-dprintf mi-eval mi-file	\
 	mi-file-transfer mi-non-stop mi-non-stop-exit			\
 	mi-ns-stale-regcache mi-nsintrall mi-nsmoribund mi-nsthrexec	\
 	mi-pending mi-pthreads mi-read-memory mi-regs mi-return		\
--- /dev/null
+++ b/testsuite/gdb.mi/mi-dprintf.c
@@ -0,0 +1,59 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2013 Free Software Foundation, Inc.
+   Contributed by Hui Zhu  <hui@codesourcery.com>
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+static int g;
+
+void
+foo (int arg)
+{
+  g += arg;
+  g *= 2; /* set dprintf 1 here */
+  g /= 2.5; /* set breakpoint 1 here */
+}
+
+int
+main (int argc, char *argv[])
+{
+  int loc = 1234;
+
+  /* Ensure these functions are available.  */
+  printf ("kickoff %d\n", loc);
+  fprintf (stderr, "also to stderr %d\n", loc);
+
+  foo (loc++);
+  foo (loc++);
+  foo (loc++);
+  return g;
+}
+
+/* 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
+   evaluation of this expression requires the program to have a function
+   "malloc".  */
+
+void
+bar (void)
+{
+  void *p = malloc (16);
+
+  free (p);
+}
--- /dev/null
+++ b/testsuite/gdb.mi/mi-dprintf.exp
@@ -0,0 +1,120 @@
+#   Copyright (C) 2013 Free Software Foundation, Inc.
+#   Contributed by Hui Zhu  <hui@codesourcery.com>
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+standard_testfile
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested mi-dprintf.exp
+     return -1
+}
+
+mi_delete_breakpoints
+
+set bp_location1 [gdb_get_line_number "set breakpoint 1 here"]
+set dp_location1 [gdb_get_line_number "set dprintf 1 here"]
+
+mi_run_to_main
+
+mi_gdb_test "1-break-insert -s" \
+    "1\\^error,msg=\"-break-insert: Option -s requires an argument\"" "mi insert without location"
+
+mi_gdb_test "2-break-insert -s foo" \
+    "2\\^error,msg=\"-break-insert: Missing <location>\"" "mi insert breakpoint without format string"
+
+mi_gdb_test "3-break-insert -s 29" \
+    "3\\^error,msg=\"-break-insert: Missing <location>\"" "mi insert second breakpoint without format string"
+
+mi_gdb_test "-break-insert main" ".*" "mi insert breakpoint main"
+mi_delete_breakpoints
+
+mi_gdb_test "4-break-insert -s \"\\\"At foo entry\\\\n\\\"\" foo" \
+    "4\\^done,bkpt=\{number=\".*\",type=\"dprintf\".*func=\"foo\",file=\".*mi-dprintf.c\",fullname=\".*mi-dprintf.c\",line=\".*\".*" "mi insert dprintf foo"
+
+mi_gdb_test "5-break-insert -s \"\\\"arg=%d, g=%d\\\\n\\\", arg, g\" $dp_location1" \
+    "5\\^done,bkpt=\{number=\".*\",type=\"dprintf\".*func=\"foo\",file=\".*mi-dprintf.c\",fullname=\".*mi-dprintf.c\",line=\"$dp_location1\".*" \
+    "mi insert dprintf dp_location1"
+
+mi_gdb_test "6-break-info" \
+    "6\\^done,BreakpointTable=\{nr_rows=\".\",nr_cols=\".\",hdr=\\\[\{width=\".*\",alignment=\".*\",col_name=\"number\",colhdr=\"Num\"\},\{width=\".*\",alignment=\".*\",col_name=\"type\",colhdr=\"Type\"\},\{width=\".*\",alignment=\".*\",col_name=\"disp\",colhdr=\"Disp\"\},\{width=\".*\",alignment=\".*\",col_name=\"enabled\",colhdr=\"Enb\"\},\{width=\".*\",alignment=\".*\",col_name=\"addr\",colhdr=\"Address\"\},\{width=\".*\",alignment=\".*\",col_name=\"what\",colhdr=\"What\"\}\\\],body=\\\[bkpt=\{number=\"3\",type=\"dprintf\".*func=\"foo\",file=\".*mi-dprintf.c\",fullname=\".*mi-dprintf.c\",line=\".*\".*,bkpt=\{number=\".*\",type=\"dprintf\".*func=\"foo\",file=\".*mi-dprintf.c\",fullname=\".*mi-dprintf.c\",line=\"$dp_location1\".*" \
+    "mi info dprintf"
+
+mi_gdb_test "-break-insert $bp_location1" ".*" "mi insert breakpoint bp_location1"
+
+proc mi_continue_dprintf {args} {
+    with_test_prefix $args {
+	mi_run_cmd
+	mi_gdb_test "continue" ".*At foo entry.*arg=1234, g=1234.*" "mi 1st dprintf"
+	mi_gdb_test "continue" ".*At foo entry.*arg=1235, g=2222.*" "mi 2nd dprintf"
+    }
+}
+
+mi_continue_dprintf "gdb"
+
+# The "call" style depends on having I/O functions available, so test.
+
+if ![target_info exists gdb,noinferiorio] {
+
+    # Now switch styles and rerun; in the absence of redirection the
+    # output should be the same.
+
+    mi_gdb_test "set dprintf-style call" ".*" "mi set dprintf style to call"
+    mi_continue_dprintf "call"
+
+    mi_gdb_test "set dprintf-function fprintf" ".*" "mi set dprintf-channel stderr"
+    mi_gdb_test "set dprintf-channel stderr" ".*" "mi set dprintf channel"
+    mi_continue_dprintf "fprintf"
+}
+
+# Sleep 1 second to make sure set dprintf-style agent get right outout.
+sleep 1
+set target_can_dprintf 0
+set msg "set dprintf style to agent"
+send_gdb "set dprintf-style agent\n"
+gdb_expect {
+    -re "warning: Target cannot run dprintf commands, falling back to GDB printf.*$mi_gdb_prompt$" {
+	unsupported "$msg"
+    }
+    -re "done.*$mi_gdb_prompt$" {
+	set target_can_dprintf 1
+	pass "$msg"
+    }
+    -re ".*$mi_gdb_prompt$" {
+	fail "$msg"
+    }
+    timeout {
+       fail "resume all, waiting for program exit (timeout)"
+    }
+}
+
+if $target_can_dprintf {
+    mi_run_cmd
+
+    mi_gdb_test "continue" ".*breakpoint-hit.*func=\"foo\".*" "mi 1st dprintf, agent"
+
+    mi_gdb_test "continue" ".*breakpoint-hit.*func=\"foo\".*" "mi 2nd dprintf, agent"
+
+    mi_gdb_test "6-break-info" ".*modified.*" "mi info dprintf second time"
+}
+
+mi_gdb_test "set dprintf-style foobar" ".*error.*" "mi set dprintf style to an unrecognized type"