Opened 10 years ago

Closed 6 months ago

#3883 closed enhancement (fixed)

[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 (18)

comment:1 by bonefish, 10 years ago

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 by mjw, 10 years ago

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)

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

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 by mjw, 10 years ago

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.

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

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 by mjw, 10 years ago

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 by bonefish, 10 years ago

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 by nielx, 9 years ago

Has a Patch: set

comment:9 by stippi, 9 years ago

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

comment:10 by phoudoin, 9 years ago

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 by mmadia, 8 years ago

Has a Patch: unset

in reply to:  5 comment:12 by mmadia, 8 years ago

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 by kallisti5, 5 years ago

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 by waddlesplash, 4 years ago

Milestone: R1Unscheduled

comment:15 by waddlesplash, 4 years ago

Milestone: UnscheduledR1

Reverting earlier milestone change

comment:16 by axeld, 2 years ago

Owner: changed from axeld to nobody
Status: newassigned

comment:17 by waddlesplash, 6 months ago

Resolution: fixed
Status: assignedclosed

Done in hrev52972 and hrev52973.

Note: See TracTickets for help on using tickets.