Re: [RFC] PR 15075 dprintf interferes with "next"

[Hui Zhu <teawater at gmail dot com>]

Ping http://sourceware.org/ml/gdb-patches/2013-05/msg00958.html

Thanks,
Hui

On Tue, May 28, 2013 at 8:01 AM, Hui Zhu <teawater@gmail.com> wrote:
> Hi Pedro and Tom,
>
> Thanks for your review.
>
> On Wed, May 22, 2013 at 8:46 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 05/22/2013 11:21 AM, Hui Zhu wrote:
>>
>>> I tried it on fedora and looks it works OK most of times.  I only got
>>> trouble is when run this test with remote, [runto main] with
>>> "non-stop" is open will fail sometime.
>>
>> Sorry, but "most of the times" is not acceptable.  Can you please
>> figure out why that is so?  This should never fail.
>
> I found a issue about remote test and non-stop.  Because it doesn't
> have a lot of relation with dprintf, I will post a separate patch for
> that later.
>
>>
>>>> > Not sure even that is necessary, given bs->stop==0 and:
>>>> >
>>>> >       /* Print nothing for this entry if we don't stop or don't
>>>> >          print.  */
>>>> >       if (!bs->stop || !bs->print)
>>>> >         bs->print_it = print_it_noop;
>>> I am not sure about this part.
>>> If my understanding about Tom's review in before is right, the action
>>> of dprintf command should be handled when GDB check the condition.
>>> And after that, dprintf not need be handled.
>>
>> Yes, but I don't see how that counters my suggestion.
>
> Added dprintf_create_breakpoints_sal. When dprintf is created, auto
> set silent to 0.
>
>>
>>>> >
>>>>> >> +  bpstat_do_actions_1 (&bs);
>>>>> >> +  bs->next = tmp;
>>>>> >> +}
>>>> >
>>>> > Could you add some comments explaining what this is
>>>> > doing, and why?
>>> I update this function to following part:
>>> /* Implement the "after_cond" breakpoint_ops method for dprintf.  */
>>>
>>> static void
>>> dprintf_after_cond (struct bpstats *bs)
>>
>>
>>> {
>>>   struct cleanup *old_chain;
>>>   struct breakpoint *b = bs->breakpoint_at;
>>>
>>>   bs->commands = b->commands;
>>>   incref_counted_command_line (bs->commands);
>>
>> I still think there's no point in moving this to the
>> after_condition_true hook, if both existing implementations
>> end up doing it themselves.  See below.
>>
>>>
>>>   /* Because function bpstat_do_actions_1 will execute all the command
>>>      list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to
>>>      NULL.  */
>>>   old_chain = make_cleanup_restore_ptr ((void **)&bs->next);
>>
>> That cast is invalid actually.  Older gcc's will complain about
>> the aliasing violation.  I think we can get away without this, by
>> passing a separate list with only one entry to bpstat_do_actions_1.
>>
>>>   bs->next = NULL;
>>>   bpstat_do_actions_1 (&bs);
>>>   do_cleanups (old_chain);
>>>
>>>   /* This dprintf need not be handled after this function
>>>      because its command list is executed by bpstat_do_actions_1.
>>>      Clear STOP and PRINT for that.  */
>>>   bs->stop = 0;
>>>   bs->print = 0;
>>
>> Thanks.  I was more looking for general design comments on why we
>> run the command list here.  This also should explain why clear stop
>> here, instead of on the more natural check_status.
>>
>> So all in all, if it works for you (seems to work fine for me), I'd
>> rather do as this patch on top of yours does:
>>
>
> Thanks for this patch.  I merged it to dprintf-continue patch.
>
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>  gdb/breakpoint.c |   62 ++++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 37 insertions(+), 25 deletions(-)
>>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index 7ef5703..181aecc 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -5292,6 +5292,15 @@ bpstat_stop_status (struct address_space *aspace,
>>                     b->enable_state = bp_disabled;
>>                   removed_any = 1;
>>                 }
>> +
>> +             if (b->silent)
>> +               bs->print = 0;
>> +             bs->commands = b->commands;
>> +             incref_counted_command_line (bs->commands);
>> +             if (command_line_is_silent (bs->commands
>> +                                         ? bs->commands->commands : NULL))
>> +               bs->print = 0;
>> +
>>               b->ops->after_condition_true (bs);
>>             }
>>
>> @@ -12742,15 +12751,7 @@ base_breakpoint_explains_signal (struct breakpoint *b)
>>  static void
>>  base_breakpoint_after_condition_true (struct bpstats *bs)
>>  {
>> -  struct breakpoint *b = bs->breakpoint_at;
>> -
>> -  if (b->silent)
>> -    bs->print = 0;
>> -  bs->commands = b->commands;
>> -  incref_counted_command_line (bs->commands);
>> -  if (command_line_is_silent (bs->commands
>> -                             ? bs->commands->commands : NULL))
>> -    bs->print = 0;
>> +  /* Nothing to do.   */
>>  }
>>
>>  struct breakpoint_ops base_breakpoint_ops =
>> @@ -13368,30 +13369,41 @@ dprintf_print_recreate (struct breakpoint *tp, struct ui_file *fp)
>>  }
>>
>>  /* Implement the "after_condition_true" breakpoint_ops method for
>> -   dprintf.  */
>> +   dprintf.
>> +
>> +   dprintf's are implemented with regular commands in their command
>> +   list, but we run the commands here instead of before presenting the
>> +   stop to the user, as dprintf's don't actually cause a stop.  This
>> +   also makes it so that the commands of multiple dprintfs at the same
>> +   address are all handled.  */
>>
>>  static void
>>  dprintf_after_condition_true (struct bpstats *bs)
>>  {
>>    struct cleanup *old_chain;
>> -  struct breakpoint *b = bs->breakpoint_at;
>> +  struct bpstats tmp_bs = { NULL };
>> +  struct bpstats *tmp_bs_p = &tmp_bs;
>>
>> -  bs->commands = b->commands;
>> -  incref_counted_command_line (bs->commands);
>> +  /* dprintf's never cause a stop.  This wasn't set in the
>> +     check_status hook instead because that would make the dprintf's
>> +     condition not be evaluated.  */
>> +  bs->stop = 0;
>>
>> -  /* Because function bpstat_do_actions_1 will execute all the command
>> -     list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to
>> -     NULL.  */
>> -  old_chain = make_cleanup_restore_ptr ((void **)&bs->next);
>> -  bs->next = NULL;
>> -  bpstat_do_actions_1 (&bs);
>> -  do_cleanups (old_chain);
>> +  /* Run the command list here.  Take ownership of it instead of
>> +     copying.  We never want these commands to run later in
>> +     bpstat_do_actions, if a breakpoint that causes a stop happens to
>> +     be set at same address as this dprintf, or even if running the
>> +     commands here throws.  */
>> +  tmp_bs.commands = bs->commands;
>> +  bs->commands = NULL;
>> +  old_chain = make_cleanup_decref_counted_command_line (&tmp_bs.commands);
>>
>> -  /* This dprintf need not be handled after this function
>> -     because its command list is executed by bpstat_do_actions_1.
>> -     Clear STOP and PRINT for that.  */
>> -  bs->stop = 0;
>> -  bs->print = 0;
>> +  bpstat_do_actions_1 (&tmp_bs_p);
>> +
>> +  /* 'tmp_bs.commands' will usually be NULL by now, but
>> +     bpstat_do_actions_1 may return early without processing the whole
>> +     list.  */
>> +  do_cleanups (old_chain);
>>  }
>>
>>  /* The breakpoint_ops structure to be used on static tracepoints with
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>>>> >> +if [prepare_for_testing $expfile $executable $srcfile {debug}] {
>>>>> >> +    untested "failed to prepare for trace tests"
>>>> >
>>>> > This is not a trace test.  Actually, prepare_for_testing
>>>> > already calls untested with whatever we pass it as first argument.
>>>> > I think it'd be the first such call in the testsuite, but I
>>>> > think this would be better:
>>>> >
>>>> > if [prepare_for_testing "failed to prepare for trace tests" \
>>>> >     $executable $srcfile {debug}] {
>>>> >    return -1
>>>> > }
>>> After I use this part of code, I got:
>>> ERROR: tcl error sourcing ../../../src/gdb/testsuite/gdb.base/dprintf-next.exp.
>>> ERROR: wrong # args: should be "prepare_for_testing testname
>>> executable ?sources? ?options?"
>>>     while executing
>>> "prepare_for_testing "failed to prepare for dprintf next tests""
>>>     invoked from within
>>> "if [prepare_for_testing "failed to prepare for dprintf next tests"
>>>     $executable $srcfile {debug}] {
>>>    return -1
>>> }"
>>
>> Hmm?  Oh, it looks like you forgot the '\' at the end of the line.
>> Please try again.
>
> Fixed both dprintf-next.exp and dprintf-non-stop.exp.
>
>>
>>> So I change this part to:
>>> if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } {
>>
>> 'dprintf-next.exp' really isn't magical.  It's just a string.  :-)
>>
>>>     return -1
>>> }
>>
>>>>> >> +gdb_test "next" ".*" "next 1"
>>>>> >> +gdb_test "next" ".*" "next 2"
>>>> >
>>>> > Please use more string regexes for these, that confirm the next stopped
>>>> > at the right lines.
>>> Fixed.
>>
>> Sorry, I should have been clearer.  Don't do that by checking
>> line numbers though, as those change when more functions are
>> added to the test.  Just make each line look different instead.
>>
>>> +
>>> +gdb_test "dprintf $dp_location, \"%d\\n\", x" \
>>> +    "Dprintf .*"
>>
>> Please add an explicit 3rd parameter to gdb_test, so that
>> the line number doesn't appear in gdb.sum.
>>
>>> +
>>> +gdb_test "next" "23.*\\+\\+x.*" "next 1"
>>> +gdb_test "next" "24.*\\+\\+x.*" "next 2"
>>
>> Do something like this:
>>
>> gdb_test "next" "\\+\\+x;" "next 1"
>> gdb_test "next" "\\+\\+x; /* set dprintf here" "next 2"
>>
>> You could add a comment to the first stepped line to
>> make that clearer:
>>
>> gdb_test "next" "\\+\\+x; /* step here.*" "next 1"
>> gdb_test "next" "\\+\\+x; /* set dprintf here" "next 2"
>
> This part is fixed.
>
>>
>>
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.base/dprintf-non-stop.c
>>> @@ -0,0 +1,30 @@
>>> +/* 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/>.  */
>>> +
>>> +void
>>> +foo ()
>>> +{
>>> +}
>>> +
>>> +int
>>> +main ()
>>> +{
>>> +  foo ();
>>> +  while (1);
>>
>> If something goes wrong, like GDB crashing, this could leave the test
>> running forever, pegging the CPU.  Just make it sleep for some
>> time instead.
>
> Changed this part to sleep(3)
>
>>
>>> +  return 0;
>>
>>> +}
>>> \ No newline at end of file
>>
>> Please watch out for these, and add a newline.
>>
>
> Fixed.
>
>>> +gdb_test "dprintf foo,\"At foo entry\\n\"" "Dprintf .*"
>>> +
>>> +send_gdb "continue &\n"
>>> +exec sleep 1
>>> +set test "interrupt"
>>> +gdb_test_multiple $test $test {
>>> +    -re "interrupt\r\n$gdb_prompt " {
>>> +     pass $test
>>> +    }
>>> +}
>>> +
>>> +set test "inferior stopped"
>>> +gdb_test_multiple "" $test {
>>> +    -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n" {
>>> +     pass $test
>>> +    }
>>> +}
>>
>> This leaves the prompt in the expect buffer.  I think
>> this is likely to confuse the following test that runs.
>>
>
> After change this part to:
> gdb_test_multiple "" $test {
>     -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n$gdb_prompt" {
>         pass $test
>     }
> }
> I got:
> Running ../../../src/gdb/testsuite/gdb.base/dprintf-non-stop.exp ...
> FAIL: gdb.base/dprintf-non-stop.exp: inferior stopped (timeout)
>
> I thought the reason is:
> (gdb) PASS: gdb.base/dprintf-non-stop.exp: interrupt
>
> [process 24118] #1 stopped.
> 0x00002aaaaad8f830 in nanosleep () from /lib/x86_64-linux-gnu/libc.so.6
> FAIL: gdb.base/dprintf-non-stop.exp: inferior stopped (timeout)
>
> There is not $gdb_prompt.
>
>
> On Wed, May 22, 2013 at 10:04 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>>
>> Hui> So I change this part to:
>> Hui> if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } {
>> Hui>     return -1
>> Hui> }
>>
>> I think it is better to use the variables created by standard_testfile.
>
> Fixed.
>
> The attachments are the new patches.
>
> Best,
> Hui
>
> 2013-05-28  Yao Qi  <yao@codesourcery.com>
>             Hui Zhu  <hui@codesourcery.com>
>             Pedro Alves  <palves@redhat.com>
>
>         PR breakpoints/15075
>         PR breakpoints/15434
>         * breakpoint.c (bpstat_stop_status): Call
>         b->ops->after_condition_true.
>         (update_dprintf_command_list): Don't append "continue" command
>         to the command list of dprintf breakpoint.
>         (base_breakpoint_after_condition_true): New function.
>         (base_breakpoint_ops): Add base_breakpoint_after_condition_true.
>         (dprintf_create_breakpoints_sal,
>         dprintf_after_condition_true): New functions.
>         (initialize_breakpoint_ops): Set dprintf_create_breakpoints_sal
>         and dprintf_after_condition_true.
>         * breakpoint.h (breakpoint_ops): Add after_condition_true.
>
> 2013-05-28  Yao Qi  <yao@codesourcery.com>
>             Hui Zhu  <hui@codesourcery.com>
>
>         PR breakpoints/15075
>         PR breakpoints/15434
>         * gdb.base/dprintf-next.c: New file.
>         * gdb.base/dprintf-next.exp: New file.
>         * gdb.base/dprintf-non-stop.c: New file.
>         * gdb.base/dprintf-non-stop.exp: New file.
>         * gdb.base/dprintf.exp: Don't check "continue" in the output
>         of "info breakpoints".
>         * gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
>         Don't check "continue" in script field.