Re: [PATCH 2/4] Push pruning old threads down to the target

[Thomas Schwinge]

Hi!

On Thu,  2 Oct 2014 17:21:34 +0100, Pedro Alves  wrote:
> When GDB wants to sync the thread list with the target's [...]

> --- a/gdb/thread.c
> +++ b/gdb/thread.c

>  void
>  update_thread_list (void)
>  {
> -  prune_threads ();
> -  target_find_new_threads ();
> +  target_update_thread_list ();
>update_threads_executing ();
>  }

Pushed to master:

commit c752a4cccb99ba73f51eff74b394dcdcd26d4c59
Author: Thomas Schwinge 
Date:   Wed May 25 18:54:40 2016 +0200

Hurd: Adjust to changes to "push pruning old threads down to the target"

For "info threads", we currently run into:

$ gdb/gdb -q -nw -nx --batch -ex start -ex info\ threads bfd/doc/chew
Temporary breakpoint 1 at 0x80486e0: file 
../../../W._C._Handy/bfd/doc/chew.c, line 1535.
[New Thread 10656.5]

Thread 4 hit Temporary breakpoint 1, main (ac=1, av=0x102cd84) at 
../../../W._C._Handy/bfd/doc/chew.c:1535
1535{
  Id   Target Id Frame
  1bogus thread id 1 Can't fetch registers from thread bogus thread 
id 1: No such thread

Before commit e8032dde10b743253125d7defb5f5503b21c1d26,
gdb/thread.c:update_thread_list used to call prune_threads, after that 
change
it doesn't anymore, and we don't implement the to_update_thread_list target
method where the prune_threads call got moved.  For now, apply a fix, 
related
to commit c82f56d9d760a9b4034eeaac44f2f0fa5779ff69 "Hurd: Adjust to
startup-with-shell changes", which restores the previous behavior:

  Id   Target Id Frame
* 4Thread 10688.4main (ac=1, av=0x102cd84) at 
../../../W._C._Handy/bfd/doc/chew.c:1535
  5Thread 10688.50x0106096c in ?? () from 
/lib/i386-gnu/libc.so.0.3

Not perfect, but at least better.

gdb/
* gnu-nat.c (gnu_create_inferior): After startup_inferior, call
prune_threads.
---
 gdb/ChangeLog | 3 +++
 gdb/gnu-nat.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git gdb/ChangeLog gdb/ChangeLog
index 302eb6e..7fd5d32 100644
--- gdb/ChangeLog
+++ gdb/ChangeLog
@@ -1,5 +1,8 @@
 2016-12-09  Thomas Schwinge  
 
+   * gnu-nat.c (gnu_create_inferior): After startup_inferior, call
+   prune_threads.
+
* inferior.c (print_selected_inferior): Avoid PATH_MAX usage.
 
 2016-12-08  Simon Marchi  
diff --git gdb/gnu-nat.c gdb/gnu-nat.c
index 124574e..85a53b8 100644
--- gdb/gnu-nat.c
+++ gdb/gnu-nat.c
@@ -2163,6 +2163,8 @@ gnu_create_inferior (struct target_ops *ops,
 
   startup_inferior (START_INFERIOR_TRAPS_EXPECTED);
   inf->pending_execs = 0;
+  /* Get rid of the old shell threads.  */
+  prune_threads ();
 
   inf_validate_procinfo (inf);
   inf_update_signal_thread (inf);


Grüße
 Thomas

Re: Heads up: Recent status: emacs24/25 FTBFS since a long time on GNU/Hurd

[Svante Signell]

On Thu, 2016-12-08 at 14:47 +0100, Richard Braun wrote:
> On Thu, Dec 08, 2016 at 10:44:09AM +0100, Svante Signell wrote:
> > Since a long time emacs FTBFS due to unknown reasons. The latest version
> > building was Debian 24.5+1-5, from 28 Nov 2015.
> 
> As already mentioned, the real issue is in Emacs. See the relevant
> LWN article [1] for details.

I've read it, thanks! I think emacs is in a similar situation as Hurd with
respect to the still missing mlockall/munlockall functions.

> > Even before successful builds were by pure luck. One suspicious issue is
> > that emacs use sbrk() for memory allocation, right? Notably sbrk() is not
> > fool-proof as implemented for Hurd in glibc: Is it or is it not?
> 
> No it's not. And this is the real point I want to make in this message.
> 
> For performance reasons, the implementation of virtual memory maps
> was changed in GNU Mach [2]. 

> GNU Mach
> used to implement a bottom-up policy prior to this change, on which
> the sbrk() implementation relied. This is no longer true. Mappings can
> be created anywhere in the map, and it turns out that some are created
> immediately following the heap, preventing sbrk() from doing its job.

OK! Then maybe the sbrk() feature should be flagged as not available in order
not to fool configure and the compiler. In fact FreeBSD/arm64 did exactly that,
see https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24892 So that platform is on
the same par as GNU/Hurd then. On all other supported platforms emacs builds and
runs perfectly, though.

> Now, we could fix this by doing the same thing Linux does, but as usual,
> someone has to do it, and the benefits aren't that big. Our virtual
> spaces have never been as tidy as on other systems, and it doesn't
> prevent most programs from working just fine. It usually matters for
> things like emulators, such as Wine,

Wine already builds and works on Hurd when I last tested it. Do you mean it does
not work any longer, it's been some time since I tried it?

> or debuggers like Valgrind,

There are efforts to port Valgrind to Hurd too, It's even in the TODO project
list. What to do, remove that task from the list?

> which
> have strong requirements regarding address values, but we currently
> don't build them because other dependencies are missing.

See above.

> In other words, we can't go back for now, especially not just to make
> an obsolete interface used by only one piece of software that is
> likely to fix the issue soon. Unless you really want to work on those
> red-black trees, stop pinging about this issue.

The current discussion on emacs-devel reveals that unexec will not be replaced
in the near future. The main discussion is about if the dumper should be written
in C or elisp. Until then, unexec will probably not be removed from emacs
upstream. Maybe not until the needed features are removed from glibc. (similar
situation as mlockall/munlockall on Hurd)

And: Consider vi* not building for more than a year, due to the same reason,
what would you have done then? Not everybody use vi* as their editor.

Re: Heads up: Recent status: emacs24/25 FTBFS since a long time on GNU/Hurd

[Richard Braun]

On Thu, Dec 08, 2016 at 10:44:09AM +0100, Svante Signell wrote:
> Since a long time emacs FTBFS due to unknown reasons. The latest version
> building was Debian 24.5+1-5, from 28 Nov 2015.

As already mentioned, the real issue is in Emacs. See the relevant
LWN article [1] for details.

> Even before successful builds were by pure luck. One suspicious issue is that
> emacs use sbrk() for memory allocation, right? Notably sbrk() is not 
> fool-proof
> as implemented for Hurd in glibc: Is it or is it not?

No it's not. And this is the real point I want to make in this message.

For performance reasons, the implementation of virtual memory maps
was changed in GNU Mach [2]. Basically, VM maps maintain a red-black
tree of holes for O(log(n)) allocations of virtual memory. This is what
Linux does too, except they have "augmented" their red-black tree to
use the one that already stores entries, instead of adding one for
holes.

It also works a bit differently because it still allows
bottom-up / top-down allocations. That's the big difference. GNU Mach
used to implement a bottom-up policy prior to this change, on which
the sbrk() implementation relied. This is no longer true. Mappings can
be created anywhere in the map, and it turns out that some are created
immediately following the heap, preventing sbrk() from doing its job.

Now, we could fix this by doing the same thing Linux does, but as usual,
someone has to do it, and the benefits aren't that big. Our virtual
spaces have never been as tidy as on other systems, and it doesn't
prevent most programs from working just fine. It usually matters for
things like emulators, such as Wine, or debuggers like Valgrind, which
have strong requirements regarding address values, but we currently
don't build them because other dependencies are missing.

On the other hand, the performance improvement is really, really needed.
It is now common, even on monolithic systems, to have processes with
thousands of entries in their address space, but keep in mind the Hurd
is a multi-server system, which basically means that what would be in
the kernel for Linux is in a userspace process on the Hurd, and with
the improvements on the page cache, it's actually normal for an ext2fs
server to have between 1k and 10k map entries, sometimes more. Here
is an example, run on one of our buildd systems :

# vminfo -v 410 | wc -l
20157

And that's one file system instance. At the time, the system was using
a total of around 56k map entries.

In other words, we can't go back for now, especially not just to make
an obsolete interface used by only one piece of software that is
likely to fix the issue soon. Unless you really want to work on those
red-black trees, stop pinging about this issue.

-- 
Richard Braun

[1] https://lwn.net/Articles/673724/
[2] 
https://git.sceen.net/hurd/gnumach.git/commit/?id=1db202eb8406785500f1bc3ceef7868566e416a1

Re: [PATCH v3 1/2] Emit inferior, thread and frame selection events to all UIs

[Thomas Schwinge]

Hi!

On Sat, 24 Sep 2016 16:13:30 -0400, Simon Marchi  
wrote:
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c

> +void
> +print_selected_inferior (struct ui_out *uiout)
> +{
> +  char buf[PATH_MAX + 256];
> +  struct inferior *inf = current_inferior ();
> +
> +  xsnprintf (buf, sizeof (buf),
> +  _("[Switching to inferior %d [%s] (%s)]\n"),
> +  inf->num,
> +  inferior_pid_to_str (inf->pid),
> +  (inf->pspace->pspace_exec_filename != NULL
> +   ? inf->pspace->pspace_exec_filename
> +   : _("")));
> +  ui_out_text (uiout, buf);
> +}
> +

On GNU/Hurd, there is no "#define PATH_MAX", so this fails to build.
(I'm aware that there is other PATH_MAX usage in GDB sources, which we
ought to fix at some point, for example in gdbserver -- which is not yet
enabled for GNU/Hurd.)

Unless I miss something, this issue could be addressed by simply using
ui_out_message instead of ui_out_text with a temporary "buf" -- OK to
push the following?

--- gdb/inferior.c
+++ gdb/inferior.c
@@ -556,17 +556,15 @@ inferior_pid_to_str (int pid)
 void
 print_selected_inferior (struct ui_out *uiout)
 {
-  char buf[PATH_MAX + 256];
   struct inferior *inf = current_inferior ();
 
-  xsnprintf (buf, sizeof (buf),
-_("[Switching to inferior %d [%s] (%s)]\n"),
-inf->num,
-inferior_pid_to_str (inf->pid),
-(inf->pspace->pspace_exec_filename != NULL
- ? inf->pspace->pspace_exec_filename
- : _("")));
-  ui_out_text (uiout, buf);
+  ui_out_message (uiout,
+ _("[Switching to inferior %d [%s] (%s)]\n"),
+ inf->num,
+ inferior_pid_to_str (inf->pid),
+ (inf->pspace->pspace_exec_filename != NULL
+  ? inf->pspace->pspace_exec_filename
+  : _("")));
 }
 
 /* Prints the list of inferiors and their details on UIOUT.  This is a


Grüße
 Thomas

Re: [pushed][PATCH v3 1/4] Extended-remote follow exec

[Thomas Schwinge]

Hi!

On Fri, 11 Sep 2015 11:38:15 -0700, Don Breazeal  wrote:
> Here is what I pushed.

> --- a/gdb/remote.c
> +++ b/gdb/remote.c

> @@ -6089,11 +6178,42 @@ Packet: '%s'\n"),
> event->ws.kind = TARGET_WAITKIND_VFORK_DONE;
> p = skip_to_semicolon (p1 + 1);
>   }
> +   else if (strncmp (p, "exec", p1 - p) == 0)
> + {
> +   ULONGEST ignored;
> +   char pathname[PATH_MAX];
> +   int pathlen;
> +
> +   /* Determine the length of the execd pathname.  */
> +   p = unpack_varlen_hex (++p1, );
> +   pathlen = (p - p1) / 2;
> +
> +   /* Save the pathname for event reporting and for
> +  the next run command.  */
> +   hex2bin (p1, (gdb_byte *) pathname, pathlen);
> +   pathname[pathlen] = '\0';
> +
> +   /* This is freed during event handling.  */
> +   event->ws.value.execd_pathname = xstrdup (pathname);
> +   event->ws.kind = TARGET_WAITKIND_EXECD;
> +
> +   /* Skip the registers included in this packet, since
> +  they may be for an architecture different from the
> +  one used by the original program.  */
> +   skipregs = 1;
> + }

On GNU/Hurd, there is no "#define PATH_MAX", so this fails to build.
(I'm aware that there is other PATH_MAX usage in GDB sources, which we
ought to fix at some point, for example in gdbserver -- which is not yet
enabled for GNU/Hurd.)

OK to push the following?  (Similar to Svante's patch in
.)

--- gdb/remote.c
+++ gdb/remote.c
@@ -6927,7 +6927,6 @@ Packet: '%s'\n"),
  else if (strprefix (p, p1, "exec"))
{
  ULONGEST ignored;
- char pathname[PATH_MAX];
  int pathlen;
 
  /* Determine the length of the execd pathname.  */
@@ -6936,11 +6935,12 @@ Packet: '%s'\n"),
 
  /* Save the pathname for event reporting and for
 the next run command.  */
+ char *pathname = (char *) xmalloc (pathlen + 1);
  hex2bin (p1, (gdb_byte *) pathname, pathlen);
  pathname[pathlen] = '\0';
 
  /* This is freed during event handling.  */
- event->ws.value.execd_pathname = xstrdup (pathname);
+ event->ws.value.execd_pathname = pathname;
  event->ws.kind = TARGET_WAITKIND_EXECD;
 
  /* Skip the registers included in this packet, since


Grüße
 Thomas

Heads up: Recent status: emacs24/25 FTBFS since a long time on GNU/Hurd

[Svante Signell]

Hello bug-hurd ML,

Since a long time emacs FTBFS due to unknown reasons. The latest version
building was Debian 24.5+1-5, from 28 Nov 2015.

Even before successful builds were by pure luck. One suspicious issue is that
emacs use sbrk() for memory allocation, right? Notably sbrk() is not fool-proof
as implemented for Hurd in glibc: Is it or is it not?
Use of sbrk is found in files alloc.c, unexelf.c and gmalloc.c, which are
all compiled. Avoiding compilation of ralloc.c with 0001-Default-REL_ALLOC-to-
no.patch did not improve the situation.

First time I compiled emacs 25.1 from upstream it passed, second time not.
Compiling Debian versions always fails now (almost always a year ago, see
below). Mostly the build fails with temacs failing to execute: Killed or dumping
core depending on crsh server settings. In my oponion it's a real loss not to
gave a modern version of emacs25 available for use in GNU/Hurd (not everybody
use vi).

As written on IRC yesterday:
(23:17:22) srs: braunr: I'd like to add a few facts about emacs building on
GNU/Hurd: Latest successful build was 24.1+1-5 on 28 Nov 2015.
(23:17:22) srs: Installed then was: glibc-2.19-22, hurd-0.7-1, gnumach-1.6-1.
Trying to build that version now fails miserably,
(23:17:22) srs: (I just did that as well as on Nov 11 2016) so it is not only
not yet supported causing the build failure :(
(23:17:22) srs: The missing functions were not implemented  a year ago either.

And here is the latest gdb trace using the core file:
(unfortunately the rpctrace output is too large to include here)

emacs25-25.1+1/debian/build-x$ gdb ./src/bootstrap-emacs -c lisp/core
...
warning: core file may not match specified executable file.
[New process 24043]
[New process 1]

warning: Unexpected size of section `.reg2/24043' in core file.
Core was generated by `../src/bootstrap-emacs -batch --no-site-lisp --no-site-
file --eval (setq max-lis'.
Program terminated with signal SIGSEGV, Segmentation fault.

warning: Unexpected size of section `.reg2/24043' in core file.
#0  0x081a5c32 in backtrace_top () at eval.c:184
184 eval.c: No such file or directory.
[Current thread is 1 (process 24043)]
(gdb) info thre
  Id   Target Id Frame 
* 1process 24043 0x081a5c32 in backtrace_top () at eval.c:184
  2process 1 warning: Unexpected size of section `.reg2/1' in core
file.
0x0520c9ac in ?? ()
(gdb) thread apply all bt full

Thread 2 (process 1):
#0  0x0520c9ac in ?? ()
No symbol table info available.

Thread 1 (process 24043):
#0  0x081a5c32 in backtrace_top () at eval.c:184
pdl = 0x358d78
#1  near_C_stack_top () at eval.c:202
No locals.
#2  0x0814aaed in stack_overflow (siginfo=0x84d573c )
at sysdep.c:1659
addr = 0x8d31cd8 
bot = 0x98128c7 ""
top = 
#3  handle_sigsegv (sig=11, siginfo=0x84d573c , 
arg=0x84d5548 ) at sysdep.c:1691
fatal = false
#4  0x0522da72 in ?? ()
No symbol table info available.
#5  0x000b in ?? ()
No symbol table info available.
#6  0x084d573c in sigsegv_stack ()
No symbol table info available.
#7  0x084d5548 in sigsegv_stack ()
No symbol table info available.
#8  0x05257600 in ?? ()
No symbol table info available.
#9  0x0522da76 in ?? ()
No symbol table info available.
#10 0x084d5460 in sigsegv_stack ()
No symbol table info available.
#11 0x0001 in ?? ()
No symbol table info available.
#12 0x in ?? ()
No symbol table info available.
#0  0x0520c9ac in ?? ()
  Id   Target Id Frame 
  1process 24043 0x081a5c32 in backtrace_top () at eval.c:184
* 2process 1 0x0520c9ac in ?? ()

Re: [PATCH v3 4/7] Per-inferior/Inferior-qualified thread IDs

[Thomas Schwinge]

Hi Simon!

On Wed, 7 Dec 2016 16:28:58 -0500, Simon Marchi  
wrote:
> On 16-12-07 04:09 PM, Thomas Schwinge wrote:
> > commit 14f6890677849172a4b13779acd9089c9baa3a81
> > Author: Thomas Schwinge 
> > Date:   Tue May 24 19:36:57 2016 +0200
> > 
> > Hurd: Adjust to "Per-inferior/Inferior-qualified thread IDs" changes
> > 
> > [...]/gdb/gnu-nat.c: In function 'set_sig_thread_cmd':
> > [...]/gdb/gnu-nat.c:2973:7: warning: implicit declaration of 
> > function 'thread_id_to_pid' [-Wimplicit-function-declaration]
> >ptid_t ptid = thread_id_to_pid (atoi (args));
> >^
> > [...]/gdb/gnu-nat.c:2973:7: error: invalid initializer
> > 
> > That's commit 5d5658a1d3c3eb2a09c03f2f0662a1c01963c869, which renamed
> > `thread_id_to_pid` to `global_thread_id_to_ptid`.

> > --- gdb/gnu-nat.c
> > +++ gdb/gnu-nat.c
> > @@ -2964,7 +2964,7 @@ set_sig_thread_cmd (char *args, int from_tty)
> >  inf->signal_thread = 0;
> >else
> >  {
> > -  ptid_t ptid = thread_id_to_pid (atoi (args));
> > +  ptid_t ptid = global_thread_id_to_ptid (atoi (args));
> 
> I think your patch is better than the status quo, since it fixes the build.
> However, global_thread_id_to_ptid expects global thread numbers, which are
> nowadays only used in MI, never presented to the user in the CLI.  Since this
> is a CLI command, it should accept the inferior-qualified format instead.

Thanks for the review!

> Something similar to this should do the trick (I can't test it though):

> --- a/gdb/gnu-nat.c
> +++ b/gdb/gnu-nat.c
> @@ -2964,13 +2964,13 @@ set_sig_thread_cmd (char *args, int from_tty)
>  inf->signal_thread = 0;
>else
>  {
> -  ptid_t ptid = thread_id_to_pid (atoi (args));
> +  struct thread_info *tp = parse_thread_id (args, NULL)
> 
> -  if (ptid_equal (ptid, minus_one_ptid))
> +  if (tp == nullptr)
> error (_("Thread ID %s not known.  "
>  "Use the \"info threads\" command to\n"
>"see the IDs of currently known threads."), args);
> -  inf->signal_thread = inf_tid_to_thread (inf, ptid_get_lwp (ptid));
> +  inf->signal_thread = inf_tid_to_thread (inf, ptid_get_lwp (tp->ptid));
>  }
>  }

Thanks for the patch!  Actually, parse_thread_id will never return NULL
("Either a valid thread is returned, or an error is thrown."), so that
can be made even simpler.  (As also the parse_thread_id usage in
gdb/thread.c doesn't use ENDPTR to check for "junk" after the thread ID,
I don't see a need to do that here.)  ;-)

> If there's a more efficient/concise way to go from a thread_info* to a proc*, 
> we
> should probably use it, but I couldn't find one.

Indeed that seems to be the standard pattern.

I pushed the following to master:

commit c3187fa5cc72734e6fc766a85d657018c0516bad
Author: Simon Marchi 
Date:   Thu Dec 8 09:45:59 2016 +0100

Hurd: In the CLI, use parse_thread_id instead of global_thread_id_to_ptid

Follow-up to commit 14f6890677849172a4b13779acd9089c9baa3a81.
global_thread_id_to_ptid expects global thread numbers, which are nowadays 
only
used in MI, never presented to the user in the CLI.  Since this is a CLI
command, it should accept the inferior-qualified format instead.

gdb/
* gnu-nat.c (set_sig_thread_cmd): Use parse_thread_id instead of
global_thread_id_to_ptid.
---
 gdb/ChangeLog |  6 ++
 gdb/gnu-nat.c | 12 
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git gdb/ChangeLog gdb/ChangeLog
index 171d03a..2bd99f1 100644
--- gdb/ChangeLog
+++ gdb/ChangeLog
@@ -1,3 +1,9 @@
+2016-12-08  Simon Marchi  
+   Thomas Schwinge  
+
+   * gnu-nat.c (set_sig_thread_cmd): Use parse_thread_id instead of
+   global_thread_id_to_ptid.
+
 2016-12-08  Thomas Schwinge  
 
* config/i386/i386gnu.mh (%_S.o %_U.o): Add "-x c" to
diff --git gdb/gnu-nat.c gdb/gnu-nat.c
index 5fd59a2..124574e 100644
--- gdb/gnu-nat.c
+++ gdb/gnu-nat.c
@@ -63,6 +63,7 @@ extern "C"
 #include "gdbcore.h"
 #include "gdbthread.h"
 #include "gdb_obstack.h"
+#include "tid-parse.h"
 
 #include "gnu-nat.h"
 #include "inf-child.h"
@@ -2975,19 +2976,14 @@ set_sig_thread_cmd (char *args, int from_tty)
 
   if (!args || (!isdigit (*args) && strcmp (args, "none") != 0))
 error (_("Illegal argument to \"set signal-thread\" command.\n"
-  "Should be an integer thread ID, or `none'."));
+"Should be a thread ID, or \"none\"."));
 
   if (strcmp (args, "none") == 0)
 inf->signal_thread = 0;
   else
 {
-  ptid_t ptid = global_thread_id_to_ptid (atoi (args));
-
-  if (ptid_equal (ptid, minus_one_ptid))
-   error (_("Thread ID %s not known.  "
-"Use the \"info threads\" command to\n"
-  "see the IDs