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

Add phtread_key and readdir #132

Merged
merged 19 commits into from
Jul 27, 2022
Merged

Conversation

carbotaniuman
Copy link
Contributor

This also fixes a bug in pthread_mutex_init, wherein a null pointer could've been dereferenced.

@carbotaniuman carbotaniuman changed the title Add phtread_key and readdir Draft: Add phtread_key and readdir Jul 21, 2022
@carbotaniuman carbotaniuman changed the title Draft: Add phtread_key and readdir Add phtread_key and readdir Jul 21, 2022
@carbotaniuman
Copy link
Contributor Author

This does not yet handle destructors in pthread_key, as I'm not sure from where I should be registering the destructor.

c-scape/src/threads/key.rs Outdated Show resolved Hide resolved
unsafe extern "C" fn readdir() {
//libc!(libc::readdir());
unimplemented!("readdir")
unsafe extern "C" fn readdir(dir: *mut libc::DIR) -> *mut libc::dirent {
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting. The presence of undefined references to readdir on GLIBC indicates a code which has not been compiled with -D_FILE_OFFSET_BITS=64 or equivalent. In this case, it looks like the code in question is libgit2, so I've now filed rust-lang/git2-rs#862 to fix this. With that PR, libgit2 will use readdir64 instead of readdir, on GLIBC.

I wonder if it makes sense for mustang to leave readdir undefined, when target_env = "gnu", to help catch programs that haven't enabled _FILE_OFFSET_BITS=64/_TIME_BITS=64 yet.

@sunfishcode
Copy link
Owner

This does not yet handle destructors in pthread_key, as I'm not sure from where I should be registering the destructor.

We can register a function in pthread_setspecific with origin::at_thread_exit, on each thread the first time pthread_setspecific is called on that thread, using something like a #[thread_local] static Cell<bool>.

c-gull/src/use_libc.rs Outdated Show resolved Hide resolved
c-scape/src/threads/key.rs Outdated Show resolved Hide resolved
c-scape/src/threads/key.rs Outdated Show resolved Hide resolved
c-scape/src/threads/key.rs Outdated Show resolved Hide resolved
// all the pthread_cleanup functions,
// then all thread-local dtors,
// and then finally all static
// (`__cxa_thread_atexit_impl`) dtors.
Copy link
Owner

Choose a reason for hiding this comment

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

Do you know if POSIX specifies this ordering?

Also, I realize this is being picky, but could you move this mention about pthread_cleanup functions to the pthread_cleanup_push/_pop function stubs themselves? Technically, we don't need to worry about that ordering until we implement those, and we might not do that for quite a while :-).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first two are specified by POSIX:

Any clean-up handlers established by pthread_cleanup_push(3) that
have not yet been popped, are popped (in the reverse of the order
in which they were pushed) and executed. If the thread has any
thread-specific data, then, after the clean-up handlers have been
executed, the corresponding destructor functions are called, in
an unspecified order.

And the last two are specced by the C++ standard:

Destructors for initialized objects with thread storage duration within a given thread are called as a result of returning from the initial function of that thread and as a result of that thread calling std::exit. The completions of the destructors for all initialized objects with thread storage duration within that thread are sequenced before the initiation of the destructors of any object with static storage duration.

Copy link
Owner

Choose a reason for hiding this comment

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

Looking at this, I noticed that origin wasn't calling the TLS dtors on exit. I've now filed #133 for that.

Other than that, and other than ptthread_cleanup_push, I think we're ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

other than ptthread_cleanup_push

I'm not quite sure what you're referring to here.

Copy link
Owner

Choose a reason for hiding this comment

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

I mean that we haven't implemented the ordering for destructors registered with pthread_cleanup_push yet, but that's ok for now because we haven't implemented pthread_cleanup_push yet.

c-scape/src/threads/key.rs Show resolved Hide resolved
c-scape/src/threads/key.rs Outdated Show resolved Hide resolved
@sunfishcode
Copy link
Owner

Thanks! I may still think about putting readdir under a cargo feature, to help people find codebases that haven't added -D_FILE_OFFSET_BITS=64 and -D_TIME_BITS=64, which glibc needs, but we can figure that out later.

@sunfishcode sunfishcode merged commit e891c1d into sunfishcode:main Jul 27, 2022
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

Successfully merging this pull request may close these issues.

2 participants