public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [RFC 4/6] Introduce run_on_main_thread
  2019-03-09 17:23 [RFC 0/6] Demangle minimal symbol names in worker threads Tom Tromey
                   ` (4 preceding siblings ...)
  2019-03-09 17:23 ` [RFC 6/6] Demangle minsyms in parallel Tom Tromey
@ 2019-03-09 17:23 ` Tom Tromey
  2019-03-09 18:09 ` [RFC 0/6] Demangle minimal symbol names in worker threads Eli Zaretskii
  2019-03-11 17:26 ` John Baldwin
  7 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-03-09 17:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

This introduces a way for a callback to be run on the main thread.

2019-03-08  Tom Tromey  <tom@tromey.com>

	* ser-event.h (run_on_main_thread): Declare.
	* ser-event.c (runnable_event, runnables, runnable_mutex): New
	globals.
	(run_events, run_on_main_thread, _initialize_ser_event): New
	functions.
---
 gdb/ChangeLog   |  8 +++++++
 gdb/ser-event.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/ser-event.h |  6 +++++
 3 files changed, 73 insertions(+)

diff --git a/gdb/ser-event.c b/gdb/ser-event.c
index d3956346246..821b2f169df 100644
--- a/gdb/ser-event.c
+++ b/gdb/ser-event.c
@@ -20,6 +20,8 @@
 #include "ser-event.h"
 #include "serial.h"
 #include "common/filestuff.h"
+#include <mutex>
+#include "event-loop.h"
 
 /* On POSIX hosts, a serial_event is basically an abstraction for the
    classical self-pipe trick.
@@ -217,3 +219,60 @@ serial_event_clear (struct serial_event *event)
   ResetEvent (state->event);
 #endif
 }
+
+\f
+
+/* The serial event used when posting runnables.  */
+
+static struct serial_event *runnable_event;
+
+/* Runnables that have been posted.  */
+
+static std::vector<std::function<void ()>> runnables;
+
+/* Mutex to hold when handling runnable_event or runnables.  */
+
+static std::mutex runnable_mutex;
+
+/* Run all the queued runnables.  */
+
+static void
+run_events (int error, gdb_client_data client_data)
+{
+  std::vector<std::function<void ()>> local;
+
+  /* Hold the lock while changing the globals, but not while running
+     the runnables.  */
+  {
+    std::lock_guard<std::mutex> lock (runnable_mutex);
+
+    /* Clear the event fd.  Do this before flushing the events list,
+       so that any new event post afterwards is sure to re-awaken the
+       event loop.  */
+    serial_event_clear (runnable_event);
+
+    /* Move the vector in case running a runnable pushes a new
+       runnable.  */
+    std::swap (local, runnables);
+  }
+
+  for (auto &item : local)
+    item ();
+}
+
+/* See ser-event.h.  */
+
+void
+run_on_main_thread (std::function<void ()> &&func)
+{
+  std::lock_guard<std::mutex> lock (runnable_mutex);
+  runnables.emplace_back (std::move (func));
+  serial_event_set (runnable_event);
+}
+
+void
+_initialize_ser_event ()
+{
+  runnable_event = make_serial_event ();
+  add_file_handler (serial_event_fd (runnable_event), run_events, nullptr);
+}
diff --git a/gdb/ser-event.h b/gdb/ser-event.h
index 137348557f9..61a84f9cc79 100644
--- a/gdb/ser-event.h
+++ b/gdb/ser-event.h
@@ -19,6 +19,8 @@
 #ifndef SER_EVENT_H
 #define SER_EVENT_H
 
+#include <functional>
+
 /* This is used to be able to signal the event loop (or any other
    select/poll) of events, in a race-free manner.
 
@@ -48,4 +50,8 @@ extern void serial_event_set (struct serial_event *event);
    call is made.  */
 extern void serial_event_clear (struct serial_event *event);
 
+/* Send a runnable to the main thread.  */
+
+extern void run_on_main_thread (std::function<void ()> &&);
+
 #endif
-- 
2.17.2

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

* [RFC 0/6] Demangle minimal symbol names in worker threads
@ 2019-03-09 17:23 Tom Tromey
  2019-03-09 17:23 ` [RFC 2/6] Remove static buffer from ada_decode Tom Tromey
                   ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Tom Tromey @ 2019-03-09 17:23 UTC (permalink / raw)
  To: gdb-patches

I've thought for a while that gdb should take advantage of multiple
cores in order to speed up its processing.  This series is some
initial work in that direction.

In particular, this patch arranges to do the demangling for minimal
symbols in worker threads.  I chose this because it seemed relatively
simple to reason about, as the demangler is already mostly thread-safe
(except, as it turns out, the Ada demangler, which is fixed in this
series).  It isn't actually a very important thing to speed up, as
minimal symbol reading is already reasonably speedy; but I thought it
best to start with something straightforward to facilitate flushing
out thread safety bugs.

That said, this does yield a modest speedup on the largest .so on my
system:

A typical pre-patch run looks like:

    /bin/time ./gdb -nx --batch --data-directory ./data-directory/ /usr/lib64/firefox/libxul.so 
    1.22user 0.21system 0:01.46elapsed 98%CPU (0avgtext+0avgdata 188204maxresident)k

And with the patches:

    1.47user 0.39system 0:01.11elapsed 167%CPU (0avgtext+0avgdata 206380maxresident)k

I've only seen %CPU up to 185% or so; but since this is a 4-core
machine I suppose that means there are still gains to be had
somewhere?

This implementation adds a lock to the demangled hash table code.  I
tried a different implementation which did not add locking here.  In
that implementation, minsym names were simply not entered into the
hash table and thus not shared.  However, the lock turned out to be no
slower, and I believe the hash table saves some memory, so I decided
on this approach.

One possible enhancement in this area would be call
build_minimal_symbol_hash_tables in a background thread.  This is
trickier than it sounds, though.  First, it would require more
locking, so that gdb would not attempt to use the minsym hash tables
before they were constructed.  Second, it is possible to introduce
races where the objfile is deleted while the worker thread is still
running.

I have some more ideas for areas in gdb that could benefit from
threads.  I'm happy to discuss if you're interested.

Because threads are tricky, I think this requires some extra scrutiny.
Also I've done various tests using helgrind, which pointed out some of
the things fixed in this series.

You can't readily apply this, because it depends on both the cleanup
removal series and the minsym improvement series.  However it is in my
github repository on the branch "t/parallel-minsyms-mutex".

Tested by the buildbot.

Tom


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

* [RFC 3/6] Lock the demangled hash table
  2019-03-09 17:23 [RFC 0/6] Demangle minimal symbol names in worker threads Tom Tromey
                   ` (2 preceding siblings ...)
  2019-03-09 17:23 ` [RFC 1/6] Defer minimal symbol name-setting Tom Tromey
@ 2019-03-09 17:23 ` Tom Tromey
  2019-03-09 17:23 ` [RFC 6/6] Demangle minsyms in parallel Tom Tromey
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-03-09 17:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

This introduces a lock that is used when modifying the demangled hash
table.

2019-03-03  Tom Tromey  <tom@tromey.com>

	* symtab.c (demangled_mutex): New global.
	(symbol_set_names): Use a lock_guard.
---
 gdb/ChangeLog |   5 ++
 gdb/symtab.c  | 128 +++++++++++++++++++++++++++-----------------------
 2 files changed, 75 insertions(+), 58 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 449bc4cd2b3..0dcdc1e8b01 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -69,6 +69,7 @@
 #include "arch-utils.h"
 #include <algorithm>
 #include "common/pathstuff.h"
+#include <mutex>
 
 /* Forward declarations for local functions.  */
 
@@ -697,6 +698,9 @@ symbol_set_language (struct general_symbol_info *gsymbol,
 
 /* Functions to initialize a symbol's mangled name.  */
 
+/* Mutex that is used when modifying the demangled hash table.  */
+static std::mutex demangled_mutex;
+
 /* Objects of this type are stored in the demangled name hash table.  */
 struct demangled_name_entry
 {
@@ -825,9 +829,6 @@ symbol_set_names (struct general_symbol_info *gsymbol,
       return;
     }
 
-  if (per_bfd->demangled_names_hash == NULL)
-    create_demangled_names_hash (per_bfd);
-
   if (linkage_name[len] != '\0')
     {
       char *alloc_name;
@@ -846,64 +847,75 @@ symbol_set_names (struct general_symbol_info *gsymbol,
     = symbol_find_demangled_name (gsymbol, linkage_name_copy);
   gdb::unique_xmalloc_ptr<char> demangled_name (demangled_name_ptr);
 
-  entry.mangled = linkage_name_copy;
-  slot = ((struct demangled_name_entry **)
-	  htab_find_slot (per_bfd->demangled_names_hash.get (),
-			  &entry, INSERT));
-
-  /* If this name is not in the hash table, add it.  */
-  if (*slot == NULL
-      /* A C version of the symbol may have already snuck into the table.
-	 This happens to, e.g., main.init (__go_init_main).  Cope.  */
-      || (gsymbol->language == language_go
-	  && (*slot)->demangled[0] == '\0'))
-    {
-      int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0;
-
-      /* Suppose we have demangled_name==NULL, copy_name==0, and
-	 linkage_name_copy==linkage_name.  In this case, we already have the
-	 mangled name saved, and we don't have a demangled name.  So,
-	 you might think we could save a little space by not recording
-	 this in the hash table at all.
+  struct demangled_name_entry *found_entry;
+
+  {
+    std::lock_guard<std::mutex> guard (demangled_mutex);
+
+    if (per_bfd->demangled_names_hash == NULL)
+      create_demangled_names_hash (per_bfd);
+
+    entry.mangled = linkage_name_copy;
+    slot = ((struct demangled_name_entry **)
+	    htab_find_slot (per_bfd->demangled_names_hash.get (),
+			    &entry, INSERT));
+
+    /* If this name is not in the hash table, add it.  */
+    if (*slot == NULL
+	/* A C version of the symbol may have already snuck into the table.
+	   This happens to, e.g., main.init (__go_init_main).  Cope.  */
+	|| (gsymbol->language == language_go
+	    && (*slot)->demangled[0] == '\0'))
+      {
+	int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0;
+
+	/* Suppose we have demangled_name==NULL, copy_name==0, and
+	   linkage_name_copy==linkage_name.  In this case, we already have the
+	   mangled name saved, and we don't have a demangled name.  So,
+	   you might think we could save a little space by not recording
+	   this in the hash table at all.
 	 
-	 It turns out that it is actually important to still save such
-	 an entry in the hash table, because storing this name gives
-	 us better bcache hit rates for partial symbols.  */
-      if (!copy_name && linkage_name_copy == linkage_name)
-	{
-	  *slot
-	    = ((struct demangled_name_entry *)
-	       obstack_alloc (&per_bfd->storage_obstack,
-			      offsetof (struct demangled_name_entry, demangled)
-			      + demangled_len + 1));
-	  (*slot)->mangled = linkage_name;
-	}
-      else
-	{
-	  char *mangled_ptr;
-
-	  /* If we must copy the mangled name, put it directly after
-	     the demangled name so we can have a single
-	     allocation.  */
-	  *slot
-	    = ((struct demangled_name_entry *)
-	       obstack_alloc (&per_bfd->storage_obstack,
-			      offsetof (struct demangled_name_entry, demangled)
-			      + len + demangled_len + 2));
-	  mangled_ptr = &((*slot)->demangled[demangled_len + 1]);
-	  strcpy (mangled_ptr, linkage_name_copy);
-	  (*slot)->mangled = mangled_ptr;
-	}
+	   It turns out that it is actually important to still save such
+	   an entry in the hash table, because storing this name gives
+	   us better bcache hit rates for partial symbols.  */
+	if (!copy_name && linkage_name_copy == linkage_name)
+	  {
+	    *slot
+	      = ((struct demangled_name_entry *)
+		 obstack_alloc (&per_bfd->storage_obstack,
+				offsetof (struct demangled_name_entry, demangled)
+				+ demangled_len + 1));
+	    (*slot)->mangled = linkage_name;
+	  }
+	else
+	  {
+	    char *mangled_ptr;
+
+	    /* If we must copy the mangled name, put it directly after
+	       the demangled name so we can have a single
+	       allocation.  */
+	    *slot
+	      = ((struct demangled_name_entry *)
+		 obstack_alloc (&per_bfd->storage_obstack,
+				offsetof (struct demangled_name_entry, demangled)
+				+ len + demangled_len + 2));
+	    mangled_ptr = &((*slot)->demangled[demangled_len + 1]);
+	    strcpy (mangled_ptr, linkage_name_copy);
+	    (*slot)->mangled = mangled_ptr;
+	  }
 
-      if (demangled_name != NULL)
-	strcpy ((*slot)->demangled, demangled_name.get());
-      else
-	(*slot)->demangled[0] = '\0';
-    }
+	if (demangled_name != NULL)
+	  strcpy ((*slot)->demangled, demangled_name.get());
+	else
+	  (*slot)->demangled[0] = '\0';
+      }
+
+    found_entry = *slot;
+  }
 
-  gsymbol->name = (*slot)->mangled;
-  if ((*slot)->demangled[0] != '\0')
-    symbol_set_demangled_name (gsymbol, (*slot)->demangled,
+  gsymbol->name = found_entry->mangled;
+  if (found_entry->demangled[0] != '\0')
+    symbol_set_demangled_name (gsymbol, found_entry->demangled,
 			       &per_bfd->storage_obstack);
   else
     symbol_set_demangled_name (gsymbol, NULL, &per_bfd->storage_obstack);
-- 
2.17.2

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

* [RFC 5/6] Introduce thread-safe way to handle SIGSEGV
  2019-03-09 17:23 [RFC 0/6] Demangle minimal symbol names in worker threads Tom Tromey
  2019-03-09 17:23 ` [RFC 2/6] Remove static buffer from ada_decode Tom Tromey
@ 2019-03-09 17:23 ` Tom Tromey
  2019-03-11 17:24   ` John Baldwin
  2019-03-09 17:23 ` [RFC 1/6] Defer minimal symbol name-setting Tom Tromey
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2019-03-09 17:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

The gdb demangler installs a SIGSEGV handler in order to protect gdb
from demangler bugs.  However, this is not thread-safe, as signal
handlers are global to the process.

This patch changes gdb to always install a global SIGSEGV handler, and
then lets thread indicate their interest in handling the signal by
setting a thread-local variable.

This patch then arranges for the demangler code to use this; being
sure to arrange for calls to warning and the like to be done on the
main thread.

2019-03-08  Tom Tromey  <tom@tromey.com>

	* event-top.h (segv_handler): Declare.
	* event-top.c (segv_handler): New global.
	(handle_sigsegv): New function.
	(async_init_signals): Install SIGSEGV handler.
	* cp-support.c (gdb_demangle_jmp_buf): Change type.  Now
	thread-local.
	(report_failed_demangle): New function.
	(gdb_demangle): Make core_dump_allowed atomic.  Remove signal
	handler-setting code, instead use segv_handler.  Run warning code
	on main thread.
---
 gdb/ChangeLog    |  13 ++++++
 gdb/cp-support.c | 114 +++++++++++++++++++++++------------------------
 gdb/event-top.c  |  37 +++++++++++++++
 gdb/event-top.h  |   3 ++
 4 files changed, 109 insertions(+), 58 deletions(-)

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 79457f991fa..ae7f7e2abc2 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -37,6 +37,9 @@
 #include "common/gdb_setjmp.h"
 #include "safe-ctype.h"
 #include "common/selftest.h"
+#include <atomic>
+#include "event-top.h"
+#include "ser-event.h"
 
 #define d_left(dc) (dc)->u.s_binary.left
 #define d_right(dc) (dc)->u.s_binary.right
@@ -1470,7 +1473,7 @@ static int catch_demangler_crashes = 1;
 
 /* Stack context and environment for demangler crash recovery.  */
 
-static SIGJMP_BUF gdb_demangle_jmp_buf;
+static thread_local SIGJMP_BUF *gdb_demangle_jmp_buf;
 
 /* If nonzero, attempt to dump core from the signal handler.  */
 
@@ -1489,7 +1492,43 @@ gdb_demangle_signal_handler (int signo)
       gdb_demangle_attempt_core_dump = 0;
     }
 
-  SIGLONGJMP (gdb_demangle_jmp_buf, signo);
+  SIGLONGJMP (*gdb_demangle_jmp_buf, signo);
+}
+
+/* A helper for gdb_demangle that reports a demangling failure.  */
+
+static void
+report_failed_demangle (const char *name, int core_dump_allowed,
+			int crash_signal)
+{
+  static int error_reported = 0;
+
+  if (!error_reported)
+    {
+      std::string short_msg
+	= string_printf (_("unable to demangle '%s' "
+			   "(demangler failed with signal %d)"),
+			 name, crash_signal);
+
+      std::string long_msg
+	= string_printf ("%s:%d: %s: %s", __FILE__, __LINE__,
+			 "demangler-warning", short_msg.c_str ());
+
+      target_terminal::scoped_restore_terminal_state term_state;
+      target_terminal::ours_for_output ();
+
+      begin_line ();
+      if (core_dump_allowed)
+	fprintf_unfiltered (gdb_stderr,
+			    _("%s\nAttempting to dump core.\n"),
+			    long_msg.c_str ());
+      else
+	warn_cant_dump_core (long_msg.c_str ());
+
+      demangler_warning (__FILE__, __LINE__, "%s", short_msg.c_str ());
+
+      error_reported = 1;
+    }
 }
 
 #endif
@@ -1503,12 +1542,7 @@ gdb_demangle (const char *name, int options)
   int crash_signal = 0;
 
 #ifdef HAVE_WORKING_FORK
-#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
-  struct sigaction sa, old_sa;
-#else
-  sighandler_t ofunc;
-#endif
-  static int core_dump_allowed = -1;
+  static std::atomic<int> core_dump_allowed (-1);
 
   if (core_dump_allowed == -1)
     {
@@ -1518,23 +1552,15 @@ gdb_demangle (const char *name, int options)
 	gdb_demangle_attempt_core_dump = 0;
     }
 
-  if (catch_demangler_crashes)
-    {
-#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
-      sa.sa_handler = gdb_demangle_signal_handler;
-      sigemptyset (&sa.sa_mask);
-#ifdef HAVE_SIGALTSTACK
-      sa.sa_flags = SA_ONSTACK;
-#else
-      sa.sa_flags = 0;
-#endif
-      sigaction (SIGSEGV, &sa, &old_sa);
-#else
-      ofunc = signal (SIGSEGV, gdb_demangle_signal_handler);
-#endif
+  scoped_restore restore_segv
+    = make_scoped_restore (&segv_handler,
+			   catch_demangler_crashes
+			   ? gdb_demangle_signal_handler
+			   : nullptr);
 
-      crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
-    }
+  SIGJMP_BUF jmp_buf;
+  if (catch_demangler_crashes)
+    crash_signal = SIGSETJMP (jmp_buf);
 #endif
 
   if (crash_signal == 0)
@@ -1543,42 +1569,14 @@ gdb_demangle (const char *name, int options)
 #ifdef HAVE_WORKING_FORK
   if (catch_demangler_crashes)
     {
-#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
-      sigaction (SIGSEGV, &old_sa, NULL);
-#else
-      signal (SIGSEGV, ofunc);
-#endif
-
       if (crash_signal != 0)
 	{
-	  static int error_reported = 0;
-
-	  if (!error_reported)
-	    {
-	      std::string short_msg
-		= string_printf (_("unable to demangle '%s' "
-				   "(demangler failed with signal %d)"),
-				 name, crash_signal);
-
-	      std::string long_msg
-		= string_printf ("%s:%d: %s: %s", __FILE__, __LINE__,
-				 "demangler-warning", short_msg.c_str ());
-
-	      target_terminal::scoped_restore_terminal_state term_state;
-	      target_terminal::ours_for_output ();
-
-	      begin_line ();
-	      if (core_dump_allowed)
-		fprintf_unfiltered (gdb_stderr,
-				    _("%s\nAttempting to dump core.\n"),
-				    long_msg.c_str ());
-	      else
-		warn_cant_dump_core (long_msg.c_str ());
-
-	      demangler_warning (__FILE__, __LINE__, "%s", short_msg.c_str ());
-
-	      error_reported = 1;
-	    }
+	  std::string copy = name;
+	  run_on_main_thread ([=] ()
+            {
+	      report_failed_demangle (copy.c_str (), core_dump_allowed,
+				      crash_signal);
+	    });
 
 	  result = NULL;
 	}
diff --git a/gdb/event-top.c b/gdb/event-top.c
index 6e4031165c7..3cb2046aeb3 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -845,6 +845,29 @@ gdb_readline_no_editing_callback (gdb_client_data client_data)
 }
 \f
 
+/* See event-top.h.  */
+
+thread_local void (*segv_handler) (int);
+
+/* Handler for SIGSEGV.  */
+
+static void
+handle_sigsegv (int sig)
+{
+  if (segv_handler == nullptr)
+    {
+      signal (sig, nullptr);
+      raise (sig);
+    }
+  else
+    {
+      signal (sig, handle_sigsegv);
+      segv_handler (sig);
+    }
+}
+
+\f
+
 /* The serial event associated with the QUIT flag.  set_quit_flag sets
    this, and check_quit_flag clears it.  Used by interruptible_select
    to be able to do interruptible I/O with no race with the SIGINT
@@ -912,6 +935,20 @@ async_init_signals (void)
   sigtstp_token =
     create_async_signal_handler (async_sigtstp_handler, NULL);
 #endif
+
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+  struct sigaction sa, old_sa;
+  sa.sa_handler = handle_sigsegv;
+  sigemptyset (&sa.sa_mask);
+#ifdef HAVE_SIGALTSTACK
+  sa.sa_flags = SA_ONSTACK;
+#else
+  sa.sa_flags = 0;
+#endif
+  sigaction (SIGSEGV, &sa, &old_sa);
+#else
+  signal (SIGSEGV, handle_sigsegv);
+#endif
 }
 
 /* See defs.h.  */
diff --git a/gdb/event-top.h b/gdb/event-top.h
index 4c3e6cb8612..849f40d9192 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -70,4 +70,7 @@ extern void gdb_rl_callback_handler_install (const char *prompt);
    currently installed.  */
 extern void gdb_rl_callback_handler_reinstall (void);
 
+/* The SIGSEGV handler for this thread, or NULL if there is none.  */
+extern thread_local void (*segv_handler) (int);
+
 #endif
-- 
2.17.2

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

* [RFC 1/6] Defer minimal symbol name-setting
  2019-03-09 17:23 [RFC 0/6] Demangle minimal symbol names in worker threads Tom Tromey
  2019-03-09 17:23 ` [RFC 2/6] Remove static buffer from ada_decode Tom Tromey
  2019-03-09 17:23 ` [RFC 5/6] Introduce thread-safe way to handle SIGSEGV Tom Tromey
@ 2019-03-09 17:23 ` Tom Tromey
  2019-03-09 17:23 ` [RFC 3/6] Lock the demangled hash table Tom Tromey
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-03-09 17:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

Currently the demangled name of a minimal symbol is set when creating
the symbol.  However, there is no intrinsic need to do this.  This
patch instead arranges for the demangling to be done just before the
minsym hash tables are filled.  This will be useful in a later patch.

gdb/ChangeLog
2019-03-03  Tom Tromey  <tom@tromey.com>

	* symtab.h (struct minimal_symbol) <name_set>: New member.
	* minsyms.c (minimal_symbol_reader::record_full): Copy name.
	Don't call symbol_set_names.
	(minimal_symbol_reader::install): Call symbol_set_names.
---
 gdb/ChangeLog |  7 +++++++
 gdb/minsyms.c | 18 +++++++++++++++++-
 gdb/symtab.h  |  4 ++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 1b8e67bc98b..7872b7e2588 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1145,7 +1145,11 @@ minimal_symbol_reader::record_full (const char *name, int name_len,
   msymbol = &m_msym_bunch->contents[m_msym_bunch_index];
   symbol_set_language (msymbol, language_auto,
 		       &m_objfile->per_bfd->storage_obstack);
-  symbol_set_names (msymbol, name, name_len, copy_name, m_objfile->per_bfd);
+
+  if (copy_name)
+    name = (char *) obstack_copy0 (&m_objfile->per_bfd->storage_obstack,
+				   name, name_len);
+  msymbol->name = name;
 
   SET_MSYMBOL_VALUE_ADDRESS (msymbol, address);
   MSYMBOL_SECTION (msymbol) = section;
@@ -1404,6 +1408,18 @@ minimal_symbol_reader::install ()
       m_objfile->per_bfd->minimal_symbol_count = mcount;
       m_objfile->per_bfd->msymbols = std::move (msym_holder);
 
+      msymbols = m_objfile->per_bfd->msymbols.get ();
+      for (int i = 0; i < mcount; ++i)
+	{
+	  if (!msymbols[i].name_set)
+	    {
+	      symbol_set_names (&msymbols[i], msymbols[i].name,
+				strlen (msymbols[i].name), 0,
+				m_objfile->per_bfd);
+	      msymbols[i].name_set = 1;
+	    }
+	}
+
       /* Now build the hash tables; we can't do this incrementally
          at an earlier point since we weren't finished with the obstack
 	 yet.  (And if the msymbol obstack gets moved, all the internal
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 85dc3710483..29595685981 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -669,6 +669,10 @@ struct minimal_symbol : public general_symbol_info
      the object file format may not carry that piece of information.  */
   unsigned int has_size : 1;
 
+  /* Non-zero if this symbol ever had its demangled name set (even if
+     it was set to NULL).  */
+  unsigned int name_set : 1;
+
   /* Minimal symbols with the same hash key are kept on a linked
      list.  This is the link.  */
 
-- 
2.17.2

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

* [RFC 2/6] Remove static buffer from ada_decode
  2019-03-09 17:23 [RFC 0/6] Demangle minimal symbol names in worker threads Tom Tromey
@ 2019-03-09 17:23 ` Tom Tromey
  2019-03-09 17:23 ` [RFC 5/6] Introduce thread-safe way to handle SIGSEGV Tom Tromey
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-03-09 17:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

ada_decode has a static buffer, which means it is not thread-safe.
This patch removes the buffer and adds a parameter so that the storage
can be managed by the caller.

2019-03-03  Tom Tromey  <tom@tromey.com>

	* ada-varobj.c (ada_varobj_describe_simple_array_child): Update.
	* ada-lang.h (ada_decode): Add "storage" parameter.
	* ada-lang.c (ada_decode): Add "storage" parameter.
	(ada_decode_symbol, ada_la_decode, ada_sniff_from_mangled_name)
	(is_valid_name_for_wild_match, name_matches_regex)
	(ada_add_global_exceptions): Update.
	* ada-exp.y (write_object_renaming): Update.
---
 gdb/ChangeLog    | 10 +++++++++
 gdb/ada-exp.y    |  6 +++++-
 gdb/ada-lang.c   | 55 ++++++++++++++++++++++++++++++------------------
 gdb/ada-lang.h   |  3 ++-
 gdb/ada-varobj.c |  3 ++-
 5 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y
index 5925416e88f..1cadc19496c 100644
--- a/gdb/ada-exp.y
+++ b/gdb/ada-exp.y
@@ -817,7 +817,11 @@ write_object_renaming (struct parser_state *par_state,
 				 renamed_entity_len);
   ada_lookup_encoded_symbol (name, orig_left_context, VAR_DOMAIN, &sym_info);
   if (sym_info.symbol == NULL)
-    error (_("Could not find renamed variable: %s"), ada_decode (name));
+    {
+      gdb::unique_xmalloc_ptr<char> storage;
+      error (_("Could not find renamed variable: %s"),
+	     ada_decode (name, &storage));
+    }
   else if (SYMBOL_CLASS (sym_info.symbol) == LOC_TYPEDEF)
     /* We have a renaming of an old-style renaming symbol.  Don't
        trust the block information.  */
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 17a11842634..77ebdb10f8f 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -1156,22 +1156,18 @@ ada_remove_Xbn_suffix (const char *encoded, int *len)
 
 /* If ENCODED follows the GNAT entity encoding conventions, then return
    the decoded form of ENCODED.  Otherwise, return "<%s>" where "%s" is
-   replaced by ENCODED.
-
-   The resulting string is valid until the next call of ada_decode.
-   If the string is unchanged by decoding, the original string pointer
-   is returned.  */
+   replaced by ENCODED.  */
 
 const char *
-ada_decode (const char *encoded)
+ada_decode (const char *encoded, gdb::unique_xmalloc_ptr<char> *storage)
 {
   int i, j;
   int len0;
   const char *p;
   char *decoded;
   int at_start_name;
-  static char *decoding_buffer = NULL;
-  static size_t decoding_buffer_size = 0;
+  char *decoding_buffer = NULL;
+  size_t decoding_buffer_size = 0;
 
   /* With function descriptors on PPC64, the value of a symbol named
      ".FN", if it exists, is the entry point of the function "FN".  */
@@ -1399,9 +1395,15 @@ ada_decode (const char *encoded)
       goto Suppress;
 
   if (strcmp (decoded, encoded) == 0)
-    return encoded;
+    {
+      xfree (decoding_buffer);
+      return encoded;
+    }
   else
-    return decoded;
+    {
+      storage->reset (decoded);
+      return decoded;
+    }
 
 Suppress:
   GROW_VECT (decoding_buffer, decoding_buffer_size, strlen (encoded) + 3);
@@ -1410,8 +1412,8 @@ Suppress:
     strcpy (decoded, encoded);
   else
     xsnprintf (decoded, decoding_buffer_size, "<%s>", encoded);
+  storage->reset (decoded);
   return decoded;
-
 }
 
 /* Table for keeping permanent unique copies of decoded names.  Once
@@ -1440,7 +1442,8 @@ ada_decode_symbol (const struct general_symbol_info *arg)
 
   if (!gsymbol->ada_mangled)
     {
-      const char *decoded = ada_decode (gsymbol->name);
+      gdb::unique_xmalloc_ptr<char> storage;
+      const char *decoded = ada_decode (gsymbol->name, &storage);
       struct obstack *obstack = gsymbol->language_specific.obstack;
 
       gsymbol->ada_mangled = 1;
@@ -1470,7 +1473,8 @@ ada_decode_symbol (const struct general_symbol_info *arg)
 static char *
 ada_la_decode (const char *encoded, int options)
 {
-  return xstrdup (ada_decode (encoded));
+  gdb::unique_xmalloc_ptr<char> storage;
+  return xstrdup (ada_decode (encoded, &storage));
 }
 
 /* Implement la_sniff_from_mangled_name for Ada.  */
@@ -1478,7 +1482,8 @@ ada_la_decode (const char *encoded, int options)
 static int
 ada_sniff_from_mangled_name (const char *mangled, char **out)
 {
-  const char *demangled = ada_decode (mangled);
+  gdb::unique_xmalloc_ptr<char> storage;
+  const char *demangled = ada_decode (mangled, &storage);
 
   *out = NULL;
 
@@ -6071,7 +6076,8 @@ is_name_suffix (const char *str)
 static int
 is_valid_name_for_wild_match (const char *name0)
 {
-  const char *decoded_name = ada_decode (name0);
+  gdb::unique_xmalloc_ptr<char> storage;
+  const char *decoded_name = ada_decode (name0, &storage);
   int i;
 
   /* If the decoded name starts with an angle bracket, it means that
@@ -6319,8 +6325,9 @@ ada_lookup_name_info::matches
          is not a suitable completion.  */
       const char *sym_name_copy = sym_name;
       bool has_angle_bracket;
+      gdb::unique_xmalloc_ptr<char> storage;
 
-      sym_name = ada_decode (sym_name);
+      sym_name = ada_decode (sym_name, &storage);
       has_angle_bracket = (sym_name[0] == '<');
       match = (has_angle_bracket == m_verbatim_p);
       sym_name = sym_name_copy;
@@ -6342,12 +6349,14 @@ ada_lookup_name_info::matches
 
   /* Second: Try wild matching...  */
 
+  gdb::unique_xmalloc_ptr<char> sym_name_storage;
   if (!match && m_wild_match_p)
     {
       /* Since we are doing wild matching, this means that TEXT
          may represent an unqualified symbol name.  We therefore must
          also compare TEXT against the unqualified name of the symbol.  */
-      sym_name = ada_unqualified_name (ada_decode (sym_name));
+      sym_name = ada_unqualified_name (ada_decode (sym_name,
+						   &sym_name_storage));
 
       if (strncmp (sym_name, text, text_len) == 0)
 	match = true;
@@ -6363,7 +6372,10 @@ ada_lookup_name_info::matches
       std::string &match_str = comp_match_res->match.storage ();
 
       if (!m_encoded_p)
-	match_str = ada_decode (sym_name);
+	{
+	  gdb::unique_xmalloc_ptr<char> storage;
+	  match_str = ada_decode (sym_name, &storage);
+	}
       else
 	{
 	  if (m_verbatim_p)
@@ -13501,8 +13513,9 @@ ada_add_exceptions_from_frame (compiled_regex *preg,
 static bool
 name_matches_regex (const char *name, compiled_regex *preg)
 {
+  gdb::unique_xmalloc_ptr<char> storage;
   return (preg == NULL
-	  || preg->exec (ada_decode (name), 0, NULL, 0) == 0);
+	  || preg->exec (ada_decode (name, &storage), 0, NULL, 0) == 0);
 }
 
 /* Add all exceptions defined globally whose name name match
@@ -13535,7 +13548,9 @@ ada_add_global_exceptions (compiled_regex *preg,
 			   lookup_name_info::match_any (),
 			   [&] (const char *search_name)
 			   {
-			     const char *decoded = ada_decode (search_name);
+			     gdb::unique_xmalloc_ptr<char> storage;
+			     const char *decoded = ada_decode (search_name,
+							       &storage);
 			     return name_matches_regex (decoded, preg);
 			   },
 			   NULL,
diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
index ee03dbd2aad..754779911be 100644
--- a/gdb/ada-lang.h
+++ b/gdb/ada-lang.h
@@ -227,7 +227,8 @@ extern struct type *ada_get_decoded_type (struct type *type);
 
 extern const char *ada_decode_symbol (const struct general_symbol_info *);
 
-extern const char *ada_decode (const char*);
+extern const char *ada_decode (const char *,
+			       gdb::unique_xmalloc_ptr<char> *storage);
 
 extern enum language ada_update_initial_language (enum language);
 
diff --git a/gdb/ada-varobj.c b/gdb/ada-varobj.c
index a4d553d3786..29a4c86cc5f 100644
--- a/gdb/ada-varobj.c
+++ b/gdb/ada-varobj.c
@@ -624,6 +624,7 @@ ada_varobj_describe_simple_array_child (struct value *parent_value,
 	 of the array index type when such type qualification is
 	 needed.  */
       const char *index_type_name = NULL;
+      gdb::unique_xmalloc_ptr<char> storage;
 
       /* If the index type is a range type, find the base type.  */
       while (TYPE_CODE (index_type) == TYPE_CODE_RANGE)
@@ -634,7 +635,7 @@ ada_varobj_describe_simple_array_child (struct value *parent_value,
 	{
 	  index_type_name = ada_type_name (index_type);
 	  if (index_type_name)
-	    index_type_name = ada_decode (index_type_name);
+	    index_type_name = ada_decode (index_type_name, &storage);
 	}
 
       if (index_type_name != NULL)
-- 
2.17.2

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

* [RFC 6/6] Demangle minsyms in parallel
  2019-03-09 17:23 [RFC 0/6] Demangle minimal symbol names in worker threads Tom Tromey
                   ` (3 preceding siblings ...)
  2019-03-09 17:23 ` [RFC 3/6] Lock the demangled hash table Tom Tromey
@ 2019-03-09 17:23 ` Tom Tromey
  2019-03-09 17:23 ` [RFC 4/6] Introduce run_on_main_thread Tom Tromey
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-03-09 17:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

This patch introduces a simple parallel for_each and changes the
minimal symbol reader to use it when computing the demangled name for
a minimal symbol.  This yields a speedup when reading minimal symbols.

2019-03-03  Tom Tromey  <tom@tromey.com>

	* minsyms.c (minimal_symbol_reader::install): Use
	parallel_for_each.
	* common/parallel-for.h: New file.
	* Makefile.in (HFILES_NO_SRCDIR): Add common/parallel-for.h.
---
 gdb/ChangeLog             |  7 ++++
 gdb/Makefile.in           |  1 +
 gdb/common/parallel-for.h | 69 +++++++++++++++++++++++++++++++++++++++
 gdb/minsyms.c             | 23 +++++++------
 4 files changed, 90 insertions(+), 10 deletions(-)
 create mode 100644 gdb/common/parallel-for.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 5614cc3386c..fec9c4a505c 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1466,6 +1466,7 @@ HFILES_NO_SRCDIR = \
 	common/common-inferior.h \
 	common/netstuff.h \
 	common/host-defs.h \
+	common/parallel-for.h \
 	common/pathstuff.h \
 	common/print-utils.h \
 	common/ptid.h \
diff --git a/gdb/common/parallel-for.h b/gdb/common/parallel-for.h
new file mode 100644
index 00000000000..a0ae7bebd16
--- /dev/null
+++ b/gdb/common/parallel-for.h
@@ -0,0 +1,69 @@
+/* Parallel for loops
+
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef COMMON_PARALLEL_FOR_H
+#define COMMON_PARALLEL_FOR_H
+
+#include <algorithm>
+#include <thread>
+
+namespace gdb
+{
+
+/* A very simple "parallel for".  This iterates over the elements
+   given by the range of iterators, which must be random access
+   iterators.  For each element, it calls the callback function.  The
+   work may or may not be done by separate threads.  */
+
+template<class RandomIt, class UnaryFunction>
+void parallel_for_each (RandomIt first, RandomIt last, UnaryFunction f)
+{
+  unsigned n_threads = std::thread::hardware_concurrency ();
+  /* So we can use a local array below.  */
+  const unsigned max_threads = 16;
+  if (n_threads > max_threads)
+    n_threads = max_threads;
+
+  if (n_threads == 0 || last - first < 2 * n_threads)
+    {
+      /* Don't bother.  */
+      std::for_each (first, last, f);
+      return;
+    }
+
+  auto body = [&] (RandomIt start)
+    {
+      for (; start < last; start += n_threads)
+	f (*start);
+    };
+
+  std::thread threads[max_threads];
+  for (unsigned i = 0; i < n_threads; ++i)
+    {
+      threads[i] = std::thread (body, first);
+      ++first;
+    }
+
+  for (unsigned i = 0; i < n_threads; ++i)
+    threads[i].join ();
+}
+
+}
+
+#endif /* COMMON_PARALLEL_FOR_H */
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 7872b7e2588..75c1bb9daf4 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -53,6 +53,7 @@
 #include "common/symbol.h"
 #include <algorithm>
 #include "safe-ctype.h"
+#include "common/parallel-for.h"
 
 /* See minsyms.h.  */
 
@@ -1409,16 +1410,18 @@ minimal_symbol_reader::install ()
       m_objfile->per_bfd->msymbols = std::move (msym_holder);
 
       msymbols = m_objfile->per_bfd->msymbols.get ();
-      for (int i = 0; i < mcount; ++i)
-	{
-	  if (!msymbols[i].name_set)
-	    {
-	      symbol_set_names (&msymbols[i], msymbols[i].name,
-				strlen (msymbols[i].name), 0,
-				m_objfile->per_bfd);
-	      msymbols[i].name_set = 1;
-	    }
-	}
+      gdb::parallel_for_each
+	(&msymbols[0], &msymbols[mcount],
+	 [&] (minimal_symbol &msym)
+	 {
+	   if (!msym.name_set)
+	     {
+	       symbol_set_names (&msym, msym.name,
+				 strlen (msym.name), 0,
+				 m_objfile->per_bfd);
+	       msym.name_set = 1;
+	     }
+	 });
 
       /* Now build the hash tables; we can't do this incrementally
          at an earlier point since we weren't finished with the obstack
-- 
2.17.2

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-09 17:23 [RFC 0/6] Demangle minimal symbol names in worker threads Tom Tromey
                   ` (5 preceding siblings ...)
  2019-03-09 17:23 ` [RFC 4/6] Introduce run_on_main_thread Tom Tromey
@ 2019-03-09 18:09 ` Eli Zaretskii
  2019-03-11 17:35   ` John Baldwin
  2019-03-11 22:39   ` Tom Tromey
  2019-03-11 17:26 ` John Baldwin
  7 siblings, 2 replies; 20+ messages in thread
From: Eli Zaretskii @ 2019-03-09 18:09 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

> From: Tom Tromey <tom@tromey.com>
> Date: Sat,  9 Mar 2019 10:22:54 -0700
> 
> I've thought for a while that gdb should take advantage of multiple
> cores in order to speed up its processing.  This series is some
> initial work in that direction.
> 
> In particular, this patch arranges to do the demangling for minimal
> symbols in worker threads.  I chose this because it seemed relatively
> simple to reason about, as the demangler is already mostly thread-safe
> (except, as it turns out, the Ada demangler, which is fixed in this
> series).  It isn't actually a very important thing to speed up, as
> minimal symbol reading is already reasonably speedy; but I thought it
> best to start with something straightforward to facilitate flushing
> out thread safety bugs.

Thanks, but is std::thread portable enough?  E.g., I recall problems
with it in MinGW.

Same question regarding delivering signals to threads.

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

* Re: [RFC 5/6] Introduce thread-safe way to handle SIGSEGV
  2019-03-09 17:23 ` [RFC 5/6] Introduce thread-safe way to handle SIGSEGV Tom Tromey
@ 2019-03-11 17:24   ` John Baldwin
  0 siblings, 0 replies; 20+ messages in thread
From: John Baldwin @ 2019-03-11 17:24 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches

On 3/9/19 9:22 AM, Tom Tromey wrote:
> The gdb demangler installs a SIGSEGV handler in order to protect gdb
> from demangler bugs.  However, this is not thread-safe, as signal
> handlers are global to the process.
> 
> This patch changes gdb to always install a global SIGSEGV handler, and
> then lets thread indicate their interest in handling the signal by
> setting a thread-local variable.
> 
> This patch then arranges for the demangler code to use this; being
> sure to arrange for calls to warning and the like to be done on the
> main thread.

The one downside of always having a handler is that the $_siginfo after
a "normal" sig11 in GDB won't be valid anymore (it will have SI_USER
set now instead and si_addr won't be valid, etc.).  We could "fix" this
by having signal frames supply $_siginfo though I'm not sure what
the most intuitive model of that would be.

-- 
John Baldwin

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-09 17:23 [RFC 0/6] Demangle minimal symbol names in worker threads Tom Tromey
                   ` (6 preceding siblings ...)
  2019-03-09 18:09 ` [RFC 0/6] Demangle minimal symbol names in worker threads Eli Zaretskii
@ 2019-03-11 17:26 ` John Baldwin
  2019-03-11 22:40   ` Tom Tromey
  7 siblings, 1 reply; 20+ messages in thread
From: John Baldwin @ 2019-03-11 17:26 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches

On 3/9/19 9:22 AM, Tom Tromey wrote:
> I have some more ideas for areas in gdb that could benefit from
> threads.  I'm happy to discuss if you're interested.

My only caution here is that I find it handy that I can use single
stepping to debug many of the things I encounter in gdb, especially
the main event loop.  When I've worked with lldb I've found single
stepping less useful and have had to rely on their builtin tracing
framework to usefully debug things.  Peeling off tasks that aren't
part of the main event loop probably won't impact that, but I think
it's worth considering that tradeoff for future patches at least.

-- 
John Baldwin

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-09 18:09 ` [RFC 0/6] Demangle minimal symbol names in worker threads Eli Zaretskii
@ 2019-03-11 17:35   ` John Baldwin
  2019-03-11 22:39   ` Tom Tromey
  1 sibling, 0 replies; 20+ messages in thread
From: John Baldwin @ 2019-03-11 17:35 UTC (permalink / raw)
  To: Eli Zaretskii, Tom Tromey; +Cc: gdb-patches

On 3/9/19 10:09 AM, Eli Zaretskii wrote:
>> From: Tom Tromey <tom@tromey.com>
>> Date: Sat,  9 Mar 2019 10:22:54 -0700
>>
>> I've thought for a while that gdb should take advantage of multiple
>> cores in order to speed up its processing.  This series is some
>> initial work in that direction.
>>
>> In particular, this patch arranges to do the demangling for minimal
>> symbols in worker threads.  I chose this because it seemed relatively
>> simple to reason about, as the demangler is already mostly thread-safe
>> (except, as it turns out, the Ada demangler, which is fixed in this
>> series).  It isn't actually a very important thing to speed up, as
>> minimal symbol reading is already reasonably speedy; but I thought it
>> best to start with something straightforward to facilitate flushing
>> out thread safety bugs.
> 
> Thanks, but is std::thread portable enough?  E.g., I recall problems
> with it in MinGW.
> 
> Same question regarding delivering signals to threads.

I can't speak to MinGW, but the signal in question here is a synchronous
SIGSEGV signal due to a page fault or the like and in POSIX systems that
is always sent to the thread executing the faulting instruction (and that
is the only sane semantic since the thread can't execute any more
instructions until the signal is resolved).

-- 
John Baldwin

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-09 18:09 ` [RFC 0/6] Demangle minimal symbol names in worker threads Eli Zaretskii
  2019-03-11 17:35   ` John Baldwin
@ 2019-03-11 22:39   ` Tom Tromey
  2019-03-12  3:33     ` Eli Zaretskii
  1 sibling, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2019-03-11 22:39 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Tom Tromey, gdb-patches

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Thanks, but is std::thread portable enough?  E.g., I recall problems
Eli> with it in MinGW.

I asked around and learned that std::thread should work fine if you have
winpthreads, which apparently is used by default for mingw-w64.

I don't actually know much about the mingw world, so I don't know if
that is good enough or not.  Could you say?

Jonathan Wakely also once wrote a port of libstdc++ to the Win32 API,
but couldn't find help with testing, so never checked it in.

Eli> Same question regarding delivering signals to threads.

If the signal thing is broken in this series, then it is probably
already broken today.  However I think it ought to be ok as SEGV is a
synchronous signal; that is, it is delivered to the faulting thread.

Does that address your concern here?

Tom

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-11 17:26 ` John Baldwin
@ 2019-03-11 22:40   ` Tom Tromey
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Tromey @ 2019-03-11 22:40 UTC (permalink / raw)
  To: John Baldwin; +Cc: Tom Tromey, gdb-patches

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

Tom> I have some more ideas for areas in gdb that could benefit from
Tom> threads.  I'm happy to discuss if you're interested.

John> My only caution here is that I find it handy that I can use single
John> stepping to debug many of the things I encounter in gdb, especially
John> the main event loop.

At least for the "parallel for" code that is in this series, it would be
simple to add a debugging flag to force it to take the single-thread
path.

Tom

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-11 22:39   ` Tom Tromey
@ 2019-03-12  3:33     ` Eli Zaretskii
  2019-03-13 15:38       ` Eli Zaretskii
  0 siblings, 1 reply; 20+ messages in thread
From: Eli Zaretskii @ 2019-03-12  3:33 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org
> Date: Mon, 11 Mar 2019 16:39:15 -0600
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> Eli> Thanks, but is std::thread portable enough?  E.g., I recall problems
> Eli> with it in MinGW.
> 
> I asked around and learned that std::thread should work fine if you have
> winpthreads, which apparently is used by default for mingw-w64.
> 
> I don't actually know much about the mingw world, so I don't know if
> that is good enough or not.  Could you say?

I could try.  But regardless, I think we should allow for ports whose
std::thread is not up to the task, because evidently this is a
problematic area of libstdc++.  It is IMO better to allow running this
code in a single thread than breaking the entire build on some
platform because other platforms might benefit from multiple execution
cores.

> Eli> Same question regarding delivering signals to threads.
> 
> If the signal thing is broken in this series, then it is probably
> already broken today.  However I think it ought to be ok as SEGV is a
> synchronous signal; that is, it is delivered to the faulting thread.

I thought you were going to use that for signals in general.  Maybe
SEGV is OK.  But in general, signals are weird on Windows wrt threads;
for example, a SIGINT handler runs in a separate thread created by
Windows.

Thanks.

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-12  3:33     ` Eli Zaretskii
@ 2019-03-13 15:38       ` Eli Zaretskii
  2019-03-15 23:28         ` Tom Tromey
  2019-03-15 23:40         ` Tom Tromey
  0 siblings, 2 replies; 20+ messages in thread
From: Eli Zaretskii @ 2019-03-13 15:38 UTC (permalink / raw)
  To: tom; +Cc: gdb-patches

> Date: Tue, 12 Mar 2019 05:33:30 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: gdb-patches@sourceware.org
> 
> > I asked around and learned that std::thread should work fine if you have
> > winpthreads, which apparently is used by default for mingw-w64.
> > 
> > I don't actually know much about the mingw world, so I don't know if
> > that is good enough or not.  Could you say?
> 
> I could try.

Here's the answer.  The simple test program attached at the end
doesn't compile:

  D:\usr\eli\data>g++ -c ./threaded-hello.cc
  ./threaded-hello.cc: In function 'int main()':
  ./threaded-hello.cc:12:8: error: 'thread' is not a member of 'std'
     std::thread t1(call_from_thread);
	  ^~~~~~
  ./threaded-hello.cc:12:8: note: suggested alternative: 'tera'
     std::thread t1(call_from_thread);
	  ^~~~~~
	  tera
  ./threaded-hello.cc:15:3: error: 't1' was not declared in this scope
     t1.join();
     ^~
  ./threaded-hello.cc:15:3: note: suggested alternative: 'tm'
     t1.join();
     ^~
     tm

So I think only MinGW64 might have support for std::thread,
mingw.org's MinGW (which is what I'm using) definitely doesn't.

> But regardless, I think we should allow for ports whose
> std::thread is not up to the task, because evidently this is a
> problematic area of libstdc++.  It is IMO better to allow running this
> code in a single thread than breaking the entire build on some
> platform because other platforms might benefit from multiple execution
> cores.

This is still true.  We could detect at configure time that
std::thread isn't supported and revert to serial alternative code
instead.  Is that reasonable?  If that is deemed too much of a
maintenance burden, I could perhaps, with some guidance, implement a
simple replacement using Win32 primitives, if all we need is to start
a thread and then do the thread-join thing.  Let me know what you
think.

Here's the program I used to test this:

#include <iostream>
#include <thread>

//This function will be called from a thread

void call_from_thread() {
  std::cout << "Hello, World" << std::endl;
}

int main() {
  //Launch a thread
  std::thread t1(call_from_thread);

  //Join the thread with the main thread
  t1.join();

  return 0;
}

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-13 15:38       ` Eli Zaretskii
@ 2019-03-15 23:28         ` Tom Tromey
  2019-03-16  7:46           ` Eli Zaretskii
  2019-03-15 23:40         ` Tom Tromey
  1 sibling, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2019-03-15 23:28 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: tom, gdb-patches

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> This is still true.  We could detect at configure time that
Eli> std::thread isn't supported and revert to serial alternative code
Eli> instead.  Is that reasonable?

Yeah, I think it's not too hard to do this.

Eli> If that is deemed too much of a
Eli> maintenance burden, I could perhaps, with some guidance, implement a
Eli> simple replacement using Win32 primitives, if all we need is to start
Eli> a thread and then do the thread-join thing.

It seems like it would be good for mingw, and maybe even mingw-64, if
somebody wrote a Windows API port of the libstdc++ thread primitives.
Jonathan Wakely sent me his patches for the start of one, I can forward
them if you're interested.

Tom

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-13 15:38       ` Eli Zaretskii
  2019-03-15 23:28         ` Tom Tromey
@ 2019-03-15 23:40         ` Tom Tromey
  2019-03-16  7:52           ` Eli Zaretskii
  1 sibling, 1 reply; 20+ messages in thread
From: Tom Tromey @ 2019-03-15 23:40 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: tom, gdb-patches

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Here's the answer.  The simple test program attached at the end
Eli> doesn't compile:
[...]

Could you see if thread-locals work?
A simple program like:

    thread_local int x;

... should be enough to check, though maybe adding a main() and making
sure it could link would also be good.

Tom

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-15 23:28         ` Tom Tromey
@ 2019-03-16  7:46           ` Eli Zaretskii
  2019-03-16  8:31             ` Eli Zaretskii
  0 siblings, 1 reply; 20+ messages in thread
From: Eli Zaretskii @ 2019-03-16  7:46 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

> From: Tom Tromey <tom@tromey.com>
> Cc: tom@tromey.com,  gdb-patches@sourceware.org
> Date: Fri, 15 Mar 2019 17:28:54 -0600
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> Eli> This is still true.  We could detect at configure time that
> Eli> std::thread isn't supported and revert to serial alternative code
> Eli> instead.  Is that reasonable?
> 
> Yeah, I think it's not too hard to do this.

Then I think this would be a good way forward in the shirt run.

> Eli> If that is deemed too much of a
> Eli> maintenance burden, I could perhaps, with some guidance, implement a
> Eli> simple replacement using Win32 primitives, if all we need is to start
> Eli> a thread and then do the thread-join thing.
> 
> It seems like it would be good for mingw, and maybe even mingw-64, if
> somebody wrote a Windows API port of the libstdc++ thread primitives.
> Jonathan Wakely sent me his patches for the start of one, I can forward
> them if you're interested.

Please do, I will take a look.  (No promises yet until I've seen the
code, sorry ;-)

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-15 23:40         ` Tom Tromey
@ 2019-03-16  7:52           ` Eli Zaretskii
  0 siblings, 0 replies; 20+ messages in thread
From: Eli Zaretskii @ 2019-03-16  7:52 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

> From: Tom Tromey <tom@tromey.com>
> Cc: tom@tromey.com,  gdb-patches@sourceware.org
> Date: Fri, 15 Mar 2019 17:40:54 -0600
> 
> Could you see if thread-locals work?
> A simple program like:
> 
>     thread_local int x;
> 
> ... should be enough to check, though maybe adding a main() and making
> sure it could link would also be good.

Yes, the simple program below compiles, runs, and returns zero status.

thread_local int x;

int main (void)
{
  return x;
}

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

* Re: [RFC 0/6] Demangle minimal symbol names in worker threads
  2019-03-16  7:46           ` Eli Zaretskii
@ 2019-03-16  8:31             ` Eli Zaretskii
  0 siblings, 0 replies; 20+ messages in thread
From: Eli Zaretskii @ 2019-03-16  8:31 UTC (permalink / raw)
  To: tom; +Cc: gdb-patches

> Date: Sat, 16 Mar 2019 09:45:45 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: gdb-patches@sourceware.org
> 
> > Eli> This is still true.  We could detect at configure time that
> > Eli> std::thread isn't supported and revert to serial alternative code
> > Eli> instead.  Is that reasonable?
> > 
> > Yeah, I think it's not too hard to do this.
> 
> Then I think this would be a good way forward in the shirt run.
                                                       ^^^^^^^^^
Oops, I meant "short run", of course...

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

end of thread, other threads:[~2019-03-16  8:31 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-09 17:23 [RFC 0/6] Demangle minimal symbol names in worker threads Tom Tromey
2019-03-09 17:23 ` [RFC 2/6] Remove static buffer from ada_decode Tom Tromey
2019-03-09 17:23 ` [RFC 5/6] Introduce thread-safe way to handle SIGSEGV Tom Tromey
2019-03-11 17:24   ` John Baldwin
2019-03-09 17:23 ` [RFC 1/6] Defer minimal symbol name-setting Tom Tromey
2019-03-09 17:23 ` [RFC 3/6] Lock the demangled hash table Tom Tromey
2019-03-09 17:23 ` [RFC 6/6] Demangle minsyms in parallel Tom Tromey
2019-03-09 17:23 ` [RFC 4/6] Introduce run_on_main_thread Tom Tromey
2019-03-09 18:09 ` [RFC 0/6] Demangle minimal symbol names in worker threads Eli Zaretskii
2019-03-11 17:35   ` John Baldwin
2019-03-11 22:39   ` Tom Tromey
2019-03-12  3:33     ` Eli Zaretskii
2019-03-13 15:38       ` Eli Zaretskii
2019-03-15 23:28         ` Tom Tromey
2019-03-16  7:46           ` Eli Zaretskii
2019-03-16  8:31             ` Eli Zaretskii
2019-03-15 23:40         ` Tom Tromey
2019-03-16  7:52           ` Eli Zaretskii
2019-03-11 17:26 ` John Baldwin
2019-03-11 22:40   ` Tom Tromey

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