Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pthread_t need to be a scalar to satisfy gcc #2

Open
martell opened this issue Oct 6, 2014 · 3 comments
Open

pthread_t need to be a scalar to satisfy gcc #2

martell opened this issue Oct 6, 2014 · 3 comments

Comments

@martell
Copy link

martell commented Oct 6, 2014

Hi @GerHobbelt,

Would you be able to help me with something.

I want to change pthreads.h pthread_t from a struct to a uintptr_t
As gcc assumes a scalar value for c++11 threading

I don't know how to propagate this change throughout pthread without breaking it :(
and I really need todo this

this is the error gcc will produce

libstdc++-v3/include/thread:88:30: error: no match for ‘operator<’ (operand types are ‘std::thread::native_handle_type {aka pte_handle_t}’ and ‘std::thread::native_handle_type {aka pte_handle_t}’) { return __x._M_thread < __y._M_thread; }
@martell
Copy link
Author

martell commented Oct 6, 2014

diff --git a/pthread.h b/pthread.h
index 1d86403..eec5148 100644
--- a/pthread.h
+++ b/pthread.h
@@ -391,12 +391,15 @@ enum
      * that available with a simple pointer. It should scale for either
      * IA-32 or IA-64.
      */
+/*
     typedef struct
       {
-        void * p;                   /* Pointer to actual object */
-        unsigned int x;             /* Extra information - reuse count etc */
+        void * p;                   
+        unsigned int x;             
       } pte_handle_t;
+*/

+    typedef uintptr_t pte_handle_t;
     typedef pte_handle_t pthread_t;
     typedef struct pthread_attr_t_ * pthread_attr_t;
     typedef struct pthread_once_t_ pthread_once_t;

This is what I need to change it to

@GerHobbelt
Copy link
Owner

Hi Martell,

I'm sorry but I'm right now completely swamped in a release run where I'm
sitting in the critical paths (plural :-( ), so all moments of, ah, sanity
are spent on that.

I haven't touched C and pthreads in a while (~ 2-3 years by now), so I'm
running this off my geriatric memory.

Anything I write after this line is:

  1. flow of consciousness and recall kicking in while I write, so the
    statements here are soft, rather than hard guarantees.
  2. check everything I say (as I always advise people to do anyway); I may
    have mixed up one or two issues in my brains; generally I have the habit of
    writing long(ish) comments where I touch complex stuff and I've run into a
    spot of trouble myself.

With those caveats said, off we go:

WinPthreads is 'special' in that it has to be cross-platform compatible
source code (32 and 64-bit support for both int=32-bit and int=64-bit
compiler platforms); I know that one (hairy) way to make sure the type is
big enough for any platform to work on is to use a union and then let the
platform specific code, which is the only part which really cares about the
exact 'meaning' of each bit in there, use the element in the union that
matches the native OS handles (HANDLE=void* while Unix boxes generally
produce int sized handles)

Detail: Windows native APIs, IIRC, produce 64-bit HANDLEs, at least on
Win64, which have to fit in the pthread_t type to be carried around
undamaged, you cannot strip them to 'int' (32-bit) and expect to live
happily ever after, unless you introduce a whole new layer of indirection
so that pthreads indexes into a list (array/hashtable) which then stores
those big HANDLEs; currently WinPthread doesn't do that but uses a union
instead, which is the max size of 'void *' and 'unsigned int'. I mention
the extra layer of indirection only as a thought, but realize that you are
then talking about a major overhaul and added complexity, so the better
question to ask is:

when you are talking about gcc requiring an uintptr_t, that sounds to me
like the pthread_t union then just needs an extra field of uintptr_t type
so that GCC-specific code (and I bet you'll get some nasty surprises too
when it comes to gcc on Win64/Win32 vs. linux/osx vs embedded platforms)
can use that union-member.

What has me confused during writing and thinking about it right now is that
I see a struct in your diff, rather than a union...

I do seem to recall some old trouble with gcc yakking about pointer
aliasing trouble thanks to the use of that union typedef and I remember
that episode as I recall some gcc4 early versions screwing up the generated
code for the union-based code; there's also that comment which says 'extra
information' which looks to me, today, like we might be indeed talking
about a struct because we might be keeping counters in there as a kind of
'smart pointer' emulation (C++ smart pointers are pointer+counter).

So getting out of the deep end and wondering about the higher level
problem:

  • what is going wrong now with the current code that breaks Cx11? Does it
    require an uintptr_t, mandatory, or is that only the new 'optimal' way? I
    haven't looked at Cx11 so I cannot tell you which it is.
  • two directions to investigate the code if uintptr_t is an axiom in the
    new Cx11 reality: see what the counter (.x field) does and who uses it: can
    we do without it and if we kill it, what are the consequences? (potential
    handle/memory leakage on regular threads? on crashed threads? (failure
    modes and their run-time execution paths are always to be considered
    separately to keep a proper focus on them when analysing/writing/reviewing
    code)
  • if all else fails, and gcc is such a b*tch about uintptr_t and nothing
    else, can we apply

#ifdef GCC4_CX11_SOMETHING
typedef uintptr_t pthread_t;
#else
// legacy code: typedef struct bla bla pthread_t;
#endif

style hacks to make it happen on Cx11?

On that same subject: what does it mean exactly: 'gcc requires a scalar
type: uintptr_t'?
When we check the sizeof uintptr_t, is it large enough to handle a
pthread_t when we hard cast the one into the other? Do we need to discard
the .x 'counter' element in that struct and does the cast work then?

That's my starting point: my thinking process when I start to attack the
problem you describe. I hope it helps you a little.
Note that, for me, there are a few unclarities at the very start:

  • what's the gcc limitation/requirement about uintptr_t exactly?
  • Is it God's Will or are there ways around it?
  • where exactly is this requirement enforced? Some function=thread
    parameter, some place else? --> I'ld have to look it up as I don't know the
    boundaries of my uintptr_t restriction.

(I bet there is as C has always strives to be backwards compatible, so it
may not be nice, it may even be very compiler specific and/or platform
specific, but my first bet goes to the side which says: you can tell gcc
it's otherwise... use a #pragma xyz-something or whatever and gcc will
work, while nobody else will.)

And then of course the question nobody would like to ask: why do we use the
WinPthreads library if this is so gcc/Cx11 specific anyway? Why don't we
use the native threads that may be in there (remember, I haven't read the
Cx11 spec yet, so I have no clue. Blog articles on the net are mere hints
to me which can fall either way; what's the reference manual/document
itself say? --> Where can I go and twist its arm?)

My 1 cent. I really hope it helps you move forward.

Met vriendelijke groeten / Best regards,

Ger Hobbelt


web: http://www.hobbelt.com/
http://www.hebbut.net/
mail: [email protected]

mobile: +31-6-11 120 978

On Mon, Oct 6, 2014 at 6:49 PM, Martell Malone [email protected]
wrote:

Hi @GerHobbelt https://github.com/GerHobbelt,

Would you be able to help me with something.

I want to change pthreads.h pthread_t from a struct to a uintptr_t
As gcc assumes a scalar value for c++11 threading

I don't know how to propagate this change throughout pthread without
breaking it :(
and I really need todo this


Reply to this email directly or view it on GitHub
#2.

@martell
Copy link
Author

martell commented Oct 6, 2014

Wow that is a really long response.
Thanks for taking the time.

The struct is actually already in pthread.h my diff just removes that and replaces it with uintptr_t
After some further investigation I can see that the x element is just used for reusing memory of old threads when creating new one. pthreads-win32 seems to hang onto finished threads in a stack and then uses that memoey again. This seems to me optimisation for the sake of optimisation.

gcc or rather libstdc++ requires a variable type so that it can do comparators.
Technically this is a bug in libstdc++ as the spec does not dis allow struct.
gcc devs have acknowledged this but its a minor bug as pthreads-win32 is the only implementation not to use a scalar value

The reason I went with uintptr_t is because that is what the mingw-w64 libpthreads does.

I am not actually working from pthreads-win32 I'm working of a fork called pthreads-embedded
This is based on pthreads-win32 but restructured to support multiple embedded platforms like the psp or the ps3. It does however have this common issue when trying to deal with gcc/libstdc++ for c++ 11.

GerHobbelt pushed a commit that referenced this issue Apr 13, 2021
remove obnoxious compiler warning; this will declare VERY LONG timeou…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants