Opened 10 years ago

Last modified 23 months ago

#3883 assigned enhancement

[patch] please implement pthread_attr_getstack()

Reported by: mjw Owned by: nobody
Priority: normal Milestone: R1
Component: System/libroot.so Version: R1/pre-alpha1
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

I have created a patch to implement pthread_attr_getstack(). It also implements pthread_getattr_np() as a supporting function.

Attachments (1)

pthread_attr_getstack_2.patch (5.6 KB) - added by mjw 10 years ago.

Download all attachments as: .zip

Change History (17)

Changed 10 years ago by mjw

comment:1 Changed 10 years ago by bonefish

pthread_getattr_np() is a non-standard GNU extension. I'm unsure whether we should provide it. Besides I wouldn't implement it by using the cached pthread creation attributes, but rather by collecting the info at that time. Most attributes we don't really support anyway -- so those can get the default values -- the stack address and size can be retrieved via get_thread_info().

comment:2 Changed 10 years ago by mjw

Not quite an absolute "no", then.

Would it be more acceptable if I were to re-work the patch, removing pthread_getattr_np() and not caching the pthread_attr in pthread_create()?

So, the following would be valid: pthread_attr_init(attr) pthread_create(..., &attr, ...) pthread_attr_getstack(&attr)

comment:3 in reply to:  2 Changed 10 years ago by bonefish

Replying to mjw:

Not quite an absolute "no", then.

Would it be more acceptable if I were to re-work the patch, removing pthread_getattr_np() and not caching the pthread_attr in pthread_create()?

Sure. Even better, if you also implemented the other stack-related attribute functions.

So, the following would be valid: pthread_attr_init(attr) pthread_create(..., &attr, ...) pthread_attr_getstack(&attr)

It would be valid, but I'm not sure that it would do what you expect. That is if you expect, that the pthread_attr_getstack() returns the stack address of the newly created thread. pthread_create() doesn't modify the supplied attributes -- it must not do that, since they are "const" and can't do that anyway, since that would prevent the attributes to be used for creating multiple threads. So, unless one explicitly sets the stack address, pthread_attr_getstack() should return NULL or some other placeholder.

Is there a special use case/ported software where the stack address and/or size is needed?

comment:4 Changed 10 years ago by mjw

Yes, you are right. I was working to old docs. It looks as though pthread_attr_getstack() should return either NULL for the address base, or the address you passed into pthread_attr_setstack(). The only way to get a thread's stack base out of it is to call pthread_getattr_np() to get the actual thread's attributes before calling pthread_attr_getstack().

I was porting Kaffe Java VM which uses pthread_attr_getstack() in its unix pthreads code. I believe mono needs to know the thread stack base as well.

With that in mind, get_thread_info() looks like the correct thing to use, so this can be closed as invalid if you like.

comment:5 in reply to:  4 ; Changed 10 years ago by bonefish

Replying to mjw:

Yes, you are right. I was working to old docs. It looks as though pthread_attr_getstack() should return either NULL for the address base, or the address you passed into pthread_attr_setstack(). The only way to get a thread's stack base out of it is to call pthread_getattr_np() to get the actual thread's attributes before calling pthread_attr_getstack().

I was porting Kaffe Java VM which uses pthread_attr_getstack() in its unix pthreads code. I believe mono needs to know the thread stack base as well.

Don't misunderstand me. I'm generally not opposed to adding non-POSIX functionality. It just seems pthread_getattr_np() is really GNU-only and not supported by other platforms. How is this solved for other non-GNU platforms (e.g. the BSDs)? Maybe there's another, more portable feature we could provide.

With that in mind, get_thread_info() looks like the correct thing to use, so this can be closed as invalid if you like.

The summary is valid at any rate. We should implement the missing pthread attribute functions earlier or later. :-)

comment:6 Changed 10 years ago by mjw

I had an investigate and found that the BSDs implement their own function called pthread_attr_get_np().

NetBSD code is here: http://cvsweb.netbsd.org/bsdweb.cgi/~checkout~/src/lib/libpthread/pthread_attr.c?rev=1.12&content-type=text/plain&only_with_tag=MAIN

FreeBSD core is here: http://svn.freebsd.org/viewvc/base/head/lib/libthr/thread/thr_attr.c?revision=178446&view=markup

Man page is here: http://www.gsp.com/cgi-bin/man.cgi?section=3&topic=pthread_attr_get_np

This is almost identical to the GNU version, except that you must call pthread_attr_init() before calling these functions.

Looking at the complexity of the mono code, I don't think there is a nice cross platform way of doing this.

If you do: pthread_attr_init(attr); pthread_create(..., attr, ...); pthread_attr_getstack(attr, ...); then Linux, OpenSolaris and FreeBSD (and, I assume, other BSDs) return default values for stack base and stack size.

So, implementing pthread_attr_getstack() so that it returns the default values is simple. The only question is whether it is worth implementing either pthread_getattr_np() or pthread_attr_get_np(). If so, the argument comes down to whether you prefer BSD or GNU.

comment:7 Changed 10 years ago by bonefish

I'd go with the GNU function for the time being. We can still decide to add the BSD function to our BSD compatibility library later.

comment:8 Changed 9 years ago by nielx

Has a Patch: set

comment:9 Changed 9 years ago by stippi

I've read the comments, but it is unclear whether the patch would be ready as is.

comment:10 Changed 9 years ago by phoudoin

Dunno myself.

Anyway, I may be wrong but I think the current patch's pthread_getattr_np() implementation will leak memory when called with a dst parameter already initialized by pthread_attr_init(), as recommended, like in this example:

size_t
get_thread_stack_size(pthread_t pid)
{
    pthread_attr_t attr;
    size_t size;

    pthread_attr_init(&attr);
    pthread_attr_get_np(pid, &attr);
    pthread_attr_getstacksize(&attr, &size); 
    pthread_attr_destroy(&attr);

    return(size);
}

Otherwise, Ingo raised concern about caching thread stack base address in thread internal attributes when such info can already be retrieved with get_thread_info(), wich will avoid to extent private pthread_attr struct.

comment:11 Changed 7 years ago by mmadia

Has a Patch: unset

comment:12 in reply to:  5 Changed 7 years ago by mmadia

Replying to bonefish:

Replying to mjw:

With that in mind, get_thread_info() looks like the correct thing to use, so this can be closed as invalid if you like.

The summary is valid at any rate. We should implement the missing pthread attribute functions earlier or later. :-)

Setting the patch as obsolete, the above exchange seems to indicate that the patch was not acceptable as-is.

comment:13 Changed 4 years ago by kallisti5

The lack of these pthread functions are breaking Ruby 2.x.x on Haiku.

See https://bugs.ruby-lang.org/issues/10811 for additional info.

comment:14 Changed 4 years ago by waddlesplash

Milestone: R1Unscheduled

comment:15 Changed 4 years ago by waddlesplash

Milestone: UnscheduledR1

Reverting earlier milestone change

comment:16 Changed 23 months ago by axeld

Owner: changed from axeld to nobody
Status: newassigned
Note: See TracTickets for help on using tickets.