public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2 2/3] Tell update_global_location_list to insert breakpoints
  2014-09-17 22:47 [PATCH v2 0/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto" Pedro Alves
@ 2014-09-17 22:47 ` Pedro Alves
  2014-09-17 22:47 ` [PATCH v2 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto" Pedro Alves
  2014-09-17 22:47 ` [PATCH v2 1/3] Change parameter type of update_global_location_list from boolean to enum Pedro Alves
  2 siblings, 0 replies; 12+ messages in thread
From: Pedro Alves @ 2014-09-17 22:47 UTC (permalink / raw)
  To: gdb-patches

This adds a new mode for update_global_location_list, that allows
callers saying "please insert breakpoints, even if
breakpoints_always_inserted_mode() is false".  This allows removing a
couple breakpoints_always_inserted_mode checks.

gdb/
2014-09-17  Pedro Alves  <palves@redhat.com>

	* breakpoint.c (enum ugll_insert_mode): Add UGLL_INSERT.
	(insert_breakpoints): Don't call insert_breakpoint_locations here.
	Instead, pass UGLL_INSERT to update_global_location_list.
	(update_global_location_list): Change parameter type from boolean
	to enum ugll_insert_mode.  All callers adjusted.  Adjust to use
	breakpoints_should_be_inserted_now and handle UGLL_INSERT.
	(create_solib_event_breakpoint_1): New, factored out from ...
	(create_solib_event_breakpoint): ... this.
	(create_and_insert_solib_event_breakpoint): Use
	create_solib_event_breakpoint_1 instead of calling
	insert_breakpoint_locations manually.
	(update_global_location_list): Handle UGLL_INSERT.
---
 gdb/breakpoint.c | 56 +++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 46cd079..3f372de 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -260,7 +260,11 @@ enum ugll_insert_mode
 
   /* May insert breakpoints if breakpoints_always_inserted_mode is
      true.  */
-  UGLL_MAY_INSERT
+  UGLL_MAY_INSERT,
+
+  /* Insert locations now, even if breakpoints_always_inserted_mode is
+     false.  */
+  UGLL_INSERT
 };
 
 static void update_global_location_list (enum ugll_insert_mode);
@@ -2954,13 +2958,10 @@ insert_breakpoints (void)
 	update_watchpoint (w, 0 /* don't reparse.  */);
       }
 
-  update_global_location_list (UGLL_MAY_INSERT);
-
-  /* update_global_location_list does not insert breakpoints when
-     always_inserted_mode is not enabled.  Explicitly insert them
-     now.  */
-  if (!breakpoints_always_inserted_mode ())
-    insert_breakpoint_locations ();
+  /* Updating watchpoints creates new locations, so update the global
+     location list.  Explicitly tell ugll to insert locations and
+     ignore breakpoints_always_inserted_mode.  */
+  update_global_location_list (UGLL_INSERT);
 }
 
 /* Invoke CALLBACK for each of bp_location.  */
@@ -7722,17 +7723,28 @@ remove_solib_event_breakpoints_at_next_stop (void)
       b->disposition = disp_del_at_next_stop;
 }
 
-struct breakpoint *
-create_solib_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
+/* Helper for create_solib_event_breakpoint /
+   create_and_insert_solib_event_breakpoint.  Allows specifying which
+   INSERT_MODE to pass through to update_global_location_list.  */
+
+static struct breakpoint *
+create_solib_event_breakpoint_1 (struct gdbarch *gdbarch, CORE_ADDR address,
+				 enum ugll_insert_mode insert_mode)
 {
   struct breakpoint *b;
 
   b = create_internal_breakpoint (gdbarch, address, bp_shlib_event,
 				  &internal_breakpoint_ops);
-  update_global_location_list_nothrow (UGLL_MAY_INSERT);
+  update_global_location_list_nothrow (insert_mode);
   return b;
 }
 
+struct breakpoint *
+create_solib_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
+{
+  return create_solib_event_breakpoint_1 (gdbarch, address, UGLL_MAY_INSERT);
+}
+
 /* See breakpoint.h.  */
 
 struct breakpoint *
@@ -7740,9 +7752,9 @@ create_and_insert_solib_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR add
 {
   struct breakpoint *b;
 
-  b = create_solib_event_breakpoint (gdbarch, address);
-  if (!breakpoints_always_inserted_mode ())
-    insert_breakpoint_locations ();
+  /* Explicitly tell update_global_location_list to insert
+     locations.  */
+  b = create_solib_event_breakpoint_1 (gdbarch, address, UGLL_INSERT);
   if (!b->loc->inserted)
     {
       delete_breakpoint (b);
@@ -12578,8 +12590,9 @@ force_breakpoint_reinsertion (struct bp_location *bl)
    deleted, to update the global location list and recompute which
    locations are duplicate of which.
 
-   The INSERT_MODE flag determines whether locations may or may not be
-   inserted now.  See 'enum ugll_insert_mode' for more info.  */
+   The INSERT_MODE flag determines whether locations may not, may, or
+   shall be inserted now.  See 'enum ugll_insert_mode' for more
+   info.  */
 
 static void
 update_global_location_list (enum ugll_insert_mode insert_mode)
@@ -12917,11 +12930,12 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			"a permanent breakpoint"));
     }
 
-  if (breakpoints_always_inserted_mode ()
-      && (have_live_inferiors ()
-	  || (gdbarch_has_global_breakpoints (target_gdbarch ()))))
+  if (insert_mode == UGLL_INSERT
+      || (breakpoints_always_inserted_mode ()
+	  && (have_live_inferiors ()
+	      || (gdbarch_has_global_breakpoints (target_gdbarch ())))))
     {
-      if (insert_mode == UGLL_MAY_INSERT)
+      if (insert_mode != UGLL_DONT_INSERT)
 	insert_breakpoint_locations ();
       else
 	{
@@ -12935,7 +12949,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 	}
     }
 
-  if (insert_mode == UGLL_MAY_INSERT)
+  if (insert_mode != UGLL_DONT_INSERT)
     download_tracepoint_locations ();
 
   do_cleanups (cleanups);
-- 
1.9.3

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 0/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
@ 2014-09-17 22:47 Pedro Alves
  2014-09-17 22:47 ` [PATCH v2 2/3] Tell update_global_location_list to insert breakpoints Pedro Alves
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Pedro Alves @ 2014-09-17 22:47 UTC (permalink / raw)
  To: gdb-patches

Here's v2 of the patch originally sent at:
  https://sourceware.org/ml/gdb-patches/2014-09/msg00562.html

In a nutshell, this is another step towards making all-stop / non-stop
closer in behavior.  It fixes "breakpoint always-inserted off" to work
correctly in non-stop mode.  As non-stop no longer needs to force
"breakpoint always-inserted on", the "breakpoint always-inserted auto"
mode becomes unnecessary, and thus this removes it.

v2:
  - Split in 3 patches.
  - Add test.
  - Docs unchanged, and already approved by Eli.

Tested on x86_64 Fedora 20.

Pedro Alves (3):
  Change parameter type of update_global_location_list from boolean to
    enum
  Tell update_global_location_list to insert breakpoints
  Fix "breakpoint always-inserted off"; remove "breakpoint
    always-inserted auto"

 gdb/NEWS                                          |   7 +
 gdb/breakpoint.c                                  | 241 +++++++++++++---------
 gdb/breakpoint.h                                  |   2 +-
 gdb/doc/gdb.texinfo                               |  13 +-
 gdb/infrun.c                                      |   5 +-
 gdb/remote.c                                      |   5 +-
 gdb/testsuite/gdb.threads/break-while-running.c   |  51 +++++
 gdb/testsuite/gdb.threads/break-while-running.exp |  87 ++++++++
 8 files changed, 297 insertions(+), 114 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/break-while-running.c
 create mode 100644 gdb/testsuite/gdb.threads/break-while-running.exp

-- 
1.9.3

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
  2014-09-17 22:47 [PATCH v2 0/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto" Pedro Alves
  2014-09-17 22:47 ` [PATCH v2 2/3] Tell update_global_location_list to insert breakpoints Pedro Alves
@ 2014-09-17 22:47 ` Pedro Alves
  2014-09-18  1:49   ` Yao Qi
  2014-09-17 22:47 ` [PATCH v2 1/3] Change parameter type of update_global_location_list from boolean to enum Pedro Alves
  2 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2014-09-17 22:47 UTC (permalink / raw)
  To: gdb-patches

By default, GDB removes all breakpoints from the target when the
target stops and the prompt is given back to the user.  This is useful
in case GDB crashes while the user is interacting, as otherwise,
there's a higher chance breakpoints would be left planted on the
target.

But, as long as any thread is running free, we need to make sure to
keep breakpoints inserted, lest a thread misses a breakpoint.  With
that in mind, in preparation for non-stop mode, we added a "breakpoint
always-inserted on" mode.  This traded off the extra crash protection
for never having threads miss breakpoints, and in addition is more
efficient if there's a ton of breakpoints to remove/insert at each
user command (e.g., at each "step").

When we added non-stop mode, and for a period, we required users to
manually set "always-inserted on" when they enabled non-stop mode, as
otherwise GDB removes all breakpoints from the target as soon as any
thread stops, even if other threads are still running.  That soon
revealed a nuisance, and so later we added an extra "breakpoint
always-inserted auto" mode, that made GDB behave like "always-inserted
on" when non-stop was enabled, and "always-inserted off" when non-stop
was disabled.  "auto" was made the default at the same time.

In hindsight, this "auto" setting was unnecessary, and not the ideal
solution.  Non-stop mode does depends on breakpoints always-inserted
mode, but only as long as any thread is running.  If no thread is
running, no breakpoint can be missed.  The same is true for all-stop
too.  E.g., if, in all-stop mode, and the user does:

 (gdb) c&
 (gdb) b foo

That breakpoint at "foo" should be inserted immediately, but it
currently isn't -- currently it'll end up inserted only if the target
happens to trip on some event, and is re-resumed, e.g., an internal
breakpoint triggers that doesn't cause a user-visible stop, and so we
end up in keep_going calling insert_breakpoints.  The patch adds a new
test that covers this.  It fails before this patch.

IOW, no matter whether in non-stop or all-stop, if the target fully
stops, we can remove breakpoints.  And no matter whether in all-stop
or non-stop, if any thread is running in the target, then we need
breakpoints to be immediately inserted.  And then, if the target has
global breakpoints, we need to keep breakpoints even when the target
is stopped.

So with that in mind, and aiming at reducing all-stop vs non-stop
differences for all-stop-on-stop-of-non-stop, this patch fixes
"breakpoint always-inserted off" to not remove breakpoints from the
target until it fully stops, and then removes the "auto" setting as
unnecessary.  I propose removing it straight away rather than keeping
it as an alias, unless someone complains they have scripts that need
it and that can't adjust.

Tested on x86_64 Fedora 20.

gdb/
2014-09-17  Pedro Alves  <palves@redhat.com>

	* NEWS: Mention merge of "breakpoint always-inserted" modes "off"
	and "auto" merged.
	* breakpoint.c (enum ugll_insert_mode): New enum.
	(always_inserted_mode): Now a plain boolean.
	(show_always_inserted_mode): No longer handle AUTO_BOOLEAN_AUTO.
	(breakpoints_always_inserted_mode): Delete.
	(breakpoints_should_be_inserted_now): New function.
	(insert_breakpoints): Pass UGLL_INSERT to
	update_global_location_list instead of calling
	insert_breakpoint_locations manually.
	(create_solib_event_breakpoint_1): New, factored out from ...
	(create_solib_event_breakpoint): ... this.
	(create_and_insert_solib_event_breakpoint): Use
	create_solib_event_breakpoint_1 instead of calling
	insert_breakpoint_locations manually.
	(update_global_location_list): Change parameter type from boolean
	to enum ugll_insert_mode.  All callers adjusted.  Adjust to use
	breakpoints_should_be_inserted_now and handle UGLL_INSERT.
	(update_global_location_list_nothrow): Change parameter type from
	boolean to enum ugll_insert_mode.
	(_initialize_breakpoint): "breakpoint always-inserted" option is
	now a boolean command.  Update help text.
	* breakpoint.h (breakpoints_always_inserted_mode): Delete declaration.
	(breakpoints_should_be_inserted_now): New declaration.
	* infrun.c (handle_inferior_event) <TARGET_WAITKIND_LOADED>:
	Remove breakpoints_always_inserted_mode check.
	(normal_stop): Adjust to use breakpoints_should_be_inserted_now.
	* remote.c (remote_start_remote): Likewise.

gdb/doc/
2014-09-17  Pedro Alves  <palves@redhat.com>

	* gdb.texinfo (Set Breaks): Document that "set breakpoint
	always-inserted off" is the default mode now.  Delete
	documentation of "set breakpoint always-inserted auto".

gdb/testsuite/
2014-09-17  Pedro Alves  <palves@redhat.com>

	* gdb.threads/break-while-running.exp: New file.
	* gdb.threads/break-while-running.c: New file.
---
 gdb/NEWS                                          |  7 ++
 gdb/breakpoint.c                                  | 90 +++++++++++++----------
 gdb/breakpoint.h                                  |  2 +-
 gdb/doc/gdb.texinfo                               | 13 +---
 gdb/infrun.c                                      |  5 +-
 gdb/remote.c                                      |  5 +-
 gdb/testsuite/gdb.threads/break-while-running.c   | 51 +++++++++++++
 gdb/testsuite/gdb.threads/break-while-running.exp | 87 ++++++++++++++++++++++
 8 files changed, 205 insertions(+), 55 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/break-while-running.c
 create mode 100644 gdb/testsuite/gdb.threads/break-while-running.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 343ee49..ac94874 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -31,6 +31,13 @@ queue-signal signal-name-or-number
   confirmation if the program had stopped for a signal and the user
   switched threads meanwhile.
 
+* "breakpoint always-inserted" modes "off" and "auto" merged.
+
+  Now, when 'breakpoint always-inserted mode' is set to "off", GDB
+  won't remove breakpoints from the target until all threads stop,
+  even in non-stop mode.  The "auto" mode has been removed, and "off"
+  is now the default mode.
+
 *** Changes in GDB 7.8
 
 * New command line options
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3f372de..3675b4f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -258,12 +258,17 @@ enum ugll_insert_mode
      the inferior.  */
   UGLL_DONT_INSERT,
 
-  /* May insert breakpoints if breakpoints_always_inserted_mode is
-     true.  */
+  /* May insert breakpoints iff breakpoints_should_be_inserted_now
+     claims breakpoints should be inserted now.  */
   UGLL_MAY_INSERT,
 
-  /* Insert locations now, even if breakpoints_always_inserted_mode is
-     false.  */
+  /* Insert locations now, irrespective of
+     breakpoints_should_be_inserted_now.  E.g., say all threads are
+     stopped right now, and the user did "continue".  We need to
+     insert breakpoints _before_ resuming the target, but
+     UGLL_MAY_INSERT wouldn't insert them, because
+     breakpoints_should_be_inserted_now returns false at that point,
+     as no thread is running yet.  */
   UGLL_INSERT
 };
 
@@ -451,34 +456,51 @@ show_automatic_hardware_breakpoints (struct ui_file *file, int from_tty,
 		    value);
 }
 
-/* If on, gdb will keep breakpoints inserted even as inferior is
-   stopped, and immediately insert any new breakpoints.  If off, gdb
-   will insert breakpoints into inferior only when resuming it, and
-   will remove breakpoints upon stop.  If auto, GDB will behave as ON
-   if in non-stop mode, and as OFF if all-stop mode.*/
-
-static enum auto_boolean always_inserted_mode = AUTO_BOOLEAN_AUTO;
+/* If on, GDB keeps breakpoints inserted even if the inferior is
+   stopped, and immediately inserts any new breakpoints as soon as
+   they're created.  If off (default), GDB keeps breakpoints off of
+   the target as long as possible.  That is, it delays inserting
+   breakpoints until the next resume, and removes them again when the
+   target fully stops.  This is a bit safer in case GDB crashes while
+   processing user input.  */
+static int always_inserted_mode = 0;
 
 static void
 show_always_inserted_mode (struct ui_file *file, int from_tty,
 		     struct cmd_list_element *c, const char *value)
 {
-  if (always_inserted_mode == AUTO_BOOLEAN_AUTO)
-    fprintf_filtered (file,
-		      _("Always inserted breakpoint "
-			"mode is %s (currently %s).\n"),
-		      value,
-		      breakpoints_always_inserted_mode () ? "on" : "off");
-  else
-    fprintf_filtered (file, _("Always inserted breakpoint mode is %s.\n"),
-		      value);
+  fprintf_filtered (file, _("Always inserted breakpoint mode is %s.\n"),
+		    value);
 }
 
 int
-breakpoints_always_inserted_mode (void)
+breakpoints_should_be_inserted_now (void)
 {
-  return (always_inserted_mode == AUTO_BOOLEAN_TRUE
-	  || (always_inserted_mode == AUTO_BOOLEAN_AUTO && non_stop));
+  if (gdbarch_has_global_breakpoints (target_gdbarch ()))
+    {
+      /* If breakpoints are global, they should be inserted even if no
+	 thread under gdb's control is running, or even if there are
+	 no threads under GDB's control yet.  */
+      return 1;
+    }
+  else if (target_has_execution)
+    {
+      struct thread_info *tp;
+
+      if (always_inserted_mode)
+	{
+	  /* The user wants breakpoints inserted even if all threads
+	     are stopped.  */
+	  return 1;
+	}
+
+      ALL_NON_EXITED_THREADS (tp)
+        {
+	  if (tp->executing)
+	    return 1;
+	}
+    }
+  return 0;
 }
 
 static const char condition_evaluation_both[] = "host or target";
@@ -12930,10 +12952,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			"a permanent breakpoint"));
     }
 
-  if (insert_mode == UGLL_INSERT
-      || (breakpoints_always_inserted_mode ()
-	  && (have_live_inferiors ()
-	      || (gdbarch_has_global_breakpoints (target_gdbarch ())))))
+  if (insert_mode == UGLL_INSERT || breakpoints_should_be_inserted_now ())
     {
       if (insert_mode != UGLL_DONT_INSERT)
 	insert_breakpoint_locations ();
@@ -17020,18 +17039,15 @@ a warning will be emitted for such breakpoints."),
 			   &breakpoint_set_cmdlist,
 			   &breakpoint_show_cmdlist);
 
-  add_setshow_auto_boolean_cmd ("always-inserted", class_support,
-				&always_inserted_mode, _("\
+  add_setshow_boolean_cmd ("always-inserted", class_support,
+			   &always_inserted_mode, _("\
 Set mode for inserting breakpoints."), _("\
 Show mode for inserting breakpoints."), _("\
-When this mode is off, breakpoints are inserted in inferior when it is\n\
-resumed, and removed when execution stops.  When this mode is on,\n\
-breakpoints are inserted immediately and removed only when the user\n\
-deletes the breakpoint.  When this mode is auto (which is the default),\n\
-the behaviour depends on the non-stop setting (see help set non-stop).\n\
-In this case, if gdb is controlling the inferior in non-stop mode, gdb\n\
-behaves as if always-inserted mode is on; if gdb is controlling the\n\
-inferior in all-stop mode, gdb behaves as if always-inserted mode is off."),
+When this mode is on, breakpoints are inserted immediately as soon as\n\
+they're created, kept inserted even when execution stops, and removed\n\
+only when the user deletes them.  When this mode is off (the default),\n\
+breakpoints are inserted only when execution continues, and removed\n\
+when execution stops."),
 				NULL,
 				&show_always_inserted_mode,
 				&breakpoint_set_cmdlist,
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 00c8802..e191c10 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1491,7 +1491,7 @@ extern void breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
 				    const gdb_byte *writebuf_org,
 				    ULONGEST memaddr, LONGEST len);
 
-extern int breakpoints_always_inserted_mode (void);
+extern int breakpoints_should_be_inserted_now (void);
 
 /* Called each time new event from target is processed.
    Retires previously deleted breakpoint locations that
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1bb1c0c..537fae8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3850,22 +3850,13 @@ This behavior can be controlled with the following commands::
 @item set breakpoint always-inserted off
 All breakpoints, including newly added by the user, are inserted in
 the target only when the target is resumed.  All breakpoints are
-removed from the target when it stops.
+removed from the target when it stops.  This is the default mode.
 
 @item set breakpoint always-inserted on
 Causes all breakpoints to be inserted in the target at all times.  If
 the user adds a new breakpoint, or changes an existing breakpoint, the
 breakpoints in the target are updated immediately.  A breakpoint is
-removed from the target only when breakpoint itself is removed.
-
-@cindex non-stop mode, and @code{breakpoint always-inserted}
-@item set breakpoint always-inserted auto
-This is the default mode.  If @value{GDBN} is controlling the inferior
-in non-stop mode (@pxref{Non-Stop Mode}), gdb behaves as if
-@code{breakpoint always-inserted} mode is on.  If @value{GDBN} is
-controlling the inferior in all-stop mode, @value{GDBN} behaves as if
-@code{breakpoint always-inserted} mode is off.
-@end table
+removed from the target only when breakpoint itself is deleted.
 
 @value{GDBN} handles conditional breakpoints by evaluating these conditions
 when a breakpoint breaks.  If the condition is true, then the process being
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c18267f..3725f2d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3448,8 +3448,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 	{
 	  /* Loading of shared libraries might have changed breakpoint
 	     addresses.  Make sure new breakpoints are inserted.  */
-	  if (stop_soon == NO_STOP_QUIETLY
-	      && !breakpoints_always_inserted_mode ())
+	  if (stop_soon == NO_STOP_QUIETLY)
 	    insert_breakpoints ();
 	  resume (0, GDB_SIGNAL_0);
 	  prepare_to_wait (ecs);
@@ -6110,7 +6109,7 @@ normal_stop (void)
       printf_filtered (_("No unwaited-for children left.\n"));
     }
 
-  if (!breakpoints_always_inserted_mode () && target_has_execution)
+  if (!breakpoints_should_be_inserted_now () && target_has_execution)
     {
       if (remove_breakpoints ())
 	{
diff --git a/gdb/remote.c b/gdb/remote.c
index 357e9f2..41ea012 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3604,9 +3604,8 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
      up.  */
   rs->starting_up = 0;
 
-  /* If breakpoints are global, insert them now.  */
-  if (gdbarch_has_global_breakpoints (target_gdbarch ())
-      && breakpoints_always_inserted_mode ())
+  /* Maybe breakpoints are global and need to be inserted now.  */
+  if (breakpoints_should_be_inserted_now ())
     insert_breakpoints ();
 }
 
diff --git a/gdb/testsuite/gdb.threads/break-while-running.c b/gdb/testsuite/gdb.threads/break-while-running.c
new file mode 100644
index 0000000..b900996
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/break-while-running.c
@@ -0,0 +1,51 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+#include <pthread.h>
+#include <assert.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+
+volatile unsigned int counter = 1;
+
+void *
+child_function (void *arg)
+{
+  while (counter > 0)
+    {
+      counter++; /* breakpoint A child here */
+      usleep (1); /* breakpoint B child here */
+    }
+
+  pthread_exit (NULL);
+}
+
+int
+main (void)
+{
+  pthread_t child_thread;
+  int res;
+
+  res = pthread_create (&child_thread, NULL, child_function, NULL);
+  assert (res == 0);
+
+  pthread_join (child_thread, NULL);
+
+  exit (EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.threads/break-while-running.exp b/gdb/testsuite/gdb.threads/break-while-running.exp
new file mode 100644
index 0000000..163c0ec
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/break-while-running.exp
@@ -0,0 +1,87 @@
+# Copyright (C) 2014 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/>.
+
+# Test that setting a breakpoint while a thread is running results in
+# the breakpoint being inserted.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] == -1} {
+    return -1
+}
+
+# The test proper.
+
+proc test {} {
+    global srcfile binfile
+    global gdb_prompt
+    global decimal
+
+    clean_restart $binfile
+
+    if ![runto_main] {
+	continue
+    }
+
+    # Check whether we're testing with the remote or extended-remote
+    # targets.  If so, skip the tests, as with the RSP, we can't issue
+    # commands until the target replies to vCont.
+    if [gdb_is_target_remote] {
+	return -1
+    }
+
+    gdb_breakpoint [gdb_get_line_number "breakpoint A child here"]
+    gdb_continue_to_breakpoint "run to child breakpoint"
+    gdb_test "info threads" "\\\* 2 .* 1.*" "info threads shows all threads"
+
+    delete_breakpoints
+
+    # Leave one thread stopped, so GDB
+    gdb_test_no_output "set scheduler-locking on"
+
+    gdb_test "continue &" "Continuing\."
+
+    # Switch to a stopped thread, so GDB can poke at memory freely.
+    gdb_test "thread 1" "Switching to .*" "switch to stopped thread"
+
+    gdb_test "info threads" "2.*running.*\\\* 1.*" "info threads shows running thread"
+
+    set linenum [gdb_get_line_number "breakpoint B child here"]
+
+    # Don't use gdb_test as it's racy in this case -- gdb_test matches
+    # the prompt with an end anchor.  Sometimes expect will manage to
+    # read the breakpoint hit output while still processing this test,
+    # defeating the anchor.
+    set test "set breakpoint while a thread is running"
+    gdb_test_multiple "break $srcfile:$linenum" $test {
+	-re "Breakpoint $decimal at .*: file .*$srcfile, line $linenum.\r\n$gdb_prompt " {
+	    pass $test
+	}
+	-re "$gdb_prompt " {
+	    fail $test
+	}
+    }
+
+    # Check that the breakpoint is hit.  Can't use gdb_test here, as
+    # no prompt is expected to come out.
+    set test "breakpoint is hit"
+    gdb_test_multiple "" $test {
+	-re "Breakpoint .*, child_function.*break-while-running.c:$linenum.*breakpoint B child here.*" {
+	    pass $test
+	}
+    }
+}
+
+test
-- 
1.9.3

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/3] Change parameter type of update_global_location_list from boolean to enum
  2014-09-17 22:47 [PATCH v2 0/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto" Pedro Alves
  2014-09-17 22:47 ` [PATCH v2 2/3] Tell update_global_location_list to insert breakpoints Pedro Alves
  2014-09-17 22:47 ` [PATCH v2 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto" Pedro Alves
@ 2014-09-17 22:47 ` Pedro Alves
  2 siblings, 0 replies; 12+ messages in thread
From: Pedro Alves @ 2014-09-17 22:47 UTC (permalink / raw)
  To: gdb-patches

Later we'll want a tristate, but for now, convert to an enum that maps 1-1
with the current boolean's true/false.

gdb/
2014-09-17  Pedro Alves  <palves@redhat.com>

	* breakpoint.c (enum ugll_insert_mode): New enum.
	(update_global_location_list)
	(update_global_location_list_nothrow): Change parameter type from
	boolean to enum ugll_insert_mode.  All callers adjusted.
---
 gdb/breakpoint.c | 125 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 72 insertions(+), 53 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 94b55c3..46cd079 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -237,9 +237,35 @@ static void decref_bp_location (struct bp_location **loc);
 
 static struct bp_location *allocate_bp_location (struct breakpoint *bpt);
 
-static void update_global_location_list (int);
+/* update_global_location_list's modes of operation wrt to whether to
+   insert locations now.  */
+enum ugll_insert_mode
+{
+  /* Don't insert any breakpoint locations into the inferior, only
+     remove already-inserted locations that no longer should be
+     inserted.  Functions that delete a breakpoint or breakpoints
+     should specify this mode, so that deleting a breakpoint doesn't
+     have the side effect of inserting the locations of other
+     breakpoints that are marked not-inserted, but should_be_inserted
+     returns true on them.
+
+     This behavior is useful is situations close to tear-down -- e.g.,
+     after an exec, while the target still has execution, but
+     breakpoint shadows of the previous executable image should *NOT*
+     be restored to the new image; or before detaching, where the
+     target still has execution and wants to delete breakpoints from
+     GDB's lists, and all breakpoints had already been removed from
+     the inferior.  */
+  UGLL_DONT_INSERT,
+
+  /* May insert breakpoints if breakpoints_always_inserted_mode is
+     true.  */
+  UGLL_MAY_INSERT
+};
+
+static void update_global_location_list (enum ugll_insert_mode);
 
-static void update_global_location_list_nothrow (int);
+static void update_global_location_list_nothrow (enum ugll_insert_mode);
 
 static int is_hardware_watchpoint (const struct breakpoint *bpt);
 
@@ -835,7 +861,7 @@ set_condition_evaluation_mode (char *args, int from_tty,
 	}
 
       /* Do the update.  */
-      update_global_location_list (1);
+      update_global_location_list (UGLL_MAY_INSERT);
     }
 
   return;
@@ -1063,7 +1089,7 @@ condition_command (char *arg, int from_tty)
 	set_breakpoint_condition (b, p, from_tty);
 
 	if (is_breakpoint (b))
-	  update_global_location_list (1);
+	  update_global_location_list (UGLL_MAY_INSERT);
 
 	return;
       }
@@ -2908,7 +2934,7 @@ breakpoint_program_space_exit (struct program_space *pspace)
 
   /* Now update the global location list to permanently delete the
      removed locations above.  */
-  update_global_location_list (0);
+  update_global_location_list (UGLL_DONT_INSERT);
 }
 
 /* Make sure all breakpoints are inserted in inferior.
@@ -2928,7 +2954,7 @@ insert_breakpoints (void)
 	update_watchpoint (w, 0 /* don't reparse.  */);
       }
 
-  update_global_location_list (1);
+  update_global_location_list (UGLL_MAY_INSERT);
 
   /* update_global_location_list does not insert breakpoints when
      always_inserted_mode is not enabled.  Explicitly insert them
@@ -3391,7 +3417,7 @@ create_overlay_event_breakpoint (void)
          overlay_events_enabled = 0;
        }
     }
-  update_global_location_list (1);
+  update_global_location_list (UGLL_MAY_INSERT);
 }
 
 static void
@@ -3501,7 +3527,7 @@ create_longjmp_master_breakpoint (void)
 	}
     }
   }
-  update_global_location_list (1);
+  update_global_location_list (UGLL_MAY_INSERT);
 
   do_cleanups (old_chain);
 }
@@ -3557,7 +3583,7 @@ create_std_terminate_master_breakpoint (void)
     }
   }
 
-  update_global_location_list (1);
+  update_global_location_list (UGLL_MAY_INSERT);
 
   do_cleanups (old_chain);
 }
@@ -3659,7 +3685,7 @@ create_exception_master_breakpoint (void)
       b->enable_state = bp_disabled;
     }
 
-  update_global_location_list (1);
+  update_global_location_list (UGLL_MAY_INSERT);
 }
 
 void
@@ -5654,9 +5680,9 @@ bpstat_stop_status (struct address_space *aspace,
 	}
 
   if (need_remove_insert)
-    update_global_location_list (1);
+    update_global_location_list (UGLL_MAY_INSERT);
   else if (removed_any)
-    update_global_location_list (0);
+    update_global_location_list (UGLL_DONT_INSERT);
 
   return bs_head;
 }
@@ -7565,7 +7591,7 @@ enable_overlay_breakpoints (void)
     if (b->type == bp_overlay_event)
     {
       b->enable_state = bp_enabled;
-      update_global_location_list (1);
+      update_global_location_list (UGLL_MAY_INSERT);
       overlay_events_enabled = 1;
     }
 }
@@ -7579,7 +7605,7 @@ disable_overlay_breakpoints (void)
     if (b->type == bp_overlay_event)
     {
       b->enable_state = bp_disabled;
-      update_global_location_list (0);
+      update_global_location_list (UGLL_DONT_INSERT);
       overlay_events_enabled = 0;
     }
 }
@@ -7624,7 +7650,7 @@ create_thread_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
   b->addr_string
     = xstrprintf ("*%s", paddress (b->loc->gdbarch, b->loc->address));
 
-  update_global_location_list_nothrow (1);
+  update_global_location_list_nothrow (UGLL_MAY_INSERT);
 
   return b;
 }
@@ -7655,7 +7681,7 @@ create_jit_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
 
   b = create_internal_breakpoint (gdbarch, address, bp_jit_event,
 				  &internal_breakpoint_ops);
-  update_global_location_list_nothrow (1);
+  update_global_location_list_nothrow (UGLL_MAY_INSERT);
   return b;
 }
 
@@ -7703,7 +7729,7 @@ create_solib_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
 
   b = create_internal_breakpoint (gdbarch, address, bp_shlib_event,
 				  &internal_breakpoint_ops);
-  update_global_location_list_nothrow (1);
+  update_global_location_list_nothrow (UGLL_MAY_INSERT);
   return b;
 }
 
@@ -8838,7 +8864,7 @@ install_breakpoint (int internal, struct breakpoint *b, int update_gll)
   observer_notify_breakpoint_created (b);
 
   if (update_gll)
-    update_global_location_list (1);
+    update_global_location_list (UGLL_MAY_INSERT);
 }
 
 static void
@@ -9080,7 +9106,7 @@ disable_watchpoints_before_interactive_call_start (void)
     if (is_watchpoint (b) && breakpoint_enabled (b))
       {
 	b->enable_state = bp_call_disabled;
-	update_global_location_list (0);
+	update_global_location_list (UGLL_DONT_INSERT);
       }
   }
 }
@@ -9095,7 +9121,7 @@ enable_watchpoints_after_interactive_call_stop (void)
     if (is_watchpoint (b) && b->enable_state == bp_call_disabled)
       {
 	b->enable_state = bp_enabled;
-	update_global_location_list (1);
+	update_global_location_list (UGLL_MAY_INSERT);
       }
   }
 }
@@ -9104,7 +9130,7 @@ void
 disable_breakpoints_before_startup (void)
 {
   current_program_space->executing_startup = 1;
-  update_global_location_list (0);
+  update_global_location_list (UGLL_DONT_INSERT);
 }
 
 void
@@ -9140,7 +9166,7 @@ set_momentary_breakpoint (struct gdbarch *gdbarch, struct symtab_and_line sal,
   if (in_thread_list (inferior_ptid))
     b->thread = pid_to_thread_id (inferior_ptid);
 
-  update_global_location_list_nothrow (1);
+  update_global_location_list_nothrow (UGLL_MAY_INSERT);
 
   return b;
 }
@@ -9178,7 +9204,7 @@ momentary_breakpoint_from_master (struct breakpoint *orig,
   copy->disposition = disp_donttouch;
   copy->number = internal_breakpoint_number--;
 
-  update_global_location_list_nothrow (0);
+  update_global_location_list_nothrow (UGLL_DONT_INSERT);
   return copy;
 }
 
@@ -10133,7 +10159,7 @@ create_breakpoint (struct gdbarch *gdbarch,
   do_cleanups (old_chain);
 
   /* error call may happen here - have BKPT_CHAIN already discarded.  */
-  update_global_location_list (1);
+  update_global_location_list (UGLL_MAY_INSERT);
 
   return 1;
 }
@@ -10645,7 +10671,7 @@ break_range_command (char *arg, int from_tty)
 
   mention (b);
   observer_notify_breakpoint_created (b);
-  update_global_location_list (1);
+  update_global_location_list (UGLL_MAY_INSERT);
 }
 
 /*  Return non-zero if EXP is verified as constant.  Returned zero
@@ -12548,24 +12574,15 @@ force_breakpoint_reinsertion (struct bp_location *bl)
 	}
     }
 }
+/* Called whether new breakpoints are created, or existing breakpoints
+   deleted, to update the global location list and recompute which
+   locations are duplicate of which.
 
-/* If SHOULD_INSERT is false, do not insert any breakpoint locations
-   into the inferior, only remove already-inserted locations that no
-   longer should be inserted.  Functions that delete a breakpoint or
-   breakpoints should pass false, so that deleting a breakpoint
-   doesn't have the side effect of inserting the locations of other
-   breakpoints that are marked not-inserted, but should_be_inserted
-   returns true on them.
-
-   This behaviour is useful is situations close to tear-down -- e.g.,
-   after an exec, while the target still has execution, but breakpoint
-   shadows of the previous executable image should *NOT* be restored
-   to the new image; or before detaching, where the target still has
-   execution and wants to delete breakpoints from GDB's lists, and all
-   breakpoints had already been removed from the inferior.  */
+   The INSERT_MODE flag determines whether locations may or may not be
+   inserted now.  See 'enum ugll_insert_mode' for more info.  */
 
 static void
-update_global_location_list (int should_insert)
+update_global_location_list (enum ugll_insert_mode insert_mode)
 {
   struct breakpoint *b;
   struct bp_location **locp, *loc;
@@ -12904,19 +12921,21 @@ update_global_location_list (int should_insert)
       && (have_live_inferiors ()
 	  || (gdbarch_has_global_breakpoints (target_gdbarch ()))))
     {
-      if (should_insert)
+      if (insert_mode == UGLL_MAY_INSERT)
 	insert_breakpoint_locations ();
       else
 	{
-	  /* Though should_insert is false, we may need to update conditions
-	     on the target's side if it is evaluating such conditions.  We
+	  /* Even though the caller told us to not insert new
+	     locations, we may still need to update conditions on the
+	     target's side of breakpoints that were already inserted
+	     if the target is evaluating breakpoint conditions.  We
 	     only update conditions for locations that are marked
 	     "needs_update".  */
 	  update_inserted_breakpoint_locations ();
 	}
     }
 
-  if (should_insert)
+  if (insert_mode == UGLL_MAY_INSERT)
     download_tracepoint_locations ();
 
   do_cleanups (cleanups);
@@ -12938,12 +12957,12 @@ breakpoint_retire_moribund (void)
 }
 
 static void
-update_global_location_list_nothrow (int inserting)
+update_global_location_list_nothrow (enum ugll_insert_mode insert_mode)
 {
   volatile struct gdb_exception e;
 
   TRY_CATCH (e, RETURN_MASK_ERROR)
-    update_global_location_list (inserting);
+    update_global_location_list (insert_mode);
 }
 
 /* Clear BKP from a BPS.  */
@@ -14096,7 +14115,7 @@ delete_breakpoint (struct breakpoint *bpt)
      itself, since remove_breakpoint looks at location's owner.  It
      might be better design to have location completely
      self-contained, but it's not the case now.  */
-  update_global_location_list (0);
+  update_global_location_list (UGLL_DONT_INSERT);
 
   bpt->ops->dtor (bpt);
   /* On the chance that someone will soon try again to delete this
@@ -14425,7 +14444,7 @@ update_breakpoint_locations (struct breakpoint *b,
       /* Ranged breakpoints have only one start location and one end
 	 location.  */
       b->enable_state = bp_disabled;
-      update_global_location_list (1);
+      update_global_location_list (UGLL_MAY_INSERT);
       printf_unfiltered (_("Could not reset ranged breakpoint %d: "
 			   "multiple locations found\n"),
 			 b->number);
@@ -14528,7 +14547,7 @@ update_breakpoint_locations (struct breakpoint *b,
   if (!locations_are_equal (existing_locations, b->loc))
     observer_notify_breakpoint_modified (b);
 
-  update_global_location_list (1);
+  update_global_location_list (UGLL_MAY_INSERT);
 }
 
 /* Find the SaL locations corresponding to the given ADDR_STRING.
@@ -14986,7 +15005,7 @@ disable_breakpoint (struct breakpoint *bpt)
 	target_disable_tracepoint (location);
     }
 
-  update_global_location_list (0);
+  update_global_location_list (UGLL_DONT_INSERT);
 
   observer_notify_breakpoint_modified (bpt);
 }
@@ -15041,7 +15060,7 @@ disable_command (char *args, int from_tty)
 		      && is_tracepoint (loc->owner))
 		    target_disable_tracepoint (loc);
 		}
-	      update_global_location_list (0);
+	      update_global_location_list (UGLL_DONT_INSERT);
 	    }
 	  else
 	    map_breakpoint_numbers (num, do_map_disable_breakpoint, NULL);
@@ -15111,7 +15130,7 @@ enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition,
 
   bpt->disposition = disposition;
   bpt->enable_count = count;
-  update_global_location_list (1);
+  update_global_location_list (UGLL_MAY_INSERT);
 
   observer_notify_breakpoint_modified (bpt);
 }
@@ -15175,7 +15194,7 @@ enable_command (char *args, int from_tty)
 		      && is_tracepoint (loc->owner))
 		    target_enable_tracepoint (loc);
 		}
-	      update_global_location_list (1);
+	      update_global_location_list (UGLL_MAY_INSERT);
 	    }
 	  else
 	    map_breakpoint_numbers (num, do_map_enable_breakpoint, NULL);
-- 
1.9.3

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
  2014-09-17 22:47 ` [PATCH v2 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto" Pedro Alves
@ 2014-09-18  1:49   ` Yao Qi
  2014-09-19 19:05     ` [PATCH v3 " Pedro Alves
  0 siblings, 1 reply; 12+ messages in thread
From: Yao Qi @ 2014-09-18  1:49 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

Pedro Alves <palves@redhat.com> writes:

> -  /* Insert locations now, even if breakpoints_always_inserted_mode is
> -     false.  */
> +  /* Insert locations now, irrespective of
> +     breakpoints_should_be_inserted_now.  E.g., say all threads are
> +     stopped right now, and the user did "continue".  We need to
> +     insert breakpoints _before_ resuming the target, but
> +     UGLL_MAY_INSERT wouldn't insert them, because
> +     breakpoints_should_be_inserted_now returns false at that point,
> +     as no thread is running yet.  */
>    UGLL_INSERT

Yeah, that is clear to me.

> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <pthread.h>
> +#include <assert.h>
> +#include <pthread.h>

pthread.h is included twice.
> +
> +    gdb_breakpoint [gdb_get_line_number "breakpoint A child here"]
> +    gdb_continue_to_breakpoint "run to child breakpoint"
> +    gdb_test "info threads" "\\\* 2 .* 1.*" "info threads shows all threads"
> +
> +    delete_breakpoints
> +
> +    # Leave one thread stopped, so GDB
> +    gdb_test_no_output "set scheduler-locking on"
> +
> +    gdb_test "continue &" "Continuing\."
> +
> +    # Switch to a stopped thread, so GDB can poke at memory freely.
> +    gdb_test "thread 1" "Switching to .*" "switch to stopped thread"
> +
> +    gdb_test "info threads" "2.*running.*\\\* 1.*" "info threads shows running thread"
> +

IWBN to check thread 1 is stopped too, because thread 1 isn't stopped in
non-stop mode, or the test can stop thread 1 explicitly if it isn't.

-- 
Yao (齐尧)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
  2014-09-18  1:49   ` Yao Qi
@ 2014-09-19 19:05     ` Pedro Alves
  2014-09-21  9:45       ` [PATCH v4 " Pedro Alves
  0 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2014-09-19 19:05 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

On 09/18/2014 02:45 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> -  /* Insert locations now, even if breakpoints_always_inserted_mode is
>> -     false.  */
>> +  /* Insert locations now, irrespective of
>> +     breakpoints_should_be_inserted_now.  E.g., say all threads are
>> +     stopped right now, and the user did "continue".  We need to
>> +     insert breakpoints _before_ resuming the target, but
>> +     UGLL_MAY_INSERT wouldn't insert them, because
>> +     breakpoints_should_be_inserted_now returns false at that point,
>> +     as no thread is running yet.  */
>>    UGLL_INSERT
> 
> Yeah, that is clear to me.

Good.  :-)

>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +#include <pthread.h>
>> +#include <assert.h>
>> +#include <pthread.h>
> 
> pthread.h is included twice.

Thanks, fixed.

>> +
>> +    gdb_breakpoint [gdb_get_line_number "breakpoint A child here"]
>> +    gdb_continue_to_breakpoint "run to child breakpoint"
>> +    gdb_test "info threads" "\\\* 2 .* 1.*" "info threads shows all threads"
>> +
>> +    delete_breakpoints
>> +
>> +    # Leave one thread stopped, so GDB
>> +    gdb_test_no_output "set scheduler-locking on"
>> +
>> +    gdb_test "continue &" "Continuing\."
>> +
>> +    # Switch to a stopped thread, so GDB can poke at memory freely.
>> +    gdb_test "thread 1" "Switching to .*" "switch to stopped thread"
>> +
>> +    gdb_test "info threads" "2.*running.*\\\* 1.*" "info threads shows running thread"
>> +
> 
> IWBN to check thread 1 is stopped too, because thread 1 isn't stopped in
> non-stop mode, or the test can stop thread 1 explicitly if it isn't.

The test actually wasn't trying non-stop at all.  But I've
made it do that now.  Here's v3, then.  Nothing else changed.

WDYT?

---------
[PATCH] Fix "breakpoint always-inserted off"; remove "breakpoint
 always-inserted auto"

By default, GDB removes all breakpoints from the target when the
target stops and the prompt is given back to the user.  This is useful
in case GDB crashes while the user is interacting, as otherwise,
there's a higher chance breakpoints would be left planted on the
target.

But, as long as any thread is running free, we need to make sure to
keep breakpoints inserted, lest a thread misses a breakpoint.  With
that in mind, in preparation for non-stop mode, we added a "breakpoint
always-inserted on" mode.  This traded off the extra crash protection
for never having threads miss breakpoints, and in addition is more
efficient if there's a ton of breakpoints to remove/insert at each
user command (e.g., at each "step").

When we added non-stop mode, and for a period, we required users to
manually set "always-inserted on" when they enabled non-stop mode, as
otherwise GDB removes all breakpoints from the target as soon as any
thread stops, even if other threads are still running.  That soon
revealed a nuisance, and so later we added an extra "breakpoint
always-inserted auto" mode, that made GDB behave like "always-inserted
on" when non-stop was enabled, and "always-inserted off" when non-stop
was disabled.  "auto" was made the default at the same time.

In hindsight, this "auto" setting was unnecessary, and not the ideal
solution.  Non-stop mode does depends on breakpoints always-inserted
mode, but only as long as any thread is running.  If no thread is
running, no breakpoint can be missed.  The same is true for all-stop
too.  E.g., if, in all-stop mode, and the user does:

 (gdb) c&
 (gdb) b foo

That breakpoint at "foo" should be inserted immediately, but it
currently isn't -- currently it'll end up inserted only if the target
happens to trip on some event, and is re-resumed, e.g., an internal
breakpoint triggers that doesn't cause a user-visible stop, and so we
end up in keep_going calling insert_breakpoints.  The patch adds a new
test that covers this.  It fails in all-stop mode before this patch.

IOW, no matter whether in non-stop or all-stop, if the target fully
stops, we can remove breakpoints.  And no matter whether in all-stop
or non-stop, if any thread is running in the target, then we need
breakpoints to be immediately inserted.  And then, if the target has
global breakpoints, we need to keep breakpoints even when the target
is stopped.

So with that in mind, and aiming at reducing all-stop vs non-stop
differences for all-stop-on-stop-of-non-stop, this patch fixes
"breakpoint always-inserted off" to not remove breakpoints from the
target until it fully stops, and then removes the "auto" setting as
unnecessary.  I propose removing it straight away rather than keeping
it as an alias, unless someone complains they have scripts that need
it and that can't adjust.

Tested on x86_64 Fedora 20.

gdb/
2014-09-19  Pedro Alves  <palves@redhat.com>

	* NEWS: Mention merge of "breakpoint always-inserted" modes "off"
	and "auto" merged.
	* breakpoint.c (enum ugll_insert_mode): New enum.
	(always_inserted_mode): Now a plain boolean.
	(show_always_inserted_mode): No longer handle AUTO_BOOLEAN_AUTO.
	(breakpoints_always_inserted_mode): Delete.
	(breakpoints_should_be_inserted_now): New function.
	(insert_breakpoints): Pass UGLL_INSERT to
	update_global_location_list instead of calling
	insert_breakpoint_locations manually.
	(create_solib_event_breakpoint_1): New, factored out from ...
	(create_solib_event_breakpoint): ... this.
	(create_and_insert_solib_event_breakpoint): Use
	create_solib_event_breakpoint_1 instead of calling
	insert_breakpoint_locations manually.
	(update_global_location_list): Change parameter type from boolean
	to enum ugll_insert_mode.  All callers adjusted.  Adjust to use
	breakpoints_should_be_inserted_now and handle UGLL_INSERT.
	(update_global_location_list_nothrow): Change parameter type from
	boolean to enum ugll_insert_mode.
	(_initialize_breakpoint): "breakpoint always-inserted" option is
	now a boolean command.  Update help text.
	* breakpoint.h (breakpoints_always_inserted_mode): Delete declaration.
	(breakpoints_should_be_inserted_now): New declaration.
	* infrun.c (handle_inferior_event) <TARGET_WAITKIND_LOADED>:
	Remove breakpoints_always_inserted_mode check.
	(normal_stop): Adjust to use breakpoints_should_be_inserted_now.
	* remote.c (remote_start_remote): Likewise.

gdb/doc/
2014-09-19  Pedro Alves  <palves@redhat.com>

	* gdb.texinfo (Set Breaks): Document that "set breakpoint
	always-inserted off" is the default mode now.  Delete
	documentation of "set breakpoint always-inserted auto".

gdb/testsuite/
2014-09-19  Pedro Alves  <palves@redhat.com>

	* gdb.threads/break-while-running.exp: New file.
	* gdb.threads/break-while-running.c: New file.
---
 gdb/NEWS                                          |   7 ++
 gdb/breakpoint.c                                  |  90 +++++++++-------
 gdb/breakpoint.h                                  |   2 +-
 gdb/doc/gdb.texinfo                               |  13 +--
 gdb/infrun.c                                      |   5 +-
 gdb/remote.c                                      |   5 +-
 gdb/testsuite/gdb.threads/break-while-running.c   |  49 +++++++++
 gdb/testsuite/gdb.threads/break-while-running.exp | 124 ++++++++++++++++++++++
 8 files changed, 240 insertions(+), 55 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/break-while-running.c
 create mode 100644 gdb/testsuite/gdb.threads/break-while-running.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 343ee49..ac94874 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -31,6 +31,13 @@ queue-signal signal-name-or-number
   confirmation if the program had stopped for a signal and the user
   switched threads meanwhile.
 
+* "breakpoint always-inserted" modes "off" and "auto" merged.
+
+  Now, when 'breakpoint always-inserted mode' is set to "off", GDB
+  won't remove breakpoints from the target until all threads stop,
+  even in non-stop mode.  The "auto" mode has been removed, and "off"
+  is now the default mode.
+
 *** Changes in GDB 7.8
 
 * New command line options
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3f372de..3675b4f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -258,12 +258,17 @@ enum ugll_insert_mode
      the inferior.  */
   UGLL_DONT_INSERT,
 
-  /* May insert breakpoints if breakpoints_always_inserted_mode is
-     true.  */
+  /* May insert breakpoints iff breakpoints_should_be_inserted_now
+     claims breakpoints should be inserted now.  */
   UGLL_MAY_INSERT,
 
-  /* Insert locations now, even if breakpoints_always_inserted_mode is
-     false.  */
+  /* Insert locations now, irrespective of
+     breakpoints_should_be_inserted_now.  E.g., say all threads are
+     stopped right now, and the user did "continue".  We need to
+     insert breakpoints _before_ resuming the target, but
+     UGLL_MAY_INSERT wouldn't insert them, because
+     breakpoints_should_be_inserted_now returns false at that point,
+     as no thread is running yet.  */
   UGLL_INSERT
 };
 
@@ -451,34 +456,51 @@ show_automatic_hardware_breakpoints (struct ui_file *file, int from_tty,
 		    value);
 }
 
-/* If on, gdb will keep breakpoints inserted even as inferior is
-   stopped, and immediately insert any new breakpoints.  If off, gdb
-   will insert breakpoints into inferior only when resuming it, and
-   will remove breakpoints upon stop.  If auto, GDB will behave as ON
-   if in non-stop mode, and as OFF if all-stop mode.*/
-
-static enum auto_boolean always_inserted_mode = AUTO_BOOLEAN_AUTO;
+/* If on, GDB keeps breakpoints inserted even if the inferior is
+   stopped, and immediately inserts any new breakpoints as soon as
+   they're created.  If off (default), GDB keeps breakpoints off of
+   the target as long as possible.  That is, it delays inserting
+   breakpoints until the next resume, and removes them again when the
+   target fully stops.  This is a bit safer in case GDB crashes while
+   processing user input.  */
+static int always_inserted_mode = 0;
 
 static void
 show_always_inserted_mode (struct ui_file *file, int from_tty,
 		     struct cmd_list_element *c, const char *value)
 {
-  if (always_inserted_mode == AUTO_BOOLEAN_AUTO)
-    fprintf_filtered (file,
-		      _("Always inserted breakpoint "
-			"mode is %s (currently %s).\n"),
-		      value,
-		      breakpoints_always_inserted_mode () ? "on" : "off");
-  else
-    fprintf_filtered (file, _("Always inserted breakpoint mode is %s.\n"),
-		      value);
+  fprintf_filtered (file, _("Always inserted breakpoint mode is %s.\n"),
+		    value);
 }
 
 int
-breakpoints_always_inserted_mode (void)
+breakpoints_should_be_inserted_now (void)
 {
-  return (always_inserted_mode == AUTO_BOOLEAN_TRUE
-	  || (always_inserted_mode == AUTO_BOOLEAN_AUTO && non_stop));
+  if (gdbarch_has_global_breakpoints (target_gdbarch ()))
+    {
+      /* If breakpoints are global, they should be inserted even if no
+	 thread under gdb's control is running, or even if there are
+	 no threads under GDB's control yet.  */
+      return 1;
+    }
+  else if (target_has_execution)
+    {
+      struct thread_info *tp;
+
+      if (always_inserted_mode)
+	{
+	  /* The user wants breakpoints inserted even if all threads
+	     are stopped.  */
+	  return 1;
+	}
+
+      ALL_NON_EXITED_THREADS (tp)
+        {
+	  if (tp->executing)
+	    return 1;
+	}
+    }
+  return 0;
 }
 
 static const char condition_evaluation_both[] = "host or target";
@@ -12930,10 +12952,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			"a permanent breakpoint"));
     }
 
-  if (insert_mode == UGLL_INSERT
-      || (breakpoints_always_inserted_mode ()
-	  && (have_live_inferiors ()
-	      || (gdbarch_has_global_breakpoints (target_gdbarch ())))))
+  if (insert_mode == UGLL_INSERT || breakpoints_should_be_inserted_now ())
     {
       if (insert_mode != UGLL_DONT_INSERT)
 	insert_breakpoint_locations ();
@@ -17020,18 +17039,15 @@ a warning will be emitted for such breakpoints."),
 			   &breakpoint_set_cmdlist,
 			   &breakpoint_show_cmdlist);
 
-  add_setshow_auto_boolean_cmd ("always-inserted", class_support,
-				&always_inserted_mode, _("\
+  add_setshow_boolean_cmd ("always-inserted", class_support,
+			   &always_inserted_mode, _("\
 Set mode for inserting breakpoints."), _("\
 Show mode for inserting breakpoints."), _("\
-When this mode is off, breakpoints are inserted in inferior when it is\n\
-resumed, and removed when execution stops.  When this mode is on,\n\
-breakpoints are inserted immediately and removed only when the user\n\
-deletes the breakpoint.  When this mode is auto (which is the default),\n\
-the behaviour depends on the non-stop setting (see help set non-stop).\n\
-In this case, if gdb is controlling the inferior in non-stop mode, gdb\n\
-behaves as if always-inserted mode is on; if gdb is controlling the\n\
-inferior in all-stop mode, gdb behaves as if always-inserted mode is off."),
+When this mode is on, breakpoints are inserted immediately as soon as\n\
+they're created, kept inserted even when execution stops, and removed\n\
+only when the user deletes them.  When this mode is off (the default),\n\
+breakpoints are inserted only when execution continues, and removed\n\
+when execution stops."),
 				NULL,
 				&show_always_inserted_mode,
 				&breakpoint_set_cmdlist,
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 00c8802..e191c10 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1491,7 +1491,7 @@ extern void breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
 				    const gdb_byte *writebuf_org,
 				    ULONGEST memaddr, LONGEST len);
 
-extern int breakpoints_always_inserted_mode (void);
+extern int breakpoints_should_be_inserted_now (void);
 
 /* Called each time new event from target is processed.
    Retires previously deleted breakpoint locations that
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1bb1c0c..537fae8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3850,22 +3850,13 @@ This behavior can be controlled with the following commands::
 @item set breakpoint always-inserted off
 All breakpoints, including newly added by the user, are inserted in
 the target only when the target is resumed.  All breakpoints are
-removed from the target when it stops.
+removed from the target when it stops.  This is the default mode.
 
 @item set breakpoint always-inserted on
 Causes all breakpoints to be inserted in the target at all times.  If
 the user adds a new breakpoint, or changes an existing breakpoint, the
 breakpoints in the target are updated immediately.  A breakpoint is
-removed from the target only when breakpoint itself is removed.
-
-@cindex non-stop mode, and @code{breakpoint always-inserted}
-@item set breakpoint always-inserted auto
-This is the default mode.  If @value{GDBN} is controlling the inferior
-in non-stop mode (@pxref{Non-Stop Mode}), gdb behaves as if
-@code{breakpoint always-inserted} mode is on.  If @value{GDBN} is
-controlling the inferior in all-stop mode, @value{GDBN} behaves as if
-@code{breakpoint always-inserted} mode is off.
-@end table
+removed from the target only when breakpoint itself is deleted.
 
 @value{GDBN} handles conditional breakpoints by evaluating these conditions
 when a breakpoint breaks.  If the condition is true, then the process being
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c18267f..3725f2d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3448,8 +3448,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 	{
 	  /* Loading of shared libraries might have changed breakpoint
 	     addresses.  Make sure new breakpoints are inserted.  */
-	  if (stop_soon == NO_STOP_QUIETLY
-	      && !breakpoints_always_inserted_mode ())
+	  if (stop_soon == NO_STOP_QUIETLY)
 	    insert_breakpoints ();
 	  resume (0, GDB_SIGNAL_0);
 	  prepare_to_wait (ecs);
@@ -6110,7 +6109,7 @@ normal_stop (void)
       printf_filtered (_("No unwaited-for children left.\n"));
     }
 
-  if (!breakpoints_always_inserted_mode () && target_has_execution)
+  if (!breakpoints_should_be_inserted_now () && target_has_execution)
     {
       if (remove_breakpoints ())
 	{
diff --git a/gdb/remote.c b/gdb/remote.c
index 357e9f2..41ea012 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3604,9 +3604,8 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
      up.  */
   rs->starting_up = 0;
 
-  /* If breakpoints are global, insert them now.  */
-  if (gdbarch_has_global_breakpoints (target_gdbarch ())
-      && breakpoints_always_inserted_mode ())
+  /* Maybe breakpoints are global and need to be inserted now.  */
+  if (breakpoints_should_be_inserted_now ())
     insert_breakpoints ();
 }
 
diff --git a/gdb/testsuite/gdb.threads/break-while-running.c b/gdb/testsuite/gdb.threads/break-while-running.c
new file mode 100644
index 0000000..3c3a984
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/break-while-running.c
@@ -0,0 +1,49 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+#include <pthread.h>
+#include <assert.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+volatile unsigned int counter = 1;
+
+void *
+child_function (void *arg)
+{
+  while (counter > 0)
+    {
+      counter++; /* breakpoint A child here */
+      usleep (1); /* breakpoint B child here */
+    }
+
+  pthread_exit (NULL);
+}
+
+int
+main (void)
+{
+  pthread_t child_thread;
+  int res;
+
+  res = pthread_create (&child_thread, NULL, child_function, NULL);
+  assert (res == 0);
+
+  pthread_join (child_thread, NULL);
+
+  exit (EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.threads/break-while-running.exp b/gdb/testsuite/gdb.threads/break-while-running.exp
new file mode 100644
index 0000000..5b889a6
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/break-while-running.exp
@@ -0,0 +1,124 @@
+# Copyright (C) 2014 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/>.
+
+# Test that setting a breakpoint while a thread is running results in
+# the breakpoint being inserted.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] == -1} {
+    return -1
+}
+
+# The test proper.  NON_STOP is a boolean that indicates whether we're
+# testing in non-stop, or all-stop mode.
+
+proc test { non_stop } {
+    global srcfile binfile
+    global gdb_prompt
+    global decimal
+
+    clean_restart $binfile
+
+    if $non_stop {
+	gdb_test_no_output "set non-stop on"
+    }
+
+    if ![runto_main] {
+	continue
+    }
+
+    # In all-stop, check whether we're testing with the remote or
+    # extended-remote targets.  If so, skip the tests, as with the
+    # RSP, we can't issue commands until the target replies to vCont.
+    # Not an issue with the non-stop RSP variant, which has a
+    # non-blocking vCont.
+    if { !$non_stop && [gdb_is_target_remote] } {
+	return -1
+    }
+
+    gdb_breakpoint [gdb_get_line_number "breakpoint A child here"]
+    gdb_continue_to_breakpoint "run to child breakpoint"
+
+    delete_breakpoints
+
+    # Leave one thread stopped, so GDB can poke at memory freely.
+    if !$non_stop {
+	gdb_test_no_output "set scheduler-locking on"
+	gdb_test "continue &" "Continuing\."
+	gdb_test "thread 1" "Switching to .*" "switch back to main thread"
+    } else {
+	gdb_test "thread 2" "Switching to .*" "switch to event thread"
+	gdb_test "continue &" "Continuing\."
+	gdb_test "thread 1" "Switching to .*" "switch back to main thread"
+
+	set test "interrupt thread"
+	gdb_test_multiple "interrupt" "$test" {
+	    -re "$gdb_prompt" {
+		pass $test
+	    }
+	}
+
+	set test "wait for stop"
+	gdb_test_multiple "" "$test" {
+	    -re " stopped\." {
+		pass $test
+	    }
+	}
+    }
+
+    set test "one and only one thread running"
+    gdb_test_multiple "info threads" $test {
+	-re "\\\(running\\\).*\\\(running\\\).*$gdb_prompt $" {
+	    fail $test
+	}
+	-re "\\\(running\\\).*$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    set linenum [gdb_get_line_number "breakpoint B child here"]
+
+    # Don't use gdb_test as it's racy in this case -- gdb_test matches
+    # the prompt with an end anchor.  Sometimes expect will manage to
+    # read the breakpoint hit output while still processing this test,
+    # defeating the anchor.
+    set test "set breakpoint while a thread is running"
+    gdb_test_multiple "break $srcfile:$linenum" $test {
+	-re "Breakpoint $decimal at .*: file .*$srcfile, line $linenum.\r\n$gdb_prompt " {
+	    pass $test
+	}
+	-re "$gdb_prompt " {
+	    fail $test
+	}
+    }
+
+    # Check that the breakpoint is hit.  Can't use gdb_test here, as
+    # no prompt is expected to come out.
+    set test "breakpoint is hit"
+    gdb_test_multiple "" $test {
+	-re "Breakpoint .*, child_function.*break-while-running.c:$linenum.*breakpoint B child here.*" {
+	    pass $test
+	}
+    }
+}
+
+with_test_prefix "all-stop" {
+    test false
+}
+
+with_test_prefix "non-stop" {
+    test true
+}
-- 
1.9.3


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
  2014-09-19 19:05     ` [PATCH v3 " Pedro Alves
@ 2014-09-21  9:45       ` Pedro Alves
  2014-09-22  7:48         ` Yao Qi
  0 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2014-09-21  9:45 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

On 09/19/2014 08:05 PM, Pedro Alves wrote:

>> IWBN to check thread 1 is stopped too, because thread 1 isn't stopped in
>> non-stop mode, or the test can stop thread 1 explicitly if it isn't.
> 
> The test actually wasn't trying non-stop at all.  But I've
> made it do that now.  Here's v3, then.  Nothing else changed.

I've extended the test further now, so please ignore that one.

> But, as long as any thread is running free, we need to make sure to
> keep breakpoints inserted, lest a thread misses a breakpoint.  With
> that in mind, in preparation for non-stop mode, we added a "breakpoint
> always-inserted on" mode.  This traded off the extra crash protection
> for never having threads miss breakpoints, and in addition is more
> efficient if there's a ton of breakpoints to remove/insert at each
> user command (e.g., at each "step").
> 
> When we added non-stop mode, and for a period, we required users to
> manually set "always-inserted on" when they enabled non-stop mode, as
> otherwise GDB removes all breakpoints from the target as soon as any
> thread stops, even if other threads are still running.  

I realized that we should test this part too.  So I've added another thread
to the test, and I'm testing that that thread hits the breakpoint too after
the first thread hits it.  Before the fix, with "always-inserted off", the
second thread never hits the breakpoint.

So here's v4.  Only the test (and the commit log) changed.

For convenience, I've pushed the series to:

  git@github.com:palves/gdb.git palves/breakpoint_always_inserted_hybrid

-----------
From 3a59e71d955cc5f5dd2f3f4e434e93eb64411e14 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sun, 21 Sep 2014 10:23:44 +0100
Subject: [PATCH] Fix "breakpoint always-inserted off"; remove "breakpoint
 always-inserted auto"

By default, GDB removes all breakpoints from the target when the
target stops and the prompt is given back to the user.  This is useful
in case GDB crashes while the user is interacting, as otherwise,
there's a higher chance breakpoints would be left planted on the
target.

But, as long as any thread is running free, we need to make sure to
keep breakpoints inserted, lest a thread misses a breakpoint.  With
that in mind, in preparation for non-stop mode, we added a "breakpoint
always-inserted on" mode.  This traded off the extra crash protection
for never having threads miss breakpoints, and in addition is more
efficient if there's a ton of breakpoints to remove/insert at each
user command (e.g., at each "step").

When we added non-stop mode, and for a period, we required users to
manually set "always-inserted on" when they enabled non-stop mode, as
otherwise GDB removes all breakpoints from the target as soon as any
thread stops, which means the other threads still running will miss
breakpoints.  The test added by this patch exercises this.

That soon revealed a nuisance, and so later we added an extra
"breakpoint always-inserted auto" mode, that made GDB behave like
"always-inserted on" when non-stop was enabled, and "always-inserted
off" when non-stop was disabled.  "auto" was made the default at the
same time.

In hindsight, this "auto" setting was unnecessary, and not the ideal
solution.  Non-stop mode does depends on breakpoints always-inserted
mode, but only as long as any thread is running.  If no thread is
running, no breakpoint can be missed.  The same is true for all-stop
too.  E.g., if, in all-stop mode, and the user does:

 (gdb) c&
 (gdb) b foo

That breakpoint at "foo" should be inserted immediately, but it
currently isn't -- currently it'll end up inserted only if the target
happens to trip on some event, and is re-resumed, e.g., an internal
breakpoint triggers that doesn't cause a user-visible stop, and so we
end up in keep_going calling insert_breakpoints.  The test added by
this patch also covers this.

IOW, no matter whether in non-stop or all-stop, if the target fully
stops, we can remove breakpoints.  And no matter whether in all-stop
or non-stop, if any thread is running in the target, then we need
breakpoints to be immediately inserted.  And then, if the target has
global breakpoints, we need to keep breakpoints even when the target
is stopped.

So with that in mind, and aiming at reducing all-stop vs non-stop
differences for all-stop-on-stop-of-non-stop, this patch fixes
"breakpoint always-inserted off" to not remove breakpoints from the
target until it fully stops, and then removes the "auto" setting as
unnecessary.  I propose removing it straight away rather than keeping
it as an alias, unless someone complains they have scripts that need
it and that can't adjust.

Tested on x86_64 Fedora 20.

gdb/
2014-09-21  Pedro Alves  <palves@redhat.com>

	* NEWS: Mention merge of "breakpoint always-inserted" modes "off"
	and "auto" merged.
	* breakpoint.c (enum ugll_insert_mode): New enum.
	(always_inserted_mode): Now a plain boolean.
	(show_always_inserted_mode): No longer handle AUTO_BOOLEAN_AUTO.
	(breakpoints_always_inserted_mode): Delete.
	(breakpoints_should_be_inserted_now): New function.
	(insert_breakpoints): Pass UGLL_INSERT to
	update_global_location_list instead of calling
	insert_breakpoint_locations manually.
	(create_solib_event_breakpoint_1): New, factored out from ...
	(create_solib_event_breakpoint): ... this.
	(create_and_insert_solib_event_breakpoint): Use
	create_solib_event_breakpoint_1 instead of calling
	insert_breakpoint_locations manually.
	(update_global_location_list): Change parameter type from boolean
	to enum ugll_insert_mode.  All callers adjusted.  Adjust to use
	breakpoints_should_be_inserted_now and handle UGLL_INSERT.
	(update_global_location_list_nothrow): Change parameter type from
	boolean to enum ugll_insert_mode.
	(_initialize_breakpoint): "breakpoint always-inserted" option is
	now a boolean command.  Update help text.
	* breakpoint.h (breakpoints_always_inserted_mode): Delete declaration.
	(breakpoints_should_be_inserted_now): New declaration.
	* infrun.c (handle_inferior_event) <TARGET_WAITKIND_LOADED>:
	Remove breakpoints_always_inserted_mode check.
	(normal_stop): Adjust to use breakpoints_should_be_inserted_now.
	* remote.c (remote_start_remote): Likewise.

gdb/doc/
2014-09-21  Pedro Alves  <palves@redhat.com>

	* gdb.texinfo (Set Breaks): Document that "set breakpoint
	always-inserted off" is the default mode now.  Delete
	documentation of "set breakpoint always-inserted auto".

gdb/testsuite/
2014-09-21  Pedro Alves  <palves@redhat.com>

	* gdb.threads/break-while-running.exp: New file.
	* gdb.threads/break-while-running.c: New file.
---
 gdb/NEWS                                          |   7 +
 gdb/breakpoint.c                                  |  90 +++++++------
 gdb/breakpoint.h                                  |   2 +-
 gdb/doc/gdb.texinfo                               |  13 +-
 gdb/infrun.c                                      |   5 +-
 gdb/remote.c                                      |   5 +-
 gdb/testsuite/gdb.threads/break-while-running.c   | 101 +++++++++++++++
 gdb/testsuite/gdb.threads/break-while-running.exp | 151 ++++++++++++++++++++++
 8 files changed, 319 insertions(+), 55 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/break-while-running.c
 create mode 100644 gdb/testsuite/gdb.threads/break-while-running.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 82ee57b..11326f1 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -33,6 +33,13 @@ queue-signal signal-name-or-number
   confirmation if the program had stopped for a signal and the user
   switched threads meanwhile.
 
+* "breakpoint always-inserted" modes "off" and "auto" merged.
+
+  Now, when 'breakpoint always-inserted mode' is set to "off", GDB
+  won't remove breakpoints from the target until all threads stop,
+  even in non-stop mode.  The "auto" mode has been removed, and "off"
+  is now the default mode.
+
 *** Changes in GDB 7.8
 
 * New command line options
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3f372de..3675b4f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -258,12 +258,17 @@ enum ugll_insert_mode
      the inferior.  */
   UGLL_DONT_INSERT,
 
-  /* May insert breakpoints if breakpoints_always_inserted_mode is
-     true.  */
+  /* May insert breakpoints iff breakpoints_should_be_inserted_now
+     claims breakpoints should be inserted now.  */
   UGLL_MAY_INSERT,
 
-  /* Insert locations now, even if breakpoints_always_inserted_mode is
-     false.  */
+  /* Insert locations now, irrespective of
+     breakpoints_should_be_inserted_now.  E.g., say all threads are
+     stopped right now, and the user did "continue".  We need to
+     insert breakpoints _before_ resuming the target, but
+     UGLL_MAY_INSERT wouldn't insert them, because
+     breakpoints_should_be_inserted_now returns false at that point,
+     as no thread is running yet.  */
   UGLL_INSERT
 };
 
@@ -451,34 +456,51 @@ show_automatic_hardware_breakpoints (struct ui_file *file, int from_tty,
 		    value);
 }
 
-/* If on, gdb will keep breakpoints inserted even as inferior is
-   stopped, and immediately insert any new breakpoints.  If off, gdb
-   will insert breakpoints into inferior only when resuming it, and
-   will remove breakpoints upon stop.  If auto, GDB will behave as ON
-   if in non-stop mode, and as OFF if all-stop mode.*/
-
-static enum auto_boolean always_inserted_mode = AUTO_BOOLEAN_AUTO;
+/* If on, GDB keeps breakpoints inserted even if the inferior is
+   stopped, and immediately inserts any new breakpoints as soon as
+   they're created.  If off (default), GDB keeps breakpoints off of
+   the target as long as possible.  That is, it delays inserting
+   breakpoints until the next resume, and removes them again when the
+   target fully stops.  This is a bit safer in case GDB crashes while
+   processing user input.  */
+static int always_inserted_mode = 0;
 
 static void
 show_always_inserted_mode (struct ui_file *file, int from_tty,
 		     struct cmd_list_element *c, const char *value)
 {
-  if (always_inserted_mode == AUTO_BOOLEAN_AUTO)
-    fprintf_filtered (file,
-		      _("Always inserted breakpoint "
-			"mode is %s (currently %s).\n"),
-		      value,
-		      breakpoints_always_inserted_mode () ? "on" : "off");
-  else
-    fprintf_filtered (file, _("Always inserted breakpoint mode is %s.\n"),
-		      value);
+  fprintf_filtered (file, _("Always inserted breakpoint mode is %s.\n"),
+		    value);
 }
 
 int
-breakpoints_always_inserted_mode (void)
+breakpoints_should_be_inserted_now (void)
 {
-  return (always_inserted_mode == AUTO_BOOLEAN_TRUE
-	  || (always_inserted_mode == AUTO_BOOLEAN_AUTO && non_stop));
+  if (gdbarch_has_global_breakpoints (target_gdbarch ()))
+    {
+      /* If breakpoints are global, they should be inserted even if no
+	 thread under gdb's control is running, or even if there are
+	 no threads under GDB's control yet.  */
+      return 1;
+    }
+  else if (target_has_execution)
+    {
+      struct thread_info *tp;
+
+      if (always_inserted_mode)
+	{
+	  /* The user wants breakpoints inserted even if all threads
+	     are stopped.  */
+	  return 1;
+	}
+
+      ALL_NON_EXITED_THREADS (tp)
+        {
+	  if (tp->executing)
+	    return 1;
+	}
+    }
+  return 0;
 }
 
 static const char condition_evaluation_both[] = "host or target";
@@ -12930,10 +12952,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			"a permanent breakpoint"));
     }
 
-  if (insert_mode == UGLL_INSERT
-      || (breakpoints_always_inserted_mode ()
-	  && (have_live_inferiors ()
-	      || (gdbarch_has_global_breakpoints (target_gdbarch ())))))
+  if (insert_mode == UGLL_INSERT || breakpoints_should_be_inserted_now ())
     {
       if (insert_mode != UGLL_DONT_INSERT)
 	insert_breakpoint_locations ();
@@ -17020,18 +17039,15 @@ a warning will be emitted for such breakpoints."),
 			   &breakpoint_set_cmdlist,
 			   &breakpoint_show_cmdlist);
 
-  add_setshow_auto_boolean_cmd ("always-inserted", class_support,
-				&always_inserted_mode, _("\
+  add_setshow_boolean_cmd ("always-inserted", class_support,
+			   &always_inserted_mode, _("\
 Set mode for inserting breakpoints."), _("\
 Show mode for inserting breakpoints."), _("\
-When this mode is off, breakpoints are inserted in inferior when it is\n\
-resumed, and removed when execution stops.  When this mode is on,\n\
-breakpoints are inserted immediately and removed only when the user\n\
-deletes the breakpoint.  When this mode is auto (which is the default),\n\
-the behaviour depends on the non-stop setting (see help set non-stop).\n\
-In this case, if gdb is controlling the inferior in non-stop mode, gdb\n\
-behaves as if always-inserted mode is on; if gdb is controlling the\n\
-inferior in all-stop mode, gdb behaves as if always-inserted mode is off."),
+When this mode is on, breakpoints are inserted immediately as soon as\n\
+they're created, kept inserted even when execution stops, and removed\n\
+only when the user deletes them.  When this mode is off (the default),\n\
+breakpoints are inserted only when execution continues, and removed\n\
+when execution stops."),
 				NULL,
 				&show_always_inserted_mode,
 				&breakpoint_set_cmdlist,
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 00c8802..e191c10 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1491,7 +1491,7 @@ extern void breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
 				    const gdb_byte *writebuf_org,
 				    ULONGEST memaddr, LONGEST len);
 
-extern int breakpoints_always_inserted_mode (void);
+extern int breakpoints_should_be_inserted_now (void);
 
 /* Called each time new event from target is processed.
    Retires previously deleted breakpoint locations that
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1bb1c0c..537fae8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3850,22 +3850,13 @@ This behavior can be controlled with the following commands::
 @item set breakpoint always-inserted off
 All breakpoints, including newly added by the user, are inserted in
 the target only when the target is resumed.  All breakpoints are
-removed from the target when it stops.
+removed from the target when it stops.  This is the default mode.
 
 @item set breakpoint always-inserted on
 Causes all breakpoints to be inserted in the target at all times.  If
 the user adds a new breakpoint, or changes an existing breakpoint, the
 breakpoints in the target are updated immediately.  A breakpoint is
-removed from the target only when breakpoint itself is removed.
-
-@cindex non-stop mode, and @code{breakpoint always-inserted}
-@item set breakpoint always-inserted auto
-This is the default mode.  If @value{GDBN} is controlling the inferior
-in non-stop mode (@pxref{Non-Stop Mode}), gdb behaves as if
-@code{breakpoint always-inserted} mode is on.  If @value{GDBN} is
-controlling the inferior in all-stop mode, @value{GDBN} behaves as if
-@code{breakpoint always-inserted} mode is off.
-@end table
+removed from the target only when breakpoint itself is deleted.
 
 @value{GDBN} handles conditional breakpoints by evaluating these conditions
 when a breakpoint breaks.  If the condition is true, then the process being
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c18267f..3725f2d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3448,8 +3448,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 	{
 	  /* Loading of shared libraries might have changed breakpoint
 	     addresses.  Make sure new breakpoints are inserted.  */
-	  if (stop_soon == NO_STOP_QUIETLY
-	      && !breakpoints_always_inserted_mode ())
+	  if (stop_soon == NO_STOP_QUIETLY)
 	    insert_breakpoints ();
 	  resume (0, GDB_SIGNAL_0);
 	  prepare_to_wait (ecs);
@@ -6110,7 +6109,7 @@ normal_stop (void)
       printf_filtered (_("No unwaited-for children left.\n"));
     }
 
-  if (!breakpoints_always_inserted_mode () && target_has_execution)
+  if (!breakpoints_should_be_inserted_now () && target_has_execution)
     {
       if (remove_breakpoints ())
 	{
diff --git a/gdb/remote.c b/gdb/remote.c
index 357e9f2..41ea012 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3604,9 +3604,8 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
      up.  */
   rs->starting_up = 0;
 
-  /* If breakpoints are global, insert them now.  */
-  if (gdbarch_has_global_breakpoints (target_gdbarch ())
-      && breakpoints_always_inserted_mode ())
+  /* Maybe breakpoints are global and need to be inserted now.  */
+  if (breakpoints_should_be_inserted_now ())
     insert_breakpoints ();
 }
 
diff --git a/gdb/testsuite/gdb.threads/break-while-running.c b/gdb/testsuite/gdb.threads/break-while-running.c
new file mode 100644
index 0000000..0e9e09a
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/break-while-running.c
@@ -0,0 +1,101 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+#include <pthread.h>
+#include <assert.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+pthread_barrier_t barrier;
+
+volatile int second_child;
+
+void
+breakpoint_function (void)
+{
+  /* GDB sets a breakpoint in this function.  */
+}
+
+void *
+child_function_0 (void *arg)
+{
+  volatile unsigned int counter = 1;
+
+  pthread_barrier_wait (&barrier);
+
+  while (counter > 0)
+    {
+      counter++;
+      usleep (1);
+
+      if (second_child)
+	continue;
+
+      breakpoint_function ();
+    }
+
+  pthread_exit (NULL);
+}
+
+void *
+child_function_1 (void *arg)
+{
+  volatile unsigned int counter = 1;
+
+  pthread_barrier_wait (&barrier);
+
+  while (counter > 0)
+    {
+      counter++;
+      usleep (1);
+
+      if (!second_thread)
+	continue;
+
+      breakpoint_function ();
+    }
+
+  pthread_exit (NULL);
+}
+
+static int
+wait_threads (void)
+{
+  return 1; /* in wait_threads */
+}
+
+int
+main (void)
+{
+  pthread_t child_thread[2];
+  int res;
+
+  pthread_barrier_init (&barrier, NULL, 3);
+
+  res = pthread_create (&child_thread[0], NULL, child_function_0, NULL);
+  assert (res == 0);
+  res = pthread_create (&child_thread[1], NULL, child_function_1, NULL);
+  assert (res == 0);
+
+  pthread_barrier_wait (&barrier);
+  wait_threads (); /* set wait-thread breakpoint here */
+
+  pthread_join (child_thread[0], NULL);
+  pthread_join (child_thread[1], NULL);
+
+  exit (EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.threads/break-while-running.exp b/gdb/testsuite/gdb.threads/break-while-running.exp
new file mode 100644
index 0000000..857541c
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/break-while-running.exp
@@ -0,0 +1,151 @@
+# Copyright (C) 2014 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/>.
+
+# Test that:
+#
+# - setting a breakpoint while a thread is running results in the
+#   breakpoint being inserted immediately.
+#
+# - if breakpoint always-inserted mode is off, GDB doesn't remove
+#   breakpoints from the target when a thread stops, if there are
+#   still threads running.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] == -1} {
+    return -1
+}
+
+# The test proper.  NON_STOP indicates whether we're testing in
+# non-stop, or all-stop mode.  ALWAYS_INSERTED indicates whether
+# testing in "breakpoint always-inserted" mode.
+
+proc test { always_inserted non_stop } {
+    global srcfile binfile
+    global gdb_prompt
+    global decimal
+
+    clean_restart $binfile
+
+    gdb_test_no_output "set non-stop $non_stop"
+    gdb_test_no_output "set breakpoint always-inserted $always_inserted"
+
+    if ![runto_main] {
+	return -1
+    }
+
+    # In all-stop, check whether we're testing with the remote or
+    # extended-remote targets.  If so, skip the tests, as with the
+    # RSP, we can't issue commands until the target replies to vCont.
+    # Not an issue with the non-stop RSP variant, which has a
+    # non-blocking vCont.
+    if {$non_stop=="off" && [gdb_is_target_remote]} {
+	return -1
+    }
+
+    gdb_breakpoint [gdb_get_line_number "set wait-thread breakpoint here"]
+    gdb_continue_to_breakpoint "run to wait-thread breakpoint"
+
+    delete_breakpoints
+
+    # Leave the main thread stopped, so GDB can poke at memory freely.
+    if {$non_stop == "off"} {
+	gdb_test_no_output "set scheduler-locking on"
+	gdb_test "thread 2" "Switching to .*"
+	gdb_test "continue &" "Continuing\." "continuing thread 2"
+	gdb_test "thread 3" "Switching to .*"
+	gdb_test "continue &" "Continuing\." "continuing thread 3"
+	gdb_test "thread 1" "Switching to .*" "switch back to main thread"
+    }
+
+    gdb_test "info threads" \
+	"\\\(running\\\).*\\\(running\\\).*main.* at .*" \
+	"only main stopped"
+
+    # Don't use gdb_test as it's racy in this case -- gdb_test matches
+    # the prompt with an end anchor.  Sometimes expect will manage to
+    # read the breakpoint hit output while still processing this test,
+    # defeating the anchor.
+    set test "set breakpoint while a thread is running"
+    gdb_test_multiple "break breakpoint_function" $test {
+	-re "Breakpoint $decimal at .*: file .*$srcfile.*\r\n$gdb_prompt " {
+	    pass $test
+	}
+	-re "$gdb_prompt " {
+	    fail $test
+	}
+    }
+
+    # Check that the breakpoint is hit.  Can't use gdb_test here, as
+    # no prompt is expected to come out.
+    set test "breakpoint is hit"
+    gdb_test_multiple "" $test {
+	-re "Breakpoint .*, breakpoint_function.*" {
+	    pass $test
+	}
+    }
+
+    if {$non_stop == "on"} {
+	gdb_test "info threads" \
+	    "\\\(running\\\).*breakpoint_function.*main.* at .*" \
+	    "one thread running"
+
+	# Unblock the other thread, which should then trip on the same
+	# breakpoint, unless GDB removed it by mistake.  Can't use
+	# gdb_test here for the same reasons as above.
+	set test "unblock second thread"
+	gdb_test_multiple "print second_child = 1" $test {
+	    -re " = 1\r\n$gdb_prompt " {
+		pass $test
+	    }
+	    -re "$gdb_prompt " {
+		fail $test
+	    }
+	}
+
+	set test "breakpoint on second child is hit"
+	gdb_test_multiple "" $test {
+	    -re "Breakpoint .*, breakpoint_function.*" {
+		pass $test
+	    }
+	}
+
+	gdb_test "info threads" \
+	    "breakpoint_function.*breakpoint_function.*main.* at .*" \
+	    "all threads stopped"
+    } else {
+	# This test is not merged with the non-stop one becase in
+	# all-stop we don't know where the other thread stops (inside
+	# usleep, for example).
+	set test "all threads stopped"
+	gdb_test_multiple "info threads" "$test" {
+	    -re "\\\(running\\\).*$gdb_prompt $" {
+		fail $test
+	    }
+	    -re "breakpoint_function.*main.*$gdb_prompt $" {
+		pass $test
+	    }
+	}
+    }
+}
+
+foreach always_inserted { "off" "on" } {
+    foreach non_stop { "off" "on" } {
+	set stop_mode [expr ($non_stop=="off")?"all-stop":"non-stop"]
+	with_test_prefix "always-inserted $always_inserted: $stop_mode" {
+	    test $always_inserted $non_stop
+	}
+    }
+}
-- 
1.9.3


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
  2014-09-21  9:45       ` [PATCH v4 " Pedro Alves
@ 2014-09-22  7:48         ` Yao Qi
  2014-09-22  9:13           ` Pedro Alves
  0 siblings, 1 reply; 12+ messages in thread
From: Yao Qi @ 2014-09-22  7:48 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On 09/21/2014 05:45 PM, Pedro Alves wrote:
> +void *
> +child_function_1 (void *arg)
> +{
> +  volatile unsigned int counter = 1;
> +
> +  pthread_barrier_wait (&barrier);
> +
> +  while (counter > 0)
> +    {
> +      counter++;
> +      usleep (1);
> +
> +      if (!second_thread)
> +	continue;

The code doesn't compile as second_thread isn't defined.  Is it
second_child?

Otherwise, this patch looks good to me.

-- 
Yao (齐尧)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
  2014-09-22  7:48         ` Yao Qi
@ 2014-09-22  9:13           ` Pedro Alves
  2014-09-23  8:31             ` Regression for gdb.mi/mi-nsintrall.exp [Re: [PATCH v4 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"] Jan Kratochvil
  0 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2014-09-22  9:13 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

On 09/22/2014 08:43 AM, Yao Qi wrote:
>> +
>> +      if (!second_thread)
>> +	continue;
> 
> The code doesn't compile as second_thread isn't defined.  Is it
> second_child?

Yes, sorry about that.  Last minute rename; I thought I had
tested afterwards...

> Otherwise, this patch looks good to me.

Thanks.  I pushed the series in.

Below's what I pushed for this patch.

-------
Subject: [PATCH] Fix "breakpoint always-inserted off"; remove "breakpoint
 always-inserted auto"

By default, GDB removes all breakpoints from the target when the
target stops and the prompt is given back to the user.  This is useful
in case GDB crashes while the user is interacting, as otherwise,
there's a higher chance breakpoints would be left planted on the
target.

But, as long as any thread is running free, we need to make sure to
keep breakpoints inserted, lest a thread misses a breakpoint.  With
that in mind, in preparation for non-stop mode, we added a "breakpoint
always-inserted on" mode.  This traded off the extra crash protection
for never having threads miss breakpoints, and in addition is more
efficient if there's a ton of breakpoints to remove/insert at each
user command (e.g., at each "step").

When we added non-stop mode, and for a period, we required users to
manually set "always-inserted on" when they enabled non-stop mode, as
otherwise GDB removes all breakpoints from the target as soon as any
thread stops, which means the other threads still running will miss
breakpoints.  The test added by this patch exercises this.

That soon revealed a nuisance, and so later we added an extra
"breakpoint always-inserted auto" mode, that made GDB behave like
"always-inserted on" when non-stop was enabled, and "always-inserted
off" when non-stop was disabled.  "auto" was made the default at the
same time.

In hindsight, this "auto" setting was unnecessary, and not the ideal
solution.  Non-stop mode does depends on breakpoints always-inserted
mode, but only as long as any thread is running.  If no thread is
running, no breakpoint can be missed.  The same is true for all-stop
too.  E.g., if, in all-stop mode, and the user does:

 (gdb) c&
 (gdb) b foo

That breakpoint at "foo" should be inserted immediately, but it
currently isn't -- currently it'll end up inserted only if the target
happens to trip on some event, and is re-resumed, e.g., an internal
breakpoint triggers that doesn't cause a user-visible stop, and so we
end up in keep_going calling insert_breakpoints.  The test added by
this patch also covers this.

IOW, no matter whether in non-stop or all-stop, if the target fully
stops, we can remove breakpoints.  And no matter whether in all-stop
or non-stop, if any thread is running in the target, then we need
breakpoints to be immediately inserted.  And then, if the target has
global breakpoints, we need to keep breakpoints even when the target
is stopped.

So with that in mind, and aiming at reducing all-stop vs non-stop
differences for all-stop-on-stop-of-non-stop, this patch fixes
"breakpoint always-inserted off" to not remove breakpoints from the
target until it fully stops, and then removes the "auto" setting as
unnecessary.  I propose removing it straight away rather than keeping
it as an alias, unless someone complains they have scripts that need
it and that can't adjust.

Tested on x86_64 Fedora 20.

gdb/
2014-09-22  Pedro Alves  <palves@redhat.com>

	* NEWS: Mention merge of "breakpoint always-inserted" modes "off"
	and "auto" merged.
	* breakpoint.c (enum ugll_insert_mode): New enum.
	(always_inserted_mode): Now a plain boolean.
	(show_always_inserted_mode): No longer handle AUTO_BOOLEAN_AUTO.
	(breakpoints_always_inserted_mode): Delete.
	(breakpoints_should_be_inserted_now): New function.
	(insert_breakpoints): Pass UGLL_INSERT to
	update_global_location_list instead of calling
	insert_breakpoint_locations manually.
	(create_solib_event_breakpoint_1): New, factored out from ...
	(create_solib_event_breakpoint): ... this.
	(create_and_insert_solib_event_breakpoint): Use
	create_solib_event_breakpoint_1 instead of calling
	insert_breakpoint_locations manually.
	(update_global_location_list): Change parameter type from boolean
	to enum ugll_insert_mode.  All callers adjusted.  Adjust to use
	breakpoints_should_be_inserted_now and handle UGLL_INSERT.
	(update_global_location_list_nothrow): Change parameter type from
	boolean to enum ugll_insert_mode.
	(_initialize_breakpoint): "breakpoint always-inserted" option is
	now a boolean command.  Update help text.
	* breakpoint.h (breakpoints_always_inserted_mode): Delete declaration.
	(breakpoints_should_be_inserted_now): New declaration.
	* infrun.c (handle_inferior_event) <TARGET_WAITKIND_LOADED>:
	Remove breakpoints_always_inserted_mode check.
	(normal_stop): Adjust to use breakpoints_should_be_inserted_now.
	* remote.c (remote_start_remote): Likewise.

gdb/doc/
2014-09-22  Pedro Alves  <palves@redhat.com>

	* gdb.texinfo (Set Breaks): Document that "set breakpoint
	always-inserted off" is the default mode now.  Delete
	documentation of "set breakpoint always-inserted auto".

gdb/testsuite/
2014-09-22  Pedro Alves  <palves@redhat.com>

	* gdb.threads/break-while-running.exp: New file.
	* gdb.threads/break-while-running.c: New file.
---
 gdb/ChangeLog                                     |  31 +++++
 gdb/doc/ChangeLog                                 |   6 +
 gdb/testsuite/ChangeLog                           |   5 +
 gdb/NEWS                                          |   7 +
 gdb/breakpoint.c                                  |  90 +++++++------
 gdb/breakpoint.h                                  |   2 +-
 gdb/doc/gdb.texinfo                               |  13 +-
 gdb/infrun.c                                      |   5 +-
 gdb/remote.c                                      |   5 +-
 gdb/testsuite/gdb.threads/break-while-running.c   | 101 +++++++++++++++
 gdb/testsuite/gdb.threads/break-while-running.exp | 151 ++++++++++++++++++++++
 11 files changed, 361 insertions(+), 55 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/break-while-running.c
 create mode 100644 gdb/testsuite/gdb.threads/break-while-running.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b91dc72..90a991d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,36 @@
 2014-09-22  Pedro Alves  <palves@redhat.com>
 
+	* NEWS: Mention merge of "breakpoint always-inserted" modes "off"
+	and "auto" merged.
+	* breakpoint.c (enum ugll_insert_mode): New enum.
+	(always_inserted_mode): Now a plain boolean.
+	(show_always_inserted_mode): No longer handle AUTO_BOOLEAN_AUTO.
+	(breakpoints_always_inserted_mode): Delete.
+	(breakpoints_should_be_inserted_now): New function.
+	(insert_breakpoints): Pass UGLL_INSERT to
+	update_global_location_list instead of calling
+	insert_breakpoint_locations manually.
+	(create_solib_event_breakpoint_1): New, factored out from ...
+	(create_solib_event_breakpoint): ... this.
+	(create_and_insert_solib_event_breakpoint): Use
+	create_solib_event_breakpoint_1 instead of calling
+	insert_breakpoint_locations manually.
+	(update_global_location_list): Change parameter type from boolean
+	to enum ugll_insert_mode.  All callers adjusted.  Adjust to use
+	breakpoints_should_be_inserted_now and handle UGLL_INSERT.
+	(update_global_location_list_nothrow): Change parameter type from
+	boolean to enum ugll_insert_mode.
+	(_initialize_breakpoint): "breakpoint always-inserted" option is
+	now a boolean command.  Update help text.
+	* breakpoint.h (breakpoints_always_inserted_mode): Delete declaration.
+	(breakpoints_should_be_inserted_now): New declaration.
+	* infrun.c (handle_inferior_event) <TARGET_WAITKIND_LOADED>:
+	Remove breakpoints_always_inserted_mode check.
+	(normal_stop): Adjust to use breakpoints_should_be_inserted_now.
+	* remote.c (remote_start_remote): Likewise.
+
+2014-09-22  Pedro Alves  <palves@redhat.com>
+
 	* breakpoint.c (enum ugll_insert_mode): Add UGLL_INSERT.
 	(insert_breakpoints): Don't call insert_breakpoint_locations here.
 	Instead, pass UGLL_INSERT to update_global_location_list.
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index b5fac5a..d77a80e 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,9 @@
+2014-09-22  Pedro Alves  <palves@redhat.com>
+
+	* gdb.texinfo (Set Breaks): Document that "set breakpoint
+	always-inserted off" is the default mode now.  Delete
+	documentation of "set breakpoint always-inserted auto".
+
 2014-09-18  Doug Evans  <dje@google.com>
 
 	* python.texi (Symbol Tables In Python): Document "producer"
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 67c7f82..539a983 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-09-22  Pedro Alves  <palves@redhat.com>
+
+	* gdb.threads/break-while-running.exp: New file.
+	* gdb.threads/break-while-running.c: New file.
+
 2014-09-19  Yao Qi  <yao@codesourcery.com>
 
 	* gdb.dwarf2/dw2-var-zero-addr.exp: Move test into new proc test.
diff --git a/gdb/NEWS b/gdb/NEWS
index 82ee57b..11326f1 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -33,6 +33,13 @@ queue-signal signal-name-or-number
   confirmation if the program had stopped for a signal and the user
   switched threads meanwhile.
 
+* "breakpoint always-inserted" modes "off" and "auto" merged.
+
+  Now, when 'breakpoint always-inserted mode' is set to "off", GDB
+  won't remove breakpoints from the target until all threads stop,
+  even in non-stop mode.  The "auto" mode has been removed, and "off"
+  is now the default mode.
+
 *** Changes in GDB 7.8
 
 * New command line options
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3f372de..3675b4f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -258,12 +258,17 @@ enum ugll_insert_mode
      the inferior.  */
   UGLL_DONT_INSERT,
 
-  /* May insert breakpoints if breakpoints_always_inserted_mode is
-     true.  */
+  /* May insert breakpoints iff breakpoints_should_be_inserted_now
+     claims breakpoints should be inserted now.  */
   UGLL_MAY_INSERT,
 
-  /* Insert locations now, even if breakpoints_always_inserted_mode is
-     false.  */
+  /* Insert locations now, irrespective of
+     breakpoints_should_be_inserted_now.  E.g., say all threads are
+     stopped right now, and the user did "continue".  We need to
+     insert breakpoints _before_ resuming the target, but
+     UGLL_MAY_INSERT wouldn't insert them, because
+     breakpoints_should_be_inserted_now returns false at that point,
+     as no thread is running yet.  */
   UGLL_INSERT
 };
 
@@ -451,34 +456,51 @@ show_automatic_hardware_breakpoints (struct ui_file *file, int from_tty,
 		    value);
 }
 
-/* If on, gdb will keep breakpoints inserted even as inferior is
-   stopped, and immediately insert any new breakpoints.  If off, gdb
-   will insert breakpoints into inferior only when resuming it, and
-   will remove breakpoints upon stop.  If auto, GDB will behave as ON
-   if in non-stop mode, and as OFF if all-stop mode.*/
-
-static enum auto_boolean always_inserted_mode = AUTO_BOOLEAN_AUTO;
+/* If on, GDB keeps breakpoints inserted even if the inferior is
+   stopped, and immediately inserts any new breakpoints as soon as
+   they're created.  If off (default), GDB keeps breakpoints off of
+   the target as long as possible.  That is, it delays inserting
+   breakpoints until the next resume, and removes them again when the
+   target fully stops.  This is a bit safer in case GDB crashes while
+   processing user input.  */
+static int always_inserted_mode = 0;
 
 static void
 show_always_inserted_mode (struct ui_file *file, int from_tty,
 		     struct cmd_list_element *c, const char *value)
 {
-  if (always_inserted_mode == AUTO_BOOLEAN_AUTO)
-    fprintf_filtered (file,
-		      _("Always inserted breakpoint "
-			"mode is %s (currently %s).\n"),
-		      value,
-		      breakpoints_always_inserted_mode () ? "on" : "off");
-  else
-    fprintf_filtered (file, _("Always inserted breakpoint mode is %s.\n"),
-		      value);
+  fprintf_filtered (file, _("Always inserted breakpoint mode is %s.\n"),
+		    value);
 }
 
 int
-breakpoints_always_inserted_mode (void)
+breakpoints_should_be_inserted_now (void)
 {
-  return (always_inserted_mode == AUTO_BOOLEAN_TRUE
-	  || (always_inserted_mode == AUTO_BOOLEAN_AUTO && non_stop));
+  if (gdbarch_has_global_breakpoints (target_gdbarch ()))
+    {
+      /* If breakpoints are global, they should be inserted even if no
+	 thread under gdb's control is running, or even if there are
+	 no threads under GDB's control yet.  */
+      return 1;
+    }
+  else if (target_has_execution)
+    {
+      struct thread_info *tp;
+
+      if (always_inserted_mode)
+	{
+	  /* The user wants breakpoints inserted even if all threads
+	     are stopped.  */
+	  return 1;
+	}
+
+      ALL_NON_EXITED_THREADS (tp)
+        {
+	  if (tp->executing)
+	    return 1;
+	}
+    }
+  return 0;
 }
 
 static const char condition_evaluation_both[] = "host or target";
@@ -12930,10 +12952,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			"a permanent breakpoint"));
     }
 
-  if (insert_mode == UGLL_INSERT
-      || (breakpoints_always_inserted_mode ()
-	  && (have_live_inferiors ()
-	      || (gdbarch_has_global_breakpoints (target_gdbarch ())))))
+  if (insert_mode == UGLL_INSERT || breakpoints_should_be_inserted_now ())
     {
       if (insert_mode != UGLL_DONT_INSERT)
 	insert_breakpoint_locations ();
@@ -17020,18 +17039,15 @@ a warning will be emitted for such breakpoints."),
 			   &breakpoint_set_cmdlist,
 			   &breakpoint_show_cmdlist);
 
-  add_setshow_auto_boolean_cmd ("always-inserted", class_support,
-				&always_inserted_mode, _("\
+  add_setshow_boolean_cmd ("always-inserted", class_support,
+			   &always_inserted_mode, _("\
 Set mode for inserting breakpoints."), _("\
 Show mode for inserting breakpoints."), _("\
-When this mode is off, breakpoints are inserted in inferior when it is\n\
-resumed, and removed when execution stops.  When this mode is on,\n\
-breakpoints are inserted immediately and removed only when the user\n\
-deletes the breakpoint.  When this mode is auto (which is the default),\n\
-the behaviour depends on the non-stop setting (see help set non-stop).\n\
-In this case, if gdb is controlling the inferior in non-stop mode, gdb\n\
-behaves as if always-inserted mode is on; if gdb is controlling the\n\
-inferior in all-stop mode, gdb behaves as if always-inserted mode is off."),
+When this mode is on, breakpoints are inserted immediately as soon as\n\
+they're created, kept inserted even when execution stops, and removed\n\
+only when the user deletes them.  When this mode is off (the default),\n\
+breakpoints are inserted only when execution continues, and removed\n\
+when execution stops."),
 				NULL,
 				&show_always_inserted_mode,
 				&breakpoint_set_cmdlist,
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 00c8802..e191c10 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1491,7 +1491,7 @@ extern void breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
 				    const gdb_byte *writebuf_org,
 				    ULONGEST memaddr, LONGEST len);
 
-extern int breakpoints_always_inserted_mode (void);
+extern int breakpoints_should_be_inserted_now (void);
 
 /* Called each time new event from target is processed.
    Retires previously deleted breakpoint locations that
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1bb1c0c..537fae8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3850,22 +3850,13 @@ This behavior can be controlled with the following commands::
 @item set breakpoint always-inserted off
 All breakpoints, including newly added by the user, are inserted in
 the target only when the target is resumed.  All breakpoints are
-removed from the target when it stops.
+removed from the target when it stops.  This is the default mode.
 
 @item set breakpoint always-inserted on
 Causes all breakpoints to be inserted in the target at all times.  If
 the user adds a new breakpoint, or changes an existing breakpoint, the
 breakpoints in the target are updated immediately.  A breakpoint is
-removed from the target only when breakpoint itself is removed.
-
-@cindex non-stop mode, and @code{breakpoint always-inserted}
-@item set breakpoint always-inserted auto
-This is the default mode.  If @value{GDBN} is controlling the inferior
-in non-stop mode (@pxref{Non-Stop Mode}), gdb behaves as if
-@code{breakpoint always-inserted} mode is on.  If @value{GDBN} is
-controlling the inferior in all-stop mode, @value{GDBN} behaves as if
-@code{breakpoint always-inserted} mode is off.
-@end table
+removed from the target only when breakpoint itself is deleted.
 
 @value{GDBN} handles conditional breakpoints by evaluating these conditions
 when a breakpoint breaks.  If the condition is true, then the process being
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c18267f..3725f2d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3448,8 +3448,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 	{
 	  /* Loading of shared libraries might have changed breakpoint
 	     addresses.  Make sure new breakpoints are inserted.  */
-	  if (stop_soon == NO_STOP_QUIETLY
-	      && !breakpoints_always_inserted_mode ())
+	  if (stop_soon == NO_STOP_QUIETLY)
 	    insert_breakpoints ();
 	  resume (0, GDB_SIGNAL_0);
 	  prepare_to_wait (ecs);
@@ -6110,7 +6109,7 @@ normal_stop (void)
       printf_filtered (_("No unwaited-for children left.\n"));
     }
 
-  if (!breakpoints_always_inserted_mode () && target_has_execution)
+  if (!breakpoints_should_be_inserted_now () && target_has_execution)
     {
       if (remove_breakpoints ())
 	{
diff --git a/gdb/remote.c b/gdb/remote.c
index 357e9f2..41ea012 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3604,9 +3604,8 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
      up.  */
   rs->starting_up = 0;
 
-  /* If breakpoints are global, insert them now.  */
-  if (gdbarch_has_global_breakpoints (target_gdbarch ())
-      && breakpoints_always_inserted_mode ())
+  /* Maybe breakpoints are global and need to be inserted now.  */
+  if (breakpoints_should_be_inserted_now ())
     insert_breakpoints ();
 }
 
diff --git a/gdb/testsuite/gdb.threads/break-while-running.c b/gdb/testsuite/gdb.threads/break-while-running.c
new file mode 100644
index 0000000..20695ee
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/break-while-running.c
@@ -0,0 +1,101 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+#include <pthread.h>
+#include <assert.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+pthread_barrier_t barrier;
+
+volatile int second_child;
+
+void
+breakpoint_function (void)
+{
+  /* GDB sets a breakpoint in this function.  */
+}
+
+void *
+child_function_0 (void *arg)
+{
+  volatile unsigned int counter = 1;
+
+  pthread_barrier_wait (&barrier);
+
+  while (counter > 0)
+    {
+      counter++;
+      usleep (1);
+
+      if (second_child)
+	continue;
+
+      breakpoint_function ();
+    }
+
+  pthread_exit (NULL);
+}
+
+void *
+child_function_1 (void *arg)
+{
+  volatile unsigned int counter = 1;
+
+  pthread_barrier_wait (&barrier);
+
+  while (counter > 0)
+    {
+      counter++;
+      usleep (1);
+
+      if (!second_child)
+	continue;
+
+      breakpoint_function ();
+    }
+
+  pthread_exit (NULL);
+}
+
+static int
+wait_threads (void)
+{
+  return 1; /* in wait_threads */
+}
+
+int
+main (void)
+{
+  pthread_t child_thread[2];
+  int res;
+
+  pthread_barrier_init (&barrier, NULL, 3);
+
+  res = pthread_create (&child_thread[0], NULL, child_function_0, NULL);
+  assert (res == 0);
+  res = pthread_create (&child_thread[1], NULL, child_function_1, NULL);
+  assert (res == 0);
+
+  pthread_barrier_wait (&barrier);
+  wait_threads (); /* set wait-thread breakpoint here */
+
+  pthread_join (child_thread[0], NULL);
+  pthread_join (child_thread[1], NULL);
+
+  exit (EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.threads/break-while-running.exp b/gdb/testsuite/gdb.threads/break-while-running.exp
new file mode 100644
index 0000000..690d6ab
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/break-while-running.exp
@@ -0,0 +1,151 @@
+# Copyright (C) 2014 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/>.
+
+# Test that:
+#
+# - setting a breakpoint while a thread is running results in the
+#   breakpoint being inserted immediately.
+#
+# - if breakpoint always-inserted mode is off, GDB doesn't remove
+#   breakpoints from the target when a thread stops, if there are
+#   still threads running.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] == -1} {
+    return -1
+}
+
+# The test proper.  NON_STOP indicates whether we're testing in
+# non-stop, or all-stop mode.  ALWAYS_INSERTED indicates whether
+# testing in "breakpoint always-inserted" mode.
+
+proc test { always_inserted non_stop } {
+    global srcfile binfile
+    global gdb_prompt
+    global decimal
+
+    clean_restart $binfile
+
+    gdb_test_no_output "set non-stop $non_stop"
+    gdb_test_no_output "set breakpoint always-inserted $always_inserted"
+
+    if ![runto_main] {
+	return -1
+    }
+
+    # In all-stop, check whether we're testing with the remote or
+    # extended-remote targets.  If so, skip the tests, as with the
+    # RSP, we can't issue commands until the target replies to vCont.
+    # Not an issue with the non-stop RSP variant, which has a
+    # non-blocking vCont.
+    if {$non_stop=="off" && [gdb_is_target_remote]} {
+	return -1
+    }
+
+    gdb_breakpoint [gdb_get_line_number "set wait-thread breakpoint here"]
+    gdb_continue_to_breakpoint "run to wait-thread breakpoint"
+
+    delete_breakpoints
+
+    # Leave the main thread stopped, so GDB can poke at memory freely.
+    if {$non_stop == "off"} {
+	gdb_test_no_output "set scheduler-locking on"
+	gdb_test "thread 2" "Switching to .*"
+	gdb_test "continue &" "Continuing\." "continuing thread 2"
+	gdb_test "thread 3" "Switching to .*"
+	gdb_test "continue &" "Continuing\." "continuing thread 3"
+	gdb_test "thread 1" "Switching to .*" "switch back to main thread"
+    }
+
+    gdb_test "info threads" \
+	"\\\(running\\\).*\\\(running\\\).* main .*" \
+	"only main stopped"
+
+    # Don't use gdb_test as it's racy in this case -- gdb_test matches
+    # the prompt with an end anchor.  Sometimes expect will manage to
+    # read the breakpoint hit output while still processing this test,
+    # defeating the anchor.
+    set test "set breakpoint while a thread is running"
+    gdb_test_multiple "break breakpoint_function" $test {
+	-re "Breakpoint $decimal at .*: file .*$srcfile.*\r\n$gdb_prompt " {
+	    pass $test
+	}
+	-re "$gdb_prompt " {
+	    fail $test
+	}
+    }
+
+    # Check that the breakpoint is hit.  Can't use gdb_test here, as
+    # no prompt is expected to come out.
+    set test "breakpoint is hit"
+    gdb_test_multiple "" $test {
+	-re "Breakpoint .*, breakpoint_function \[^\r\n\]+" {
+	    pass $test
+	}
+    }
+
+    if {$non_stop == "on"} {
+	gdb_test "info threads" \
+	    "\\\(running\\\).* breakpoint_function .* main .*" \
+	    "one thread running"
+
+	# Unblock the other thread, which should then trip on the same
+	# breakpoint, unless GDB removed it by mistake.  Can't use
+	# gdb_test here for the same reasons as above.
+	set test "unblock second thread"
+	gdb_test_multiple "print second_child = 1" $test {
+	    -re " = 1\r\n$gdb_prompt " {
+		pass $test
+	    }
+	    -re "$gdb_prompt " {
+		fail $test
+	    }
+	}
+
+	set test "breakpoint on second child is hit"
+	gdb_test_multiple "" $test {
+	    -re "Breakpoint .*, breakpoint_function \[^\r\n\]+" {
+		pass $test
+	    }
+	}
+
+	gdb_test "info threads" \
+	    " breakpoint_function .* breakpoint_function .* main .*" \
+	    "all threads stopped"
+    } else {
+	# This test is not merged with the non-stop one because in
+	# all-stop we don't know where the other thread stops (inside
+	# usleep, for example).
+	set test "all threads stopped"
+	gdb_test_multiple "info threads" "$test" {
+	    -re "\\\(running\\\).*$gdb_prompt $" {
+		fail $test
+	    }
+	    -re "breakpoint_function .* main .*$gdb_prompt $" {
+		pass $test
+	    }
+	}
+    }
+}
+
+foreach always_inserted { "off" "on" } {
+    foreach non_stop { "off" "on" } {
+	set stop_mode [expr ($non_stop=="off")?"all-stop":"non-stop"]
+	with_test_prefix "always-inserted $always_inserted: $stop_mode" {
+	    test $always_inserted $non_stop
+	}
+    }
+}
-- 
1.9.3


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Regression for gdb.mi/mi-nsintrall.exp  [Re: [PATCH v4 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"]
  2014-09-22  9:13           ` Pedro Alves
@ 2014-09-23  8:31             ` Jan Kratochvil
  2014-09-23 16:27               ` Pedro Alves
  0 siblings, 1 reply; 12+ messages in thread
From: Jan Kratochvil @ 2014-09-23  8:31 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Yao Qi, gdb-patches

On Mon, 22 Sep 2014 11:13:16 +0200, Pedro Alves wrote:
> Below's what I pushed for this patch.

a25a5a45ef9340eb9a435f9b70599a90bb142658 is the first bad commit
commit a25a5a45ef9340eb9a435f9b70599a90bb142658
Author: Pedro Alves <palves@redhat.com>
Date:   Mon Sep 22 09:56:55 2014 +0100
    Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"

Tested in gdbserver mode with gdb.mi/mi-nsintrall.exp on Fedora 20 x86_64.

It FAILs under 10 runs.


Jan


 PASS: gdb.mi/mi-nsintrall.exp: stop 4
 mi_expect_stop: expecting: \*stopped,reason="breakpoint-hit",disp="keep",bkptno="[0-9]+",frame={addr="0x[0-9A-Fa-f]+",func="thread_function",args=\[[^
 ]*\],(?:file="[^
 ]*nsintrall.c",fullname="(/[^\n]*/|\\\\[^\\]+\\[^\n]+\\|\\[^\\][^\n]*\\|[a-zA-Z]:[^\n]*\\)nsintrall.c",line="[0-9]*"|from="nsintrall.c")},thread-id="[0-9]+",stopped-threads=[^
 ]*^M
 (=thread-selected,id="[0-9]+"^M
 |=(?:breakpoint-created|breakpoint-deleted)[^
 ]+"^M
 )*
-=breakpoint-modified,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x000000000040077a",func="thread_function",file="./gdb.mi/nsintrall.c",fullname="/home/jkratoch/redhat/gdb-clean/gdb/testsuite/gdb.mi/nsintrall.c",line="52",thread-groups=["i1"],times="6",original-location="thread_function"}^M
-~"\nBreakpoint "^M
-~"2, thread_function (arg=0x0) at ./gdb.mi/nsintrall.c:52\n"^M
+=thread-created,id="6",group-id="i1"^M
+~"[New Thread 1120]\n"^M
+~"\nProgram received signal "^M
+~"SIGTRAP, Trace/breakpoint trap.\n"^M
+~"0x000000000040077b in thread_function (arg=0x5) at ./gdb.mi/nsintrall.c:52\n"^M
 ~"52\t    int my_number =  (long) arg;\n"^M
-*stopped,reason="breakpoint-hit",disp="keep",bkptno="2",frame={addr="0x000000000040077a",func="thread_function",args=[{name="arg",value="0x0"}],file="./gdb.mi/nsintrall.c",fullname="/home/jkratoch/redhat/gdb-clean/gdb/testsuite/gdb.mi/nsintrall.c",line="52"},thread-id="1",stopped-threads=["1"],core="4"^M
-PASS: gdb.mi/mi-nsintrall.exp: stop 5
+*stopped,reason="signal-received",signal-name="SIGTRAP",signal-meaning="Trace/breakpoint trap",frame={addr="0x000000000040077b",func="thread_function",args=[{name="arg",value="0x5"}],file="./gdb.mi/nsintrall.c",fullname="/home/jkratoch/redhat/gdb-clean/gdb/testsuite/gdb.mi/nsintrall.c",line="52"},thread-id="6",stopped-threads=["6"],core="7"^M
+FAIL: gdb.mi/mi-nsintrall.exp: stop 5 (timeout)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Regression for gdb.mi/mi-nsintrall.exp  [Re: [PATCH v4 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"]
  2014-09-23  8:31             ` Regression for gdb.mi/mi-nsintrall.exp [Re: [PATCH v4 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"] Jan Kratochvil
@ 2014-09-23 16:27               ` Pedro Alves
  2014-09-25 15:00                 ` Pedro Alves
  0 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2014-09-23 16:27 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Yao Qi, gdb-patches

On 09/23/2014 09:31 AM, Jan Kratochvil wrote:
> On Mon, 22 Sep 2014 11:13:16 +0200, Pedro Alves wrote:
>> Below's what I pushed for this patch.
> 
> a25a5a45ef9340eb9a435f9b70599a90bb142658 is the first bad commit
> commit a25a5a45ef9340eb9a435f9b70599a90bb142658
> Author: Pedro Alves <palves@redhat.com>
> Date:   Mon Sep 22 09:56:55 2014 +0100
>     Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
> 
> Tested in gdbserver mode with gdb.mi/mi-nsintrall.exp on Fedora 20 x86_64.
> 
> It FAILs under 10 runs.

I can reproduce it.  Looking.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Regression for gdb.mi/mi-nsintrall.exp  [Re: [PATCH v4 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"]
  2014-09-23 16:27               ` Pedro Alves
@ 2014-09-25 15:00                 ` Pedro Alves
  0 siblings, 0 replies; 12+ messages in thread
From: Pedro Alves @ 2014-09-25 15:00 UTC (permalink / raw)
  To: Pedro Alves, Jan Kratochvil; +Cc: Yao Qi, gdb-patches

On 09/23/2014 05:27 PM, Pedro Alves wrote:
> On 09/23/2014 09:31 AM, Jan Kratochvil wrote:
>> On Mon, 22 Sep 2014 11:13:16 +0200, Pedro Alves wrote:
>>> Below's what I pushed for this patch.
>>
>> a25a5a45ef9340eb9a435f9b70599a90bb142658 is the first bad commit
>> commit a25a5a45ef9340eb9a435f9b70599a90bb142658
>> Author: Pedro Alves <palves@redhat.com>
>> Date:   Mon Sep 22 09:56:55 2014 +0100
>>     Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"
>>
>> Tested in gdbserver mode with gdb.mi/mi-nsintrall.exp on Fedora 20 x86_64.
>>
>> It FAILs under 10 runs.
> 
> I can reproduce it.  Looking.

Fix is here:

 https://sourceware.org/ml/gdb-patches/2014-09/msg00732.html

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2014-09-25 15:00 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-17 22:47 [PATCH v2 0/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto" Pedro Alves
2014-09-17 22:47 ` [PATCH v2 2/3] Tell update_global_location_list to insert breakpoints Pedro Alves
2014-09-17 22:47 ` [PATCH v2 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto" Pedro Alves
2014-09-18  1:49   ` Yao Qi
2014-09-19 19:05     ` [PATCH v3 " Pedro Alves
2014-09-21  9:45       ` [PATCH v4 " Pedro Alves
2014-09-22  7:48         ` Yao Qi
2014-09-22  9:13           ` Pedro Alves
2014-09-23  8:31             ` Regression for gdb.mi/mi-nsintrall.exp [Re: [PATCH v4 3/3] Fix "breakpoint always-inserted off"; remove "breakpoint always-inserted auto"] Jan Kratochvil
2014-09-23 16:27               ` Pedro Alves
2014-09-25 15:00                 ` Pedro Alves
2014-09-17 22:47 ` [PATCH v2 1/3] Change parameter type of update_global_location_list from boolean to enum Pedro Alves

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).