This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Pedro,

Thanks for your review.

On Sat, May 18, 2013 at 5:01 AM, Pedro Alves <palves@redhat.com> wrote:
> On 05/16/2013 08:28 AM, Hui Zhu wrote:
>> Hi,
>>
>> Because this patch also can handle bug 15434, I updated the test
>> patches to add test dprintf with non-stop mode.
>
> Looks like the test is racy.  :-/  I got:
>
> At foo entry
> arg=1235, g=2222
> iAt foo entry
> ntarg=1236, g=3013
> errupt
> (gdb) FAIL: gdb.base/dprintf.exp: interrupt
>
> This will need to be sorted out.
>
> I also saw:
>
> (gdb) At foo entry
> arg=1235, g=2222
> At foo entry
> arg=1236, g=3013
> [Inferior 1 (process 30651) exited with code 0152]
> FAIL: gdb.base/dprintf.exp: shell echo foo (timeout)
> interrupt
> ../../src/gdb/thread.c:694: internal-error: set_stop_requested: Assertion `tp' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) FAIL: gdb.base/dprintf.exp: interrupt (GDB internal error)
> Resyncing due to internal error.
> n
> ../../src/gdb/thread.c:694: internal-error: set_stop_requested: Assertion `tp' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Create a core file of GDB? (y or n) n
> (gdb) FAIL: gdb.base/dprintf.exp: process stopped (timeout)
> testcase ../../../src/gdb/testsuite/gdb.base/dprintf.exp completed in 20 seconds
>
> :-(  Though that's likely unrelated.

I updated this part and put it to dpintf-non-stop.exp.
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.

>
>
>> 2013-05-16  Yao Qi  <yao@codesourcery.com>
>>           Hui Zhu  <hui@codesourcery.com>
>>
>>       PR breakpoints/15075
>>       PR breakpoints/15434
>>       * breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
>>       (update_dprintf_command_list): Don't append "continue" command
>>       to the command of dprintf breakpoint.
>
> "to the command list."

Fixed.

>
>>       (base_breakpoint_after_cond): New.
>>       (base_breakpoint_ops): Add base_breakpoint_after_cond.
>>       (dprintf_after_cond): New.
>
> Say "New function."

Fixed.

>
>>       (initialize_breakpoint_ops): Set dprintf_after_cond.
>>       * breakpoint.h (breakpoint_ops): Add after_cond.
>>
>> 2013-05-16  Yao Qi  <yao@codesourcery.com>
>>           Hui Zhu  <hui@codesourcery.com>
>>
>>       PR breakpoints/15075
>>       PR breakpoints/15434
>>       * gdb.base/dprintf.exp: Don't check "continue" in the output
>>       of "info breakpoints".
>>       Add test dprintf with non-stop mode.
>
>         Add test for dprintf in non-stop mode.
>
>>       * gdb.base/dprintf.c: Add a sleep for test dprintf with non-stop
>
> for testing.

Fixed.

>
>>       mode.
>>       * gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
>>       Don't check "continue" in script field.
>>       * gdb.base/pr15075.c: New.
>>       * gdb.base/pr15075.exp: New.
>>
>
>
>
>>
>>
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -5283,13 +5283,7 @@ bpstat_stop_status (struct address_space
>>                   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_cond (bs);
>>           }
>>
>>       }
>> @@ -8939,25 +8933,16 @@ update_dprintf_command_list (struct brea
>>                   _("Invalid dprintf style."));
>>
>>    gdb_assert (printf_line != NULL);
>> -  /* Manufacture a printf/continue sequence.  */
>> +  /* Manufacture a printf sequence.  */
>>    {
>> -    struct command_line *printf_cmd_line, *cont_cmd_line = NULL;
>> -
>> -    if (strcmp (dprintf_style, dprintf_style_agent) != 0)
>> -      {
>> -     cont_cmd_line = xmalloc (sizeof (struct command_line));
>> -     cont_cmd_line->control_type = simple_control;
>> -     cont_cmd_line->body_count = 0;
>> -     cont_cmd_line->body_list = NULL;
>> -     cont_cmd_line->next = NULL;
>> -     cont_cmd_line->line = xstrdup ("continue");
>> -      }
>> +    struct command_line *printf_cmd_line
>> +      = xmalloc (sizeof (struct command_line));
>>
>>      printf_cmd_line = xmalloc (sizeof (struct command_line));
>>      printf_cmd_line->control_type = simple_control;
>>      printf_cmd_line->body_count = 0;
>>      printf_cmd_line->body_list = NULL;
>> -    printf_cmd_line->next = cont_cmd_line;
>> +    printf_cmd_line->next = NULL;
>>      printf_cmd_line->line = printf_line;
>>
>>      breakpoint_set_commands (b, printf_cmd_line);
>> @@ -12743,6 +12728,22 @@ base_breakpoint_explains_signal (struct
>>    return BPSTAT_SIGNAL_HIDE;
>>  }
>>
>
>
>
>> +/* The default 'after_cond' method.  */
>
> You used '' here.

Fixed.

>
>> +
>> +static void
>> +base_breakpoint_after_cond (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;
>
> Indentation is wrong here.
>
> I think we can keep all these where it was.

Fixed.

>
>> +}
>> +
>
>
>> +/* Implement the "after_cond"  breakpoint_ops method for dprintf.  */
>
> You used "" here.  :-)  Spurious double space before breakpoint_ops.

Fixed.  :)

>
>> +
>> +static void
>> +dprintf_after_cond (struct bpstats *bs)
>> +{
>> +  bpstat tmp;
>> +  struct breakpoint *b = bs->breakpoint_at;
>> +
>> +  bs->stop = 0;
>> +  bs->print = 0;
>
> If we make dprintf's silent (b->silent = 0), then bs->print will
> end up 0 for them too, given the:
>
>   if (b->silent)
>     bs->print = 0;
>
> bit.
>
> 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.

So dprintf_after_cond just clear the bs->stop and bs->print because
dprintf not need be handled after this function.
And call command of this dprintf because the its condition is true.

I added some comments for that.

>
>> +  bs->commands = b->commands;
>> +  tmp = bs->next;
>> +  bs->next = tmp;
>
> A = B;
> B = A;
>
> ?
>
> Was this supposed to be:
>
> A = B;
> B = NULL;
>
> ?

I think this "bs->next = tmp;" is duplicate line after
bpstat_do_actions_1.  Fixed.

>
>> +  incref_counted_command_line (bs->commands);
>
> If copying all this code here really is necessary, then keep the
> incref close to the commands copy, like in the original code.

Fixed.

>
>> +  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);

  /* 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);

  /* 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;
}

>
>> +
>>  /* The breakpoint_ops structure to be used on static tracepoints with
>>     markers (`-m').  */
>>
>> @@ -15852,6 +15872,7 @@ initialize_breakpoint_ops (void)
>>    ops->print_it = bkpt_print_it;
>>    ops->print_mention = bkpt_print_mention;
>>    ops->print_recreate = dprintf_print_recreate;
>> +  ops->after_cond = dprintf_after_cond;
>>  }
>>
>>  /* Chain containing all defined "enable breakpoint" subcommands.  */
>> --- a/gdb/breakpoint.h
>> +++ b/gdb/breakpoint.h
>> @@ -614,6 +614,9 @@ struct breakpoint_ops
>>       'catch signal' interact properly with 'handle'; see
>>       bpstat_explains_signal.  */
>>    enum bpstat_signal_value (*explains_signal) (struct breakpoint *);
>> +
>> +  /* Do some setup after check condition is true.  */
>
>    /* Called after evaluating the breakpoint's condition,
>       and only if it evaluated true.  */

Fixed.

>
>> +  void (*after_cond) (struct bpstats *bs);
>
> I'd rather spell out "cond".  I suggest:
>
>   void (*after_condition_true) (...);
>
> To be a bit more self descriptive.
>

Fixed.

>>  };
>>
>>  /* Helper for breakpoint_ops->print_recreate implementations.  Prints
>>
>>
>> dprintf-continue-test.txt
>>
>>
>> --- a/gdb/testsuite/gdb.base/dprintf.c
>> +++ b/gdb/testsuite/gdb.base/dprintf.c
>> @@ -39,6 +39,9 @@ main (int argc, char *argv[])
>>    foo (loc++);
>>    foo (loc++);
>>    foo (loc++);
>> +
>> +  sleep (3);
>> +
>>    return g;
>>  }
>>
>> --- a/gdb/testsuite/gdb.base/dprintf.exp
>> +++ b/gdb/testsuite/gdb.base/dprintf.exp
>> @@ -50,10 +50,8 @@ gdb_test_sequence "info breakpoints" "dp
>>      "\[\r\n\]2       breakpoint"
>>      "\[\r\n\]3       dprintf"
>>      "\[\r\n\]        printf \"At foo entry\\\\n\""
>> -    "\[\r\n\]        continue"
>>      "\[\r\n\]4       dprintf"
>>      "\[\r\n\]        printf \"arg=%d, g=%d\\\\n\", arg, g"
>> -    "\[\r\n\]        continue"
>>  }
>>
>>  gdb_test "break $bp_location1" \
>> @@ -136,3 +134,41 @@ if $target_can_dprintf {
>>  gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \
>>      "Set dprintf style to an unrecognized type"
>>
>> +
>> +# Test dprintf with non-stop mode.
>> +# The testfile uses "run".  The real bug happened only for ![is_remote target].
>
> It doesn't need to use "run", AFAICS.  You can run to
> main as usual, and then use "c&" to get the same effect,
> and then the test should pass with GDBserver too.
>
> There are targets that don't support non-stop,
> and, the call/gdb dprintf styles works on all targets.
> The test should be gracefully skipped on those targets.
> It might be better to split this to a separate test file.

Add dprintf-non-stop.exp and dprintf-non-stop.c for it.

>
>> +if [target_info exists use_gdb_stub] {
>> +    unsupported "dprintf with non-stop mode"
>> +    return 0
>> +}
>> +gdb_test_no_output "delete 2 5"
>> +send_gdb "kill\n"
>> +gdb_expect 120 {
>> +    -re "Kill the program being debugged. .y or n. $" {
>> +     send_gdb "y\n"
>> +     verbose "\t\tKilling previous program being debugged"
>> +     exp_continue
>> +    }
>> +    -re "$gdb_prompt $" {
>> +     # OK.
>> +    }
>> +}
>
> Why do this manually?

This part was removed.

>
>> +gdb_test_no_output "set dprintf-style gdb" "Set dprintf style to gdb"
>> +gdb_test_no_output "set target-async on"
>> +gdb_test_no_output "set non-stop on"
>> +set gdbindex_warning_re "warning: Skipping \[^\r\n\]+ \\.gdb_index section \[^\r\n\]*\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\."
>
> I don't under why do we need to explicitly check for this warning?

Removed too.

>
>> +gdb_test "run &" "Starting program: \[^\r\n\]*(\r\n$gdbindex_warning_re)?"
>> +gdb_test "shell echo foo" "foo"
>
> What's this shell echo for?

This part copy from other test of non-stop.  It let test wait some
time then dprintf execution can complete.  Now I changed it to exec
sleep 1.

>
>> +set test "interrupt"
>> +gdb_test_multiple $test $test {
>> +    -re "interrupt\r\n$gdb_prompt " {
>> +     pass $test
>> +    }
>> +}
>> +set test "process stopped"
>> +gdb_test_multiple "" $test {
>> +    -re "\r\n\\\[process \[0-9\]+\\\] #1 stopped\\\.\r\n" {
>> +     pass $test
>> +    }
>> +}
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/pr15075.c
>
> We prefer that test files are named for something that suggests what they
> contain, rather than bug numbers.  Please call it something
> like dprintf-next.exp.

Fixed.

>
>> @@ -0,0 +1,26 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright (C) 2013 Free Software Foundation, Inc.
>> +
>> +   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/>.  */
>> +
>> +int
>> +main(void)
>
> Missing space.

Fixed.

>
>> + {
>> +   int x = 5;
>
> Wrong indentation.

Fixed.

>
>> +
>> +   ++x;
>> +   ++x; /* set dprintf here */
>> +   return x - 7;
>> + }
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/pr15075.exp
>> @@ -0,0 +1,38 @@
>> +# Copyright 2013 Free Software Foundation, Inc.
>> +# 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/>.
>> +
>> +standard_testfile
>> +
>> +set executable $testfile
>> +set expfile $testfile.exp
>> +
>> +set dp_location [gdb_get_line_number "set dprintf here"]
>> +
>> +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
}"


So I change this part to:
if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } {
    return -1
}

>
>> +
>> +if ![runto_main] {
>> +    fail "Can't run to main"
>> +    return -1
>> +}
>> +
>> +gdb_test "dprintf $dp_location, \"%d\\n\", x" \
>> +    "Dprintf .*"
>> +
>> +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.

>
>> +# Test inferior doesn't exit.
>
> Check that the inferior didn't exit.
>
>> +gdb_test "p x" ".* = 6"
>
> With the stricter regexes, I don't think this test would be
> necessary though.

Removed.

Post new patches according to your comments.

Best,
Hui

2013-05-22  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>

	PR breakpoints/15075
	PR breakpoints/15434
	* utils.c (restore_ptr_closure): New struct.
	(restore_ptr, make_cleanup_restore_ptr): New functions.
	* utils.h (make_cleanup_restore_ptr): New extern.
	* 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_after_condition_true): New function.
	(initialize_breakpoint_ops): Set dprintf_after_condition_true.
	* breakpoint.h (breakpoint_ops): Add after_condition_true.

2013-05-22  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.

Attachment: clean_ptr.txt
Description: Text document

Attachment: dprintf-continue.txt
Description: Text document

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]