Skip to content

More typesafe task-local data + LLVM shouldn't fold constants #3273

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

Closed
bblum opened this issue Aug 24, 2012 · 7 comments
Closed

More typesafe task-local data + LLVM shouldn't fold constants #3273

bblum opened this issue Aug 24, 2012 · 7 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@bblum
Copy link
Contributor

bblum commented Aug 24, 2012

Currently, task-local data (TLS) is implemented by keying off the code pointer of a closure that takes the desired type as its argument. You use it like this:

fn my_tls_key(+_x: @my_type) { }

local_data_set(my_key, value);
assert local_data_get(my_key).get() == my_value; // can be in a different function or module

This has three known flaws:

  • As per Move std::test into core or change the way coretest works #2912, TLS may randomly fail to retrieve values if used in coretest test cases if the other access is done in core itself.
  • On windows, TLS does not work cross-crate because of the way linking is done.
  • It has to be marked 'unsafe' because you could instantiate a polymorphic function at multiple types for easy type coercion.

An alternative proposal would be to key off the address of a global constant. A couple issues to address:

  • Shouldn't have to provide an inhabitant of the type to generate the key (maybe inhabitants can only be generated at runtime). Hence, use an empty vector: const my_key: &[my_type] = [];
  • Shouldn't allow an attacker to collide addresses by having differently-typed vectors on multiple stack frames. This would be solved by requiring the pointer to have the static region. type local_data_key<T> = &static/[T];
  • Finally, we should make sure that LLVM does not fold constants that are identical into each other. If you write const key1: &[my_type] = [], key2: &[my_type] = []; in order to have two different TLS slots to store the same type, then they shouldn't "accidentally" end up with the same data address and overwrite each other.

This last, 3rd point is really the major thing blocking this bug being easy. May require a special attribute to tell llvm not to put it in rodata.

I wrote more about this here: http://bubblingbeebles.livejournal.com/111016.html

@brson
Copy link
Contributor

brson commented May 15, 2013

My preferred approach is to add 16 bytes of the type hash to the type descriptor and use the type as the key. Or don't add it to the type descriptor and make it an intrinsic to save space.

@brson
Copy link
Contributor

brson commented May 15, 2013

On second thought, using the address of a static value also seems pretty nice since it doesn't require adding any new runtime metadata. Is there an alternative type we can use to the vector, and is it worth adding another obscure attribute to keep in from being optimized?

static key: TypeKey<MyType> = TypeKey();

@brson
Copy link
Contributor

brson commented May 15, 2013

You would probably want to make TypeKey noncopyable.

@bblum
Copy link
Contributor Author

bblum commented May 15, 2013

That would work as well. I think the problem back when I wrote this was that we didn't have static enum/newtype initializers.

Why does noncopyable matter?

bors added a commit that referenced this issue Jul 12, 2013
cc #6004 and #3273

This is a rewrite of TLS to get towards not requiring `@` when using task local storage. Most of the rewrite is straightforward, although there are two caveats:

1. Changing `local_set` to not require `@` is blocked on #7673
2. The code in `local_pop` is some of the most unsafe code I've written. A second set of eyes should definitely scrutinize it...

The public-facing interface currently hasn't changed, although it will have to change because `local_data::get` cannot return `Option<T>`, nor can it return `Option<&T>` (the lifetime isn't known). This will have to be changed to be given a closure which yield `&T` (or as an Option). I didn't do this part of the api rewrite in this pull request as I figured that it could wait until when `@` is fully removed.

This also doesn't deal with the issue of using something other than functions as keys, but I'm looking into using static slices (as mentioned in the issues).
@emberian
Copy link
Member

Fixed by #7677

@emberian emberian reopened this Jul 12, 2013
@emberian
Copy link
Member

No it wasn't. Misunderstood the github UI... I think.

@alexcrichton
Copy link
Member

Yeah this wasn't fixed just yet, but this is coming very soon...

RalfJung pushed a commit to RalfJung/rust that referenced this issue Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

No branches or pull requests

4 participants