Fix UB in transmute

[gmorenz]

The current code mutates through a & reference, this fixed that.

We could remove get_global_ptr and transmute entirely by using #![feature(drop_types_in_const)], I'm not sure if this would be preferred.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[brson]

This is really tricky code and seems to have more problems than just transmuting &T to *mut T. @eddyb has opinions.

[eddyb]

@gmorenz So this change by itself doesn't fix much, borrowing static mut is by itself problematic.
I was worried about #![feature(drop_types_in_const)] not working at stage0, but that's not true.

I think we can defer the problem of replacing static mut with static and an UnsafeCell wrapper (eventually this should be Mutex<Vec<Vec<u8>>> anyway, @Amanieu's parking_lot would make that possible), if all accesses are direct.
Still needs an Option for now because Vec::new is not const fn but I would remove the Box wrapping anyway, it's only there so usize can be used.

[eddyb]

In other words, yes, #![feature(drop_types_in_const)] instead of transmute is preferred.

[gmorenz]

@eddyb I've updated this to use drop_types_in_const and not mutably borrow the static mut. I'm somewhat curious about your comment that "borrowing static mut is by itself problematic.`, is it just something that we prefer to avoid because it's easy to break aliasing rules, or is it always undefined behaviour or something?

[eddyb]

@bors r+ Thank you!

@gmorenz It's arguably not always UB, but it's really easy to misuse, and some strict memory models might make a lot of possible uses of static mut UB and MIR optimizations could then act on that.

To build a proper wrapper around it, you want to have an UnsafeCell(-based abstraction) inside the static mut and to never borrow it mutably directly - but then you can do the same thing with a static, which makes the mut part an unnecessary footgun.
The caveat is that to create an UnsafeCell in a static, you need the ability to call a const fn from its initializer, which is not something that has been stabilized yet, so we can't even deprecate static mut.

[bors]

📌 Commit 80d2956 has been approved by eddyb

[bors]

⌛ Testing commit 80d2956 with merge 49829f3...

[bors]

💔 Test failed - auto-linux-64-opt-mir

[eddyb]

@alexcrichton Are there known valgrind issues around such globals? Should I hunt for codegen bugs?

[alexcrichton]

Hm not that I know of, that does look suspicious though...

[eddyb]

Sorry for letting this sit for this long, let's try again (just in case it was transient or a fixed MIR regression).

@bors retry

[bors]

⌛ Testing commit 80d2956 with merge b15e775...

[bors]

💔 Test failed - auto-linux-64-opt-mir

[mrhota]

@gmorenz@eddyb anything else to do here?

[eddyb]

I honestly have no idea how this goes wrong.

[gmorenz]

I also have no idea what the problem is, I think (though I'm not an expert) that the current set of optimizations doesn't take advantage of anything that might break the current code so merging this PR isn't high priority (from my point of view at least).

[bors]

☔ The latest upstream changes (presumably #36807) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

Closing due to inactivity, but feel free to resubmit with a rebase!