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

Call pthread_attr_init in jl_init_stack_limits #55594

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Aug 27, 2024

#55515 seems to have introduced segfaults on FreeBSD during the atexit and ccall tests. Prior to that PR, we had been calling pthread_attr_init in jl_init_stack_limits on FreeBSD. According to the manual page for pthread_attr_get_np, it is "HIGHLY RECOMMENDED" to use pthread_attr_init to allocate storage. Might as well put it back then, and hopefully it'll fix the segfaults.

@ararslan ararslan added the bugfix This change fixes an existing bug label Aug 27, 2024
@ararslan ararslan requested a review from vtjnash August 27, 2024 04:40
@ararslan
Copy link
Member Author

ararslan commented Aug 27, 2024

Looks like this indeed fixes the atexit segfaults on FreeBSD. The CI failures for the build on musl Linux and tests for Linux PowerPC, AArch64, and x86_64 with assertions are unrelated.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in the FreeBSD only section?

@ararslan
Copy link
Member Author

I don't know, should it? Doesn't seem to be required on Linux though it shouldn't hurt to call it.

@vtjnash
Copy link
Member

vtjnash commented Aug 27, 2024

It may cause a memory leak, since it fails to get destroyed

@ararslan
Copy link
Member Author

Doesn't it get destroyed here?

pthread_attr_destroy(&attr);

@vtjnash
Copy link
Member

vtjnash commented Aug 28, 2024

The second does, but not the first

@ararslan
Copy link
Member Author

I don't understand, I only see one pthread_attr_t being created in this function, line 68:

julia/src/init.c

Lines 50 to 101 in 7d2a7de

void jl_init_stack_limits(int ismaster, void **stack_lo, void **stack_hi)
{
#ifdef _OS_WINDOWS_
(void)ismaster;
// https://en.wikipedia.org/wiki/Win32_Thread_Information_Block
# ifdef _P64
*stack_hi = (void**)__readgsqword(0x08); // Stack Base / Bottom of stack (high address)
*stack_lo = (void**)__readgsqword(0x10); // Stack Limit / Ceiling of stack (low address)
# else // !_P64
*stack_hi = (void**)__readfsdword(0x04); // Stack Base / Bottom of stack (high address)
*stack_lo = (void**)__readfsdword(0x08); // Stack Limit / Ceiling of stack (low address)
# endif // _P64
#else // !_OS_WINDOWS_
// Only use pthread_*_np functions to get stack address for non-master
// threads since it seems to return bogus values for master thread on Linux
// and possibly OSX.
if (!ismaster) {
# if defined(_OS_LINUX_) || defined(_OS_FREEBSD_)
pthread_attr_t attr;
pthread_attr_init(&attr);
#if defined(_OS_FREEBSD_)
pthread_attr_get_np(pthread_self(), &attr);
#else
pthread_getattr_np(pthread_self(), &attr);
#endif
void *stackaddr;
size_t stacksize;
pthread_attr_getstack(&attr, &stackaddr, &stacksize);
pthread_attr_destroy(&attr);
*stack_lo = stackaddr;
*stack_hi = (char*)stackaddr + stacksize;
return;
# elif defined(_OS_DARWIN_)
extern void *pthread_get_stackaddr_np(pthread_t thread);
extern size_t pthread_get_stacksize_np(pthread_t thread);
pthread_t thread = pthread_self();
void *stackaddr = pthread_get_stackaddr_np(thread);
size_t stacksize = pthread_get_stacksize_np(thread);
*stack_lo = (char*)stackaddr - stacksize;
*stack_hi = stackaddr;
return;
# else
# warning "Getting precise stack size for thread is not supported."
# endif
}
struct rlimit rl;
getrlimit(RLIMIT_STACK, &rl);
size_t stacksize = rl.rlim_cur;
*stack_hi = __builtin_frame_address(0);
*stack_lo = (void*)((char*)*stack_hi - stacksize);
#endif
}

PR 55515 seems to have introduced segfaults on FreeBSD during the
`atexit` tests. Prior to that PR, we had been calling
`pthread_attr_init` in `jl_init_stack_limits` on FreeBSD. According to
the manual page for `pthread_attr_get_np`, it is "HIGHLY RECOMMENDED" to
use `pthread_attr_init` to allocate storage. Might as well put it back
then, and hopefully it'll fix the segfaults.
@ararslan ararslan force-pushed the aa/fix-atexit-segfault branch from 7d2a7de to 76e72db Compare August 28, 2024 20:59
@ararslan
Copy link
Member Author

I still don't get why it shouldn't be called on Linux, but at any rate, I've moved the call to be FreeBSD only.

@ararslan ararslan requested a review from vtjnash August 28, 2024 21:01
@ararslan
Copy link
Member Author

Quoth Jameson in Slack, "just merge." Okie dokie

@ararslan ararslan merged commit 4c2f728 into master Aug 30, 2024
7 checks passed
@ararslan ararslan deleted the aa/fix-atexit-segfault branch August 30, 2024 17:29
KristofferC pushed a commit that referenced this pull request Sep 12, 2024
PR 55515 seems to have introduced segfaults on FreeBSD during the `atexit`
and `ccall` tests. Prior to that PR, we had been calling
`pthread_attr_init` in `jl_init_stack_limits` on FreeBSD. According to
the [manual
page](https://man.freebsd.org/cgi/man.cgi?query=pthread_attr_get_np&apropos=0&sektion=3&manpath=FreeBSD+13.2-RELEASE&arch=default&format=html)
for `pthread_attr_get_np`, it is "HIGHLY RECOMMENDED" to use
`pthread_attr_init` to allocate storage. Might as well put it back then,
as it fixes the segfaults.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants