[djgpp/commit] Fix go32_pid_to_str and go32_thread_alive

[djgpp/commit] Fix go32_pid_to_str and go32_thread_alive

[Eli Zaretskii]

The patch below fixes what GDB prints to identify the inferior
process, and also avoids thinking that null_ptid is a live thread.

Committed.

2009-05-01  Eli Zaretskii  <eliz@gnu.org>

	* go32-nat.c (go32_pid_to_str): Call normal_pid_to_str instead of
	printing a bogus "Thread <main>".
	(go32_thread_alive): Don't return 1 for null_ptid.

Index: gdb/go32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/go32-nat.c,v
retrieving revision 1.71
diff -u -r1.71 go32-nat.c
--- gdb/go32-nat.c	19 Apr 2009 18:29:33 -0000	1.71
+++ gdb/go32-nat.c	1 May 2009 08:12:36 -0000
@@ -870,15 +870,13 @@
 static int
 go32_thread_alive (struct target_ops *ops, ptid_t ptid)
 {
-  return 1;
+  return !ptid_equal (inferior_ptid, null_ptid);
 }
 
 static char *
 go32_pid_to_str (struct target_ops *ops, ptid_t ptid)
 {
-  static char buf[64];
-  xsnprintf (buf, sizeof buf, "Thread <main>");
-  return buf;
+  return normal_pid_to_str (ptid);
 }
 
 static void

Re: [djgpp/commit] Fix go32_pid_to_str and go32_thread_alive

[Pedro Alves]

On Friday 01 May 2009 09:17:15, Eli Zaretskii wrote:

> 2009-05-01  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* go32-nat.c (go32_pid_to_str): Call normal_pid_to_str instead of
> 	printing a bogus "Thread <main>".

I thought that the inferior's PID on DJGPP is always the
fake SOMEPID (42), an internal implementation detail, that we'd
never want to show to the user, but, normal_pid_to_str will
print "process 42" here.  Isn't that bogus as well?

> 	(go32_thread_alive): Don't return 1 for null_ptid.

Interesting.  This may be masking some other problem.  How
did you get here with inferior_ptid == null_ptid?  AFAICS,
when the inferior exits or is killed, the go32_ops target
is unpushed.

> 
> Index: gdb/go32-nat.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/go32-nat.c,v
> retrieving revision 1.71
> diff -u -r1.71 go32-nat.c
> --- gdb/go32-nat.c	19 Apr 2009 18:29:33 -0000	1.71
> +++ gdb/go32-nat.c	1 May 2009 08:12:36 -0000
> @@ -870,15 +870,13 @@
>  static int
>  go32_thread_alive (struct target_ops *ops, ptid_t ptid)
>  {
> -  return 1;
> +  return !ptid_equal (inferior_ptid, null_ptid);
>  }
>  
>  static char *
>  go32_pid_to_str (struct target_ops *ops, ptid_t ptid)
>  {
> -  static char buf[64];
> -  xsnprintf (buf, sizeof buf, "Thread <main>");
> -  return buf;
> +  return normal_pid_to_str (ptid);
>  }
>  
>  static void
> 

-- 
Pedro Alves

Re: [djgpp/commit] Fix go32_pid_to_str and go32_thread_alive

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Fri, 1 May 2009 11:18:51 +0100
> 
> On Friday 01 May 2009 09:17:15, Eli Zaretskii wrote:
> 
> > 2009-05-01  Eli Zaretskii  <eliz@gnu.org>
> > 
> > 	* go32-nat.c (go32_pid_to_str): Call normal_pid_to_str instead of
> > 	printing a bogus "Thread <main>".
> 
> I thought that the inferior's PID on DJGPP is always the
> fake SOMEPID (42)

True.

> an internal implementation detail, that we'd
> never want to show to the user, but, normal_pid_to_str will
> print "process 42" here.  Isn't that bogus as well?

It is, to some degree.  But IMO talking about threads in an
environment that doesn't support threads is a greater lie, so
to say ;-)  I wanted the output to be more like when one debugs
a single-threaded program on Unix, because when I first saw
that "Thread <main>" displayed by "set debug infrun 1", I
thought there's some bug somewhere whereby GDB thinks it's
debugging a multithreaded program. 

DJGPP users are already used to see 42 popping here and there; e.g.,
that's what getuid and getgid return for the only user and group they
support.

I briefly considered a possibility to add a function to more
faithfully fake the PID of the inferior (DJGPP's version of getpid
simply returns the current BIOS clock tick counter, so I could fetch
that from GDB), but eventually I decided it wasn't worth the hassle.

> > 	(go32_thread_alive): Don't return 1 for null_ptid.
> 
> Interesting.  This may be masking some other problem.  How
> did you get here with inferior_ptid == null_ptid?

I didn't.  It just seemed unclean to me to return 1 for null_ptid,
when just a little ways above, in go32_stop, we assign it when we
delete the single alive thread.

> AFAICS, when the inferior exits or is killed, the go32_ops target is
> unpushed.

I have no doubt that you are right, but at some future point this
method could potentially be called in some other context.

If you think my change may mask other problems, now or in the future,
I'm okay with adding some appropriate gdb_assert here.

Thanks for the feedback.

Re: [djgpp/commit] Fix go32_pid_to_str and go32_thread_alive

[Pedro Alves]

On Friday 01 May 2009 13:57:40, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Fri, 1 May 2009 11:18:51 +0100
> > 
> > On Friday 01 May 2009 09:17:15, Eli Zaretskii wrote:
> > 
> > > 2009-05-01  Eli Zaretskii  <eliz@gnu.org>
> > > 
> > > 	* go32-nat.c (go32_pid_to_str): Call normal_pid_to_str instead of
> > > 	printing a bogus "Thread <main>".
> > 
> > I thought that the inferior's PID on DJGPP is always the
> > fake SOMEPID (42)
> 
> True.
> 
> > an internal implementation detail, that we'd
> > never want to show to the user, but, normal_pid_to_str will
> > print "process 42" here.  Isn't that bogus as well?
> 
> It is, to some degree.  But IMO talking about threads in an
> environment that doesn't support threads is a greater lie, so
> to say ;-)  

Okay, but I disagree with the lying :-)  There's no kernel
multi-thread support, or a thread library, but there's still
a thread of control here.

> I wanted the output to be more like when one debugs 
> a single-threaded program on Unix, because when I first saw
    ^^^^^^^^^^^^^^^

see, "single-threaded" implies a single (== 1) thread.  :-)  From
GDB's perpective there is a thread of control here.  There's
no thread support library, though, but that's a bit different.
GDB doesn't care much.

> that "Thread <main>" displayed by "set debug infrun 1", I
> thought there's some bug somewhere whereby GDB thinks it's
> debugging a multithreaded program. 
> 
> DJGPP users are already used to see 42 popping here and there; e.g.,
> that's what getuid and getgid return for the only user and group they
> support.

Ah, I see.  Makes sense then.

> I briefly considered a possibility to add a function to more
> faithfully fake the PID of the inferior (DJGPP's version of getpid
> simply returns the current BIOS clock tick counter, so I could fetch
> that from GDB), but eventually I decided it wasn't worth the hassle.

Yeah.

> > > 	(go32_thread_alive): Don't return 1 for null_ptid.
> > 
> > Interesting.  This may be masking some other problem.  How
> > did you get here with inferior_ptid == null_ptid?
> 
> I didn't.  It just seemed unclean to me to return 1 for null_ptid,
> when just a little ways above, in go32_stop, we assign it when we
> delete the single alive thread.
> 
> > AFAICS, when the inferior exits or is killed, the go32_ops target is
> > unpushed.
> 
> I have no doubt that you are right, but at some future point this
> method could potentially be called in some other context.
> 

If in non-stop mode, and debugging more than one process
simultaneously (multiprocess mode), you may get here without any
thread selected, thus inferior_ptid == null_ptid, but, there may
the live threads in the thread list.  This function should never
rely or care about what's in inferior_ptid, but instead only consider
the ptid passed in as argument.  But, DJGPP doesn't support
non-stop or multiprocess, and single-process + non-stop doesn't
make much sense for a target that doesn't support multi-threading,
of course.

> If you think my change may mask other problems, now or in the future,
> I'm okay with adding some appropriate gdb_assert here.

I can't predict the future, so I always prefer not putting asserts like
that, since if the design direction changes, we may trip on an assert for no
good reason.  I don't think djgpp will even gain multi-process + non-stop
modes, so, it doesn't matter much either way.  I prefered the previous
version as it was simpler in that it didn't assume anything at all ---
the current version added a bit of dead code, that makes the reader
stop and consider why it is done the way it is --- , but it's fine
with me either way, and it's really your call.

> Thanks for the feedback.

Thanks for explaining!

-- 
Pedro Alves