Open HeroicKatora opened 4 years ago
The comment above this line is inaccurate as to what it is doing, and the usage of transmute here to create a reference with longer lifetime is not sound.
The transmute itself does not cause a leak of the value. It is purely a pointer cast, and has no semantic effect on the machine state. What this is doing is the opposite, it is asserting to the compiler that your code has done something to the effect of leaking the value, but that the compiler just can't see. That, however, is wrong.
I see I didn't properly understood what it does, it's the first time I've experimented with unsafe code. I'm wondering why it's wrong though?
Also note that creating a statically shared reference to the value is not what you want as that would disallow any mutable reference being created for the whole remaining program lifetime. Creating any mutable reference is UB as long as a shared reference exists, thus it would be allowed for the compiler to optimize out any of the writes done by the web server. More likely though, it will instead deduplicate some reads of the referred-to value which could cause writes to appear to have no effect.
I guess I see the problem with this, but then I'm wondering why it's working in its current state? And what would you suggest would be a better way to implement this?
Thanks for the issue/introspection!
I'm wondering why it's wrong though?
If you use unsafe
then you should wonder and explain why it is right, since this is effectively a promise to the compiler. If you can't uphold that promise by fact then the use of unsafe
is likely incorrect.
I guess I see the problem with this, but then I'm wondering why it's working in its current state? And what would you suggest would be a better way to implement this?
The compiler does not have to any particular thing once UB occurs, it's allowed to do 'what you expect' or other diverging behaviour. That's why it is called undefined behaviour. It may at some point in the future give very hard to debug problems. In this case, I do have some sample program to demonstrate strange effects right away:
// Create a slider to tweak 'VALUE' in the web GUI
#[const_tweaker::tweak]
const VALUE: f64 = 0.0;
fn main() {
// Initialize the web GUI at 'http://127.0.0.1:9938'
const_tweaker::run().expect("Could not run server");
let value: &'static _ = &*VALUE;
// Enter a GUI/Game loop
loop {
assert_eq!(*value, 0.0);
}
}
If run in release mode, this assert never triggers on my machine even when changing the value through the web interface. That's because the static reference given by this library asserts to the compiler that the value will never change. The compiler, wanting to avoid wrok, may then only check the condition in the first loop iteration and subsequently the program may simply enter an unchecked infinite loop.
As said, the compiler will allow but not obligated to perform this kind of inlining. This will come back to bite easily when in any larger production different choices are made for different parts of the program, which the compiler is perfectly allowed to do because your promises do not hold. Then suddenly there are diverging view of the single constant that may cause all sorts of following misbehaviour.
I think I understand it now, but I still don't know how to fix that.
The problem I tried to solve by using the unsafe transmute is that the std::ops::Deref
on the generated struct always has to return a reference. The result of that is that I need to get the lifetime of the value from the map, which I can't because I can't control where the dereference happens and with which lifetime.
I tried to use the transmute here because I know that the value of the maps will ever be removed and I assumed that signifying that with a 'static
lifetime was enough. I didn't know the compiler was smart enough to only check on the first loop iteration.
I had quite the same issue with assets_manager
: I wanted users to keep references to assets while having hot-reloading running in the background.
I came up with a solution involving a RwLock
and interior mutability. However, due to lifetime requirements, users have to lock it manually whenever they access it. This may not be what you want, though.
Given that you only support "basic" types, maybe atomic types can help you. It won't work for &str
easily (maybe something can be done with Arc
or AtomicPtr
and some unsafe code), but it can be a solution for all others.
This is not great and is likely to be an actual soundness bug that affects real-world code. The DashMap
will reallocate as it grows, leaving a dangling pointer in place after this transmutation.
My advice: Put the Field
inside a Box
, then Box::leak
it to safely have the Field
become 'static
. Alternatively, you could use Arc
and then Arc::make_mut
when you want to mutate the constant via the web GUI.
The comment above this line is inaccurate as to what it is doing, and the usage of transmute here to create a reference with longer lifetime is not sound.
https://github.com/tversteeg/const-tweaker/blob/fd94d62f52e99756f006c303ed385a3dd545a53c/macro/src/lib.rs#L118-L120
The transmute itself does not cause a leak of the value. It is purely a pointer cast, and has no semantic effect on the machine state. What this is doing is the opposite, it is asserting to the compiler that your code has done something to the effect of leaking the value, but that the compiler just can't see. That, however, is wrong.
Also note that creating a statically shared reference to the value is not what you want as that would disallow any mutable reference being created for the whole remaining program lifetime. Creating any mutable reference is UB as long as a shared reference exists, thus it would be allowed for the compiler to optimize out any of the writes done by the web server. More likely though, it will instead deduplicate some reads of the referred-to value which could cause writes to appear to have no effect.