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

Consolidate local_data implementations, and cleanup #8447

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Member

This moves all local_data stuff into the local_data module and only that
module alone. It also removes a fair amount of "super-unsafe" code in favor of
just vanilla code generated by the compiler at the same time.

Closes #8113

@pcwalton
Copy link
Contributor

r? @brson

This looks fine to me but I ought to kick it over to you to make sure.

// but somehow this works. I have no idea what's going
// on but this seems to make things magically work. FML.
self.storage = LocalStorage(ptr::null(), None);
self.storage.take();
Copy link
Member Author

Choose a reason for hiding this comment

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

@brson, you'd probably be interested in this particular modification

Copy link
Contributor

Choose a reason for hiding this comment

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

afaics this doesn't do anything to address the deleted comment - it's the same workaround stated in a different way. Is that right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add back the comment?

The problem is that TLS is deinitiialized, then the box annihilator runs dtors that access TLS, presumably reinitializing TLS after it's already been destroyed. This line should not be necessary at all, and the way it works isn't clear (Who destructs TLS after the annihilator re-constructs it?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I talked a bit about this on IRC, but I agree that this should be left in.

For a "solution", would you be opposed to forbidding TLS accesses once it's being destroyed? Instead of being Option it could be something like Uninitialized | Initialized(obj) | Destroying. I'm not really sure how a failure could be done, I imagine that the normal fail! macro would cause problems.

This cyclic dependency between annihilation and TLS is tricky...

@catamorphism
Copy link
Contributor

@alexcrichton Needs a rebase.

// TLS, or possibly some destructors for those objects being
// annihilated invoke TLS. Sadly these two operations seemed to
// be intertwined, and miraculously work for now...
self.storage.take();
Copy link
Member Author

Choose a reason for hiding this comment

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

r? @brson, I added some of my own comments here, the existing comment didn't quite make sense to me without some extra explanation.

This moves all local_data stuff into the `local_data` module and only that
module alone. It also removes a fair amount of "super-unsafe" code in favor of
just vanilla code generated by the compiler at the same time.

Closes rust-lang#8113
bors added a commit that referenced this pull request Aug 28, 2013
This moves all local_data stuff into the `local_data` module and only that
module alone. It also removes a fair amount of "super-unsafe" code in favor of
just vanilla code generated by the compiler at the same time.

Closes #8113
@bors bors closed this Aug 28, 2013
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.

Move local_data_priv somewhere else
5 participants