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

Enable #[thread_local] on armv6k-nintendo-3ds #16

Merged
merged 1 commit into from
Mar 26, 2022
Merged

Enable #[thread_local] on armv6k-nintendo-3ds #16

merged 1 commit into from
Mar 26, 2022

Conversation

ian-h-chamberlain
Copy link

Closes #15

This does not work currently, there is a segfault happening in the thread local destructor here. It seems the ptr there is incorrect somehow, causing a page fault.

Still investigating but thought I would open the PR in the meantime.

@AzureMarker @Meziu

@ian-h-chamberlain ian-h-chamberlain changed the title Support std threads again by using pthread Enable #[thread_local] on armv6k-nintendo-3ds Feb 18, 2022
@Meziu
Copy link
Owner

Meziu commented Feb 19, 2022

Change the target branch from horizon-std to armv6k-3ds-target so I can make a separate PR to Rust’s tree.

@ian-h-chamberlain ian-h-chamberlain changed the base branch from horizon-std to armv6k-3ds-target February 19, 2022 15:14
@ian-h-chamberlain
Copy link
Author

ian-h-chamberlain commented Feb 19, 2022

@Meziu done. We probably shouldn't submit a PR just yet while it's still segfaulting in std code, though...

@Meziu
Copy link
Owner

Meziu commented Feb 19, 2022

@Meziu done. We probably shouldn't submit a PR just yet while it's still segfaulting in std code, though...

Yeah obviously 😆. I just use this branch for target-related commits since we can push those whenever we need to.

@AzureMarker
Copy link

AzureMarker commented Feb 20, 2022

Change the target branch from horizon-std to armv6k-3ds-target so I can make a separate PR to Rust’s tree.

We should probably rebase onto the latest master branch commit just to make sure we don't run into merge conflicts when we open the PR. And we can use a new branch since those commits in armv6k-3ds-target already merged.

@AzureMarker
Copy link

Adding an note here that the segfault might not occur if thread destructors aren't called in pthread:
rust3ds/pthread-3ds#13 (comment)

@ian-h-chamberlain
Copy link
Author

Need to test again with the latest set of thread changes – this might be a non-issue now and we could try to upstream, but I have been working on other things and haven't had a chance.

@Meziu
Copy link
Owner

Meziu commented Mar 18, 2022

Tested this with the thread changes. It works perfectly, though I suggest putting the #[cfg(target_os = "horizon")] close to the vxworks implementation rather than the linux one.

I'm pretty sure this check is completely useless, as it will always fail: https://github.com/Meziu/rust-horizon/blob/033ad73d19940db38606c3ea5950de2a82064d69/library/std/src/sys/unix/thread_local_dtor.rs#L31

@Meziu
Copy link
Owner

Meziu commented Mar 18, 2022

This PR could either be added to the thread changes branch, or even be committed directly (once fixed) to Rust's master.

@ian-h-chamberlain ian-h-chamberlain marked this pull request as ready for review March 24, 2022 01:02
@ian-h-chamberlain
Copy link
Author

This PR could either be added to the thread changes branch, or even be committed directly (once fixed) to Rust's master.

Hmm, good point, I think there was a build failure in the thread branch without the cfg change, but the target specification itself could probably go direct to rust-lang/rust. Do we have any other queued up changes for the target spec that would need to go in? Otherwise I think just sticking it in the thread branch might make more sense.

@Meziu
Copy link
Owner

Meziu commented Mar 24, 2022

This PR could either be added to the thread changes branch, or even be committed directly (once fixed) to Rust's master.

Hmm, good point, I think there was a build failure in the thread branch without the cfg change, but the target specification itself could probably go direct to rust-lang/rust. Do we have any other queued up changes for the target spec that would need to go in? Otherwise I think just sticking it in the thread branch might make more sense.

Looking at the rustc_target::spec documentation, I don’t see any other flags relating to our target, so we are probably done with this after has_thread_local. Maybe push here the target spec part, and in https://github.com/AzureMarker/rust-horizon/tree/feature/horizon-threads the cfg in std.

@AzureMarker
Copy link

This PR will be closed once those two new PRs are made?

@ian-h-chamberlain
Copy link
Author

This PR will be closed once those two new PRs are made?

Opened a PR against https://github.com/AzureMarker/rust-horizon/tree/feature/horizon-threads

@Meziu do you want to merge it here and PR upstream, or should I close this and just open one directly to upstream?

@Meziu
Copy link
Owner

Meziu commented Mar 26, 2022

This PR will be closed once those two new PRs are made?

Opened a PR against https://github.com/AzureMarker/rust-horizon/tree/feature/horizon-threads

@Meziu do you want to merge it here and PR upstream, or should I close this and just open one directly to upstream?

Just remove the changes in thread_local_dtor.rs from this PR, since it’s in @AzureMarker’s repository, and I’ll PR it upstream.

@ian-h-chamberlain
Copy link
Author

ian-h-chamberlain commented Mar 26, 2022

Oops, thought I had when I removed the other commit, but I didn't realize there's still one more change. Done

@Meziu Meziu merged commit 419b630 into Meziu:armv6k-3ds-target Mar 26, 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.

Enable #[cfg(target_thread_local)]
3 participants