diff mbox

Fix usage of inferior_ptid in two thread_alive implementations

Message ID 20170207212450.2232-1-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Feb. 7, 2017, 9:24 p.m. UTC 
  While inspecting some target code, I noticed that in these two
implementations of thread_alive, inferior_ptid is referenced directly
instead of using the ptid passed as parameters.  I guess that it is
wrong, although I can't really test it in both cases.

gdb/ChangeLog:

	* bsd-uthread.c (bsd_uthread_thread_alive): Use ptid instead of
	inferior_ptid.
	* go32-nat.c (go32_thread_alive): Likewise.
---
 gdb/bsd-uthread.c | 2 +-
 gdb/go32-nat.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Pedro Alves Feb. 8, 2017, 12:40 p.m. UTC | #1 
On 02/07/2017 09:24 PM, Simon Marchi wrote:
> While inspecting some target code, I noticed that in these two
> implementations of thread_alive, inferior_ptid is referenced directly
> instead of using the ptid passed as parameters.  I guess that it is
> wrong, although I can't really test it in both cases.

I can't test either, but it looks right to me.  

Thanks,
Pedro Alves
Simon Marchi Feb. 9, 2017, 4:46 p.m. UTC | #2 
On 2017-02-08 07:40, Pedro Alves wrote:
> On 02/07/2017 09:24 PM, Simon Marchi wrote:
>> While inspecting some target code, I noticed that in these two
>> implementations of thread_alive, inferior_ptid is referenced directly
>> instead of using the ptid passed as parameters.  I guess that it is
>> wrong, although I can't really test it in both cases.
> 
> I can't test either, but it looks right to me.

Soooo.. is this an approval :) ?
Pedro Alves Feb. 9, 2017, 5:53 p.m. UTC | #3 
On 02/09/2017 04:46 PM, Simon Marchi wrote:
> On 2017-02-08 07:40, Pedro Alves wrote:
>> On 02/07/2017 09:24 PM, Simon Marchi wrote:
>>> While inspecting some target code, I noticed that in these two
>>> implementations of thread_alive, inferior_ptid is referenced directly
>>> instead of using the ptid passed as parameters.  I guess that it is
>>> wrong, although I can't really test it in both cases.
>>
>> I can't test either, but it looks right to me.
> 
> Soooo.. is this an approval :) ?

OK by the end of the week to give a chance of area
maintainers or interested folks to comment.  E.g., Eli 
is the go32-nat.c maintainer and I don't mean to overstep,
though that bit does look obvious to me.  Mark or someone
with BSD access could want to comment on the BSD bit.  The
latter you could perhaps test on the compile farm, if you
have access.

Thanks,
Pedro Alves
Eli Zaretskii Feb. 9, 2017, 8:46 p.m. UTC | #4 
> Cc: Simon Marchi <simon.marchi@ericsson.com>, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 9 Feb 2017 17:53:16 +0000
> 
> On 02/09/2017 04:46 PM, Simon Marchi wrote:
> > On 2017-02-08 07:40, Pedro Alves wrote:
> >> On 02/07/2017 09:24 PM, Simon Marchi wrote:
> >>> While inspecting some target code, I noticed that in these two
> >>> implementations of thread_alive, inferior_ptid is referenced directly
> >>> instead of using the ptid passed as parameters.  I guess that it is
> >>> wrong, although I can't really test it in both cases.
> >>
> >> I can't test either, but it looks right to me.
> > 
> > Soooo.. is this an approval :) ?
> 
> OK by the end of the week to give a chance of area
> maintainers or interested folks to comment.  E.g., Eli 
> is the go32-nat.c maintainer and I don't mean to overstep,
> though that bit does look obvious to me.

Is that function even called in the go32 port?
Simon Marchi Feb. 22, 2017, 9:06 p.m. UTC | #5 
On 2017-02-09 15:46, Eli Zaretskii wrote:
>> Cc: Simon Marchi <simon.marchi@ericsson.com>, 
>> gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Thu, 9 Feb 2017 17:53:16 +0000
>> 
>> On 02/09/2017 04:46 PM, Simon Marchi wrote:
>> > On 2017-02-08 07:40, Pedro Alves wrote:
>> >> On 02/07/2017 09:24 PM, Simon Marchi wrote:
>> >>> While inspecting some target code, I noticed that in these two
>> >>> implementations of thread_alive, inferior_ptid is referenced directly
>> >>> instead of using the ptid passed as parameters.  I guess that it is
>> >>> wrong, although I can't really test it in both cases.
>> >>
>> >> I can't test either, but it looks right to me.
>> >
>> > Soooo.. is this an approval :) ?
>> 
>> OK by the end of the week to give a chance of area
>> maintainers or interested folks to comment.  E.g., Eli
>> is the go32-nat.c maintainer and I don't mean to overstep,
>> though that bit does look obvious to me.
> 
> Is that function even called in the go32 port?

Why wouldn't it?  For example, you could have this pseudo-stack:

#0 go32_thread_alive
#1 target_thread_alive
#2 thread_alive
#3 thread_apply_all_command

If this target doesn't support multiple threads, it's possible that 
inferior_ptid will always be equal to ptid (equal to the only existing 
thread).  But still it would be "more correct" to read the parameter 
instead of the global, IMO.

Simon
Eli Zaretskii Feb. 23, 2017, 3:34 a.m. UTC | #6 
> Date: Wed, 22 Feb 2017 16:06:50 -0500
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: Pedro Alves <palves@redhat.com>, simon.marchi@ericsson.com,
>  gdb-patches@sourceware.org
> 
> > Is that function even called in the go32 port?
> 
> Why wouldn't it?  For example, you could have this pseudo-stack:
> 
> #0 go32_thread_alive
> #1 target_thread_alive
> #2 thread_alive
> #3 thread_apply_all_command
> 
> If this target doesn't support multiple threads, it's possible that 
> inferior_ptid will always be equal to ptid (equal to the only existing 
> thread).

Go32 indeed doesn't support multiple threads.

> But still it would be "more correct" to read the parameter instead
> of the global, IMO.

I didn't say anything to the contrary.  I'm saying that if go32
doesn't have this function called, I don't have to worry about the
implications of the change.
Simon Marchi Feb. 23, 2017, 3:57 p.m. UTC | #7 
On 2017-02-22 22:34, Eli Zaretskii wrote:
>> But still it would be "more correct" to read the parameter instead
>> of the global, IMO.
> 
> I didn't say anything to the contrary.  I'm saying that if go32
> doesn't have this function called, I don't have to worry about the
> implications of the change.

Well, I'm saying that go32 _can_ have this function called.  So you 
should worry about the implications :).

Anyhow, given that go32 doesn't have multiple threads (thanks for 
confirming), the patch shouldn't have any practical impact, so I've 
pushed it.

Thanks,

Simon
John Baldwin March 29, 2017, 7:32 p.m. UTC | #8 
On Thursday, February 09, 2017 05:53:16 PM Pedro Alves wrote:
> On 02/09/2017 04:46 PM, Simon Marchi wrote:
> > On 2017-02-08 07:40, Pedro Alves wrote:
> >> On 02/07/2017 09:24 PM, Simon Marchi wrote:
> >>> While inspecting some target code, I noticed that in these two
> >>> implementations of thread_alive, inferior_ptid is referenced directly
> >>> instead of using the ptid passed as parameters.  I guess that it is
> >>> wrong, although I can't really test it in both cases.
> >>
> >> I can't test either, but it looks right to me.
> > 
> > Soooo.. is this an approval :) ?
> 
> OK by the end of the week to give a chance of area
> maintainers or interested folks to comment.  E.g., Eli 
> is the go32-nat.c maintainer and I don't mean to overstep,
> though that bit does look obvious to me.  Mark or someone
> with BSD access could want to comment on the BSD bit.  The
> latter you could perhaps test on the compile farm, if you
> have access.

[ Apologies for the late reply ]

I don't think any modern versions of either FreeBSD or OpenBSD
use bsd-uthread.c.  The uthread target is for a thread implementation
that only provided threads in userland on top of a single process
(like "green" threads in the JDK).  FreeBSD last shipped a release
with support for this thread model (FreeBSD 6.4) in 2008.  I can't
speak to when OpenBSD stopped using user-based threads, but the
existence of kernel-based thread support in the obsd-nat target
implies it isn't used anymore either.  For FreeBSD at least I think
it would be fine to remove bsd-uthread.c.

(I should also send a patch in to remove FreeBSD/alpha as that
platform was retired at around the same time.  The last release
to support alpha was 6.3 also released in 2008.)
Simon Marchi March 30, 2017, 12:50 a.m. UTC | #9 
On 2017-03-29 15:32, John Baldwin wrote:
> I don't think any modern versions of either FreeBSD or OpenBSD
> use bsd-uthread.c.  The uthread target is for a thread implementation
> that only provided threads in userland on top of a single process
> (like "green" threads in the JDK).  FreeBSD last shipped a release
> with support for this thread model (FreeBSD 6.4) in 2008.  I can't
> speak to when OpenBSD stopped using user-based threads, but the
> existence of kernel-based thread support in the obsd-nat target
> implies it isn't used anymore either.  For FreeBSD at least I think
> it would be fine to remove bsd-uthread.c.
> 
> (I should also send a patch in to remove FreeBSD/alpha as that
> platform was retired at around the same time.  The last release
> to support alpha was 6.3 also released in 2008.)

Thanks for the reply, this is very valuable information.

I would appreciate very much any effort to remove obsolete targets, as 
it has some maintenance cost.  For example, doing refactors that require 
touching all the targets (such as changing the interface of a target_ops 
method) is very long.  If we can remove any target that has no reason to 
be today, it can only help.

Simon
diff mbox

Patch

diff --git a/gdb/bsd-uthread.c b/gdb/bsd-uthread.c
index 41c7d59be1..20eecd3879 100644
--- a/gdb/bsd-uthread.c
+++ b/gdb/bsd-uthread.c
@@ -401,7 +401,7 @@  bsd_uthread_thread_alive (struct target_ops *ops, ptid_t ptid)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
   struct target_ops *beneath = find_target_beneath (ops);
-  CORE_ADDR addr = ptid_get_tid (inferior_ptid);
+  CORE_ADDR addr = ptid_get_tid (ptid);
 
   if (addr != 0)
     {
diff --git a/gdb/go32-nat.c b/gdb/go32-nat.c
index 3e9e99f068..1fca8e2e53 100644
--- a/gdb/go32-nat.c
+++ b/gdb/go32-nat.c
@@ -938,7 +938,7 @@  go32_terminal_ours (struct target_ops *self)
 static int
 go32_thread_alive (struct target_ops *ops, ptid_t ptid)
 {
-  return !ptid_equal (inferior_ptid, null_ptid);
+  return !ptid_equal (ptid, null_ptid);
 }
 
 static char *