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

Hui Zhu
2013-04-12T10:39:31Z

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

Hi Pedro,

Thanks for your review.

On Thu, Apr 11, 2013 at 5:14 PM, Pedro Alves <palves@redhat.com> wrote:
> On 04/11/2013 03:40 AM, Hui Zhu wrote:
>
>>> New MI features need a NEWS entry.
>>
>> This is for NEWS:
>> ** The -s of MI command -break-insert can set a dynamic printf.
>>
>
> Please send that as a real patch, so e.g., we can see what section of
> NEWS you're proposing adding it to.

OK. I add this change to doc patch.

>
>>
>>>
>>>> +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.
>
> I still see it in the patch. But, you can simplify and
> replace that gdb_compile call with a build_executable call, I think?
> Then you don't need the untested call.

Fixed.

>
>>>
>>>> + 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?
>
> Typo -> pasto. Pasto is like a typo, but refers to
> blindly copy/pasting text from elsewhere.
>
> "resume all, waiting for program exit" makes no sense here.
> It should be $msg, no?

Fixed.

>
>>> 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.
>
> I still get:
>
> $ make check gdbserver RUNTESTFLAGS="--target_board=native-gdbserver mi-dprintf.exp"

This part is really odd.
In my part, without "sleep 1" will random get fail with "Set dprintf
style to agent ".
The reason of fail is test try to check the output before it call
send_gdb "set dprintf-style agent\n".
This is why I add a "sleep 1" for it.

But looks it still not OK in your part, so I change it to:
mi_gdb_test "pwd" ".*"

If it is still not OK in your part, I suggest remove this part of test
because it is not very important for this test. The "set
dprintf-style agent" is tested in "dprintf.exp".

> ...
> FAIL: gdb.mi/mi-dprintf.exp: set dprintf style to agent
>
> === gdb Summary ===
>
> # of expected passes 13
> # of unexpected failures 1
>
> gdb.log:
>
> ~"arg=1235, g=2222\n"
> *running,thread-id="1"
> =breakpoint-modified,bkpt={number="5",type="breakpoint",disp="keep",enabled="y",addr="0x00000000004006c2",func="foo",file="../../../src/gdb/testsuite/gdb.mi/mi-dprintf.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-dprintf.c",line="29",thread-groups=["i1"],times="2",original-location="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-dprintf.c:29"}
> *stopped,reason="breakpoint-hit",disp="keep",bkptno="5",frame={addr="0x00000000004006c2",func="foo",args=[{name="arg",value="1235"}],file="../../../src/gdb/testsuite/gdb.mi/mi-dprintf.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-dprintf.c",line="29"},thread-id="1",stopped-threads="all",core="1"
> (gdb)
> PASS: gdb.mi/mi-dprintf.exp: gdb: mi 2nd dprintf
> &"continue\n"
> ~"Continuing.\n"
> ^running
> *running,thread-id="1"
> (gdb)
> =breakpoint-modified,bkpt={number="3",type="dprintf",disp="keep",enabled="y",addr="0x00000000004006a3",func="foo",file="../../../src/gdb/testsuite/gdb.mi/mi-dprintf.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-dprintf.c",line="27",thread-groups=["i1"],times="3",script={"printf \\"At foo entry\\\\n\\"","continue"},original-location="foo"}
> *stopped
> ~"At foo entry\n"
> *running,thread-id="1"
> =breakpoint-modified,bkpt={number="4",type="dprintf",disp="keep",enabled="y",addr="0x00000000004006b4",func="foo",file="../../../src/gdb/testsuite/gdb.mi/mi-dprintf.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-dprintf.c",line="28",thread-groups=["i1"],times="3",script={"printf \\"arg=%d, g=%d\\\\n\\", arg, g","continue"},original-location="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-dprintf.c:28"}
> *stopped
> ~"arg=1236, g=3013\n"
> *running,thread-id="1"
> =breakpoint-modified,bkpt={number="5",type="breakpoint",disp="keep",enabled="y",addr="0x00000000004006c2",func="foo",file="../../../src/gdb/testsuite/gdb.mi/mi-dprintf.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-dprintf.c",line="29",thread-groups=["i1"],times="3",original-location="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-dprintf.c:29"}
> *stopped,reason="breakpoint-hit",disp="keep",bkptno="5",frame={addr="0x00000000004006c2",func="foo",args=[{name="arg",value="1236"}],file="../../../src/gdb/testsuite/gdb.mi/mi-dprintf.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-dprintf.c",line="29"},thread-id="1",stopped-threads="all",core="0"
> (gdb)
> FAIL: gdb.mi/mi-dprintf.exp: set dprintf style to agent
>
>
>> +# Sleep 1 second to make sure set dprintf-style agent get right outout.
>
> s/outout/output/ ?

Fixed.

>
> --
> Pedro Alves
>

Thanks,
Hui

2013-04-12 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-12 Hui Zhu <hui@codesourcery.com>

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

2013-04-12 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: 1951 bytes --]

--- a/NEWS
+++ b/NEWS
@@ -27,6 +27,9 @@ show remote trace-status-packet
** The -trace-save MI command can optionally save trace buffer in Common
Trace Format now.

+ ** The new option -s of the MI command -break-insert sets a dynamic
+ printf breakpoint.
+
*** Changes in GDB 7.6

* Target record has been renamed to record-full.
--- a/doc/gdb.texinfo
+++ b/doc/gdb.texinfo
@@ -28745,7 +28745,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
@@ -28785,6 +28787,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: 7126 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,121 @@
+# 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 {[build_executable $testfile.exp $testfile $srcfile {debug}] == -1} {
+ untested "failed to compile $testfile"
+ 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"
+}
+
+# To make sure set dprintf-style agent get right output.
+mi_gdb_test "pwd" ".*"
+
+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 "$msg"
+ }
+}
+
+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"