This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: wrong assumptions about pthread_t being numeric


On 10/01/2011 03:59 AM, Pedro Alves wrote:
On Saturday 01 October 2011 01:59:25, John Spencer wrote:
i disagree. adding a proper solution once is superior to creating dozens
of special case hacks.
What's the "proper" solution then?  The only possible way I can think
of to handle some libc what uses a struct for pthread_t, is with some way at
runtime to know which libc/thread_db gdb is talking to, failing that,
#ifdefs and/or autoconfig'ury.  Since a simple cast will
work for glibc, android, musl, and whatever other libc's happen be
be building gdbserver today, what's the point?
I'll even go a step further, and suggest you do expose pthread_t
as a typedef to long in public headers, and cast it to pointer internally
in musl, just like glibc and most other libc's do.  Hiding the fact
that you implement things with a pointer could even be a good thing.

It's just easier for everybody else that way.  There's really no point in
being different for the sake of it.

that's definitely going to far, you can't dictate how libcs should
handle an opaque type internally just to keep broken code working.
The fact that you're still calling gdb's libthread_db.so handling
code broken because of this only shows that you still missed the point
of the code you're talking about.  Let me sum it up:
The debugger is coordinating with a libc-provided shared library, that
is used to peek at the libc's internal state.
that's the disturbing part. there are cleaner ways to get a unique number without
poking at libc internals.
however, the error has been made and it is much effort to correct it, so
let's continue patching for the time being.
   pthread_t's type is
implementation defined.  The debugger's code in question is interacting
with a specific libc implementation (glibc).  For that fact, the
debugger's code in question can be seen as being part of "the
implementation" (as in the pthread_t's type being implementation
defined part).  The fact that other libcs work with the same code
is a bonus.

Again:

that's definitely going to far, you can't dictate how libcs should
handle an opaque type internally just to keep broken code working.
I can at least try to persuade one libc author into sanity. :-)

i'm not musl's author, just helping to get some important packages built ;)

i doubt that the author will change his implementation, since
the solution you proposed will induce a performance penalty on some compilers,
and result in less strict compile-time type checks.


Ever heard of not inflicting unnecessary pain on others for no
good reason?   You know people do sprinkle around printfs of
pthread_t's for debug purposes, don't you?  Things like printing
printf ("%ld been here\n", pthread_self ()).  I really don't see
the point of not being source compatible with glibc (and all other
linux libcs) when it's super easy to be so...

Again:

that's definitely going to far, you can't dictate how libcs should
handle an opaque type internally just to keep broken code working.
public header:

typedef unsigned long pthread_t;

musl implementation:

struct pthread
{
   whatever you have today;
};

int pthread_foo_public_function (pthread_t th, ...)
{
    struct pthread *t = (pthread_t) th;

    // business as usual.
}

There, how did that dictate how your libc handles its
opaque type _internally_ again?  It shouldn't take more than
15 minutes to change all of that in musl, and everyone lives
happy ever after.

there
should be at least a explicit function/macro which takes a thread_t and
converts it to long, since it is assumed in a couple of spots that it is
of this type.
that is exactly what my patch does.

and as you wished, it fixes the current issue with minimal effort.
The patch has a number of problems (no biggie, just the usual for
someone not used to GNU code).  I'll take a look if I still failed
to convince you to change musl instead.


yes, please take a look at the patch.
it imposes no performance costs and makes the code more correct.
also if a new platform shows up, one can simply add a single ifdef to the macro and
all code accessing the thread_t in a numeric context will magically work.


-- JS


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]