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 06/19/13 02:09, Pedro Alves wrote:

On 06/07/2013 04:15 AM, Hui Zhu wrote:


2013-06-07  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.


Context is split in multiple lines with '()'s, not ','s:


Fixed.



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

  };

  /* Default breakpoint_ops methods.  */
@@ -13351,6 +13353,76 @@ dprintf_print_recreate (struct breakpoin
    print_recreate_thread (tp, fp);
  }

+/* Implement the "create_breakpoints_sal" breakpoint_ops method for
+   dprintf.  */
+
+static void
+dprintf_create_breakpoints_sal (struct gdbarch *gdbarch,
+				struct linespec_result *canonical,
+				struct linespec_sals *lsal,
+				char *cond_string,
+				char *extra_string,
+				enum bptype type_wanted,
+				enum bpdisp disposition,
+				int thread,
+				int task, int ignore_count,
+				const struct breakpoint_ops *ops,
+				int from_tty, int enabled,
+				int internal, unsigned flags)
+{
+  struct breakpoint *b;
+
+  create_breakpoints_sal_default (gdbarch, canonical, lsal,
+				  cond_string, extra_string,
+				  type_wanted,
+				  disposition, thread, task,
+				  ignore_count, ops, from_tty,
+				  enabled, internal, flags);
+
+  b = get_breakpoint (breakpoint_count);
+  gdb_assert (b != NULL);
+
+  breakpoint_set_silent (b, 0);
+}


When I tried it last, I didn't find making the dprintf
explicitly silent necessary, given:

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

So did it turn out really necessary for some reason?  Please
don't leave such changes between revisions unexplained.


Sorry for my misunderstand your comments.  They were removed.




+gdb_test "next" "\\+\\+x\;.*\/\* Next without dprintf.*" "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>


(
IMO, we should stop adding these (like glibc has done so too):
http://sourceware.org/ml/gdb-patches/2013-05/msg00253.html
)


Removed.




--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-non-stop.exp
@@ -0,0 +1,54 @@
+#   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/>.
+
+if [is_remote target] then {
+    unsupported "Dprintf with non-stop is not supported."


That's not exactly true.  It's supported, but testing is racy at the moment.
Write instead:

if [is_remote target] then {
    # Testing with remote/non-stop is racy at the moment.
    unsupported "Testing dprintf with remote/non-stop is not supported."
    return 0
}


Fixed.




+    return 0
+}
+
+standard_testfile
+
+if [prepare_for_testing "failed to prepare for dprintf with non-stop" \
+    ${testfile} ${srcfile} {debug}] {
+    return -1
+}
+
+gdb_test_no_output "set target-async on"
+gdb_test_no_output "set non-stop on"
+
+if ![runto main] {
+    fail "Can't run to main"
+    return -1
+}
+
+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


Hmm, this still looks racy to me, even on native targets.
"continue &" produces a gdb prompt.  gdb_test_multiple
inside has a match for the prompt:

	-re "\r\n$gdb_prompt $" {
	    if ![string match "" $message] then {
		fail "$message"
	    }
	    set result 1
	}

So if the expect happens to read

continue &
(gdb)

from the buffer, we'll hit the fail.  Doesn't the read1 hack catch this?


EXPECT=../contrib/expect-read1.sh make check RUNTESTFLAGS="dprintf-non-stop.exp" is OK in my part.



We need to consume the prompt after that "continue&".


+    }
+}
+
+set test "inferior stopped"
+gdb_test_multiple "" $test {
+    -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n" {
+	pass $test
+    }
+}


Likewise?



I just copy this part of code from "gdb.base/async-shell.exp".
Could you give me some help about how to consume the prompt after that "continue&"?

Thanks,
Hui

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]