search

Meziu / rust-horizon

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
3 stars 1 forks source link

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

Closed ian-h-chamberlain closed 2 years ago

ian-h-chamberlain commented 2 years ago

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

Meziu commented 2 years ago

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 commented 2 years ago

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

Meziu commented 2 years ago

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

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

AzureMarker commented 2 years ago

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 commented 2 years ago

Adding an note here that the segfault might not occur if thread destructors aren't called in pthread: https://github.com/Meziu/pthread-3ds/pull/13#discussion_r810560101

ian-h-chamberlain commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

ian-h-chamberlain commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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