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


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

> 	(base_breakpoint_after_cond): New.
> 	(base_breakpoint_ops): Add base_breakpoint_after_cond.
> 	(dprintf_after_cond): New.

Say "New function."

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

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

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

> +}
> +


> +/* Implement the "after_cond"  breakpoint_ops method for dprintf.  */

You used "" here.  :-)  Spurious double space before breakpoint_ops.

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

> +  bs->commands = b->commands;
> +  tmp = bs->next;
> +  bs->next = tmp;

A = B;
B = A;

?

Was this supposed to be:

A = B;
B = NULL;

?

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

> +  bpstat_do_actions_1 (&bs);
> +  bs->next = tmp;
> +}

Could you add some comments explaining what this is
doing, and why?

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

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

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

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

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

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

What's this shell echo for?

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

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

> + {
> +   int x = 5;

Wrong indentation.

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

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

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

-- 
Pedro Alves


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