Wrong optimization of Some(Rc<RefCell<...>>) in thread_local! (STATUS_ILLEGAL_INSTRUCTION)

[mikevoronov]

It seems that there is a some sort of wrong optimization of Some(Rc<RefCell<...>>) by using thread_local!. Code like this one https://github.com/michaelvoronov/rust_poc_1 is failed on

rust/src/libstd/thread/local.rs

Line 277 in c84e797

None => hint::unreachable_unchecked(),

when compiled:

error: process didn't exit successfully: "target\debug\rust_poc_1" (exit code: 0xc000001d, STATUS_ILLEGAL_INSTRUCTION)

I dig a little bit around and find out that decompiled code of init for this code looks like that: https://gist.github.com/michaelvoronov/b4aa66d36fe44c99df05a9249a6c708c. But if we delete Rc from ArrayDeque<[Rc<RefCell<SomeStruct>>; 1024], Wrapping>, we obtain decompiled listing like this one: https://gist.github.com/michaelvoronov/a5f2ca75c1c7cdc7547298d12e5c1edf (it isn't failed).

It seems that the key difference is *(_QWORD *)some_value = 1i64; that represents Some of Option enum.

P.S. Tested on 1.34.0-nightly toolchain.

[jonas-schievink]

Reproduces on Linux (both on stable 1.32.0 and 1.34.0-nightly (146aa60 2019-02-18)).

Would be nice to get rid of the arraydeque dependency.

[Mark-Simulacrum]

Cc @rust-lang/compiler

[michaelwoerister]

Nominating for priority assignment.

[pnkfelix]

triage: P-high. Leaving nomination label. Seems like candidate for both 1. narrowing test case and 2. attempting bisection (assuming one can backtrack to an earlier stable rustc that didn't have the fault, e.g. maybe prior to last LLVM upgrade).

[pnkfelix]

assigning to self for initial investigation and removing nomination label.

[pnkfelix]

I spent some time narrowing this down to a smaller test case.

I did the narrowing by first cut-and-pasting the arraydeque code as a submodule of the crate, and then trimming as much as I could out of that arraydeque submodule.

But now that I have something plausible, I'm worried that I may have narrowed it too much. (That, or exposed some fundamental unsoundness in the original code.)

For reference, the code I now have is this (play):

use std::cell::RefCell;
use std::mem;
use std::mem::ManuallyDrop;
use std::rc::Rc;

struct SomeStruct { field: i32 }

struct ArrayDeque<A> { xs: ManuallyDrop<A> }

struct BigStruct { field: ArrayDeque<[Rc<SomeStruct>; 1024]> }

impl BigStruct {
    pub fn new() -> Self {
        unsafe {
            BigStruct {
                field: ArrayDeque {
                    xs: ManuallyDrop::new(mem::uninitialized()),
                }
            }
        }
    }

    pub fn foo(&self) -> i32 { 1 }
}

thread_local! {
    static GM: RefCell<BigStruct> = RefCell::new(BigStruct::new())
}

fn main() {
    GM.with(|gm| gm.borrow().foo() );
    println!("finish...");
}

[pnkfelix]

Hmm. Based on stepping through the execution of local::LocalKey::with (→ try_with → init), I wonder if somehow the use of mem::uninitialized is creating a value that looks like the tag that is used to represent Option::None in the thread local's instance of GM, and thus its tickling this code here:

rust/src/libstd/thread/local.rs

Lines 264 to 274 in f22dca0

	mem::replace(&mut *ptr, Some(value));
	// After storing `Some` we want to get a reference to the contents of
	// what we just stored. While we could use `unwrap` here and it should
	// always work it empirically doesn't seem to always get optimized away,
	// which means that using something like `try_with` can pull in
	// panicking code and cause a large size bloat.
	match *ptr {
	Some(ref x) => x,
	None => hint::unreachable_unchecked(),
	}

[oli-obk]

Here's a smaller, self-contained example not using thread locals: https://play.rust-lang.org/?version=nightly&mode=release&edition=2018&gist=09537cc61a99276e4bace501fd9eb59f. I can't test it against miri, because miri immediately bails out on the mem::uninitialized()

[pnkfelix]

Yeah at this point I'm assuming that unintialized is filling in the Rc::ptr field (which is NonNull) with a null, and that then is treated as an Option::None in the thread/local.rs code I quoted earlier.

My question now is: Which component in the original composition of crates is at fault here?

[oli-obk]

When changing it to support miri (by using a union instead of mem::uninitialized), it works again: https://play.rust-lang.org/?version=nightly&mode=release&edition=2018&gist=ccc12441024623d4895720b4a11f5c9b

My question now is: Which component in the original composition of crates is at fault here?

My immediate reaction (as conditioned by @RalfJung) is to say mem::uninitialized is at fault, and miri agrees (I'm aware this is a kind of circular reasoning).

[nagisa]

ManuallyDrop is not MaybeUninit and therefore must be initialized with valid data. See #58780.

[pnkfelix]

@oli-obk has convinced me that this is not a problem in our std library; it arises because the arraydeque is misusing mem::uninitialized.

(I have filed a bug against arraydeque here: andylokandy/arraydeque#12)