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"


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.

>> > 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.

>> >
>>> >> +  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:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 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.

> 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"


> --- /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.

> +  return 0;

> +}
> \ No newline at end of file

Please watch out for these, and add a newline.

> +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.

-- 
Pedro Alves


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