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 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: mi-dprintf.txt
Description: Text document

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

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