Consider using Miri

[saethlin]

This crate executes multiple kinds of UB. I ran into it doing a crater run for this PR: rust-lang/rust#121282 which improves the optimization of transmutes from integer to pointer types.

I haven't been able to diagnose exactly why that PR breaks this crate, because in running Miri on this crate I run into other problems that I'm not sure how to fix.

To get started with Miri, you'll need a nightly toolchain because Miri isn't available on stable yet.

rustup toolchain add nightly
rustup component add --toolchain=nightly miri

Then you can run cargo +nightly miri test, for this repo I suggest you start by adding the environment variable to disable aliasing analysis, because that will let you skip some of the more complicated forms of UB. For example, MIRIFLAGS=-Zmiri-disable-stacked-borrows cargo +nightly miri test. Miri has decent documentation in its README: https://github.com/rust-lang/miri

[Liorst4]

Oh wow, I didn't know the Rust's team bot runs my package :)

Looking at the creater run
rust-lang/rust#121282 (comment)
https://crater-reports.s3.amazonaws.com/pr-121282/index.html
https://crater-reports.s3.amazonaws.com/pr-121282/try%23d073071d77ce0f93b4fd8cc567a1e2b9e1b22126%2Brustflags=-Copt-level=3/gh/Liorst4.liorforth/log.txt

[INFO] [stdout] ---- tests::tests::test_counted_string stdout ----
[INFO] [stdout] thread 'tests::tests::test_counted_string' panicked at src/tests.rs:457:22:
[INFO] [stdout] called `Option::unwrap()` on a `None` value

I can see that the failure comes from here:

liorforth/src/tests.rs

Lines 454 to 458 in 390a615

	let counted_string: &CountedString = unsafe {
	std::mem::transmute::<Cell, *const CountedString>(counted_string_address)
	.as_ref()
	.unwrap()
	};

I mostly stopped using transmute to cast between isize and pointers a while ago.
This transmute cast is probably one of the last in the code base.

CountedString is just a string with a counter at the beginning instead of a null terminator.

liorforth/src/main.rs

Lines 179 to 183 in 390a615

	#[repr(packed(1))]
	struct CountedString {
	len: Byte,
	data: [Byte; 0],
	}

Cell is just isize (signed), its the native type of Forth, and used to hold pointers.

liorforth/src/main.rs

Line 18 in 390a615

type Cell = isize;

Try this simple code to see if your compiler issue reproduces.

type Byte = u8;

type Cell = isize;

#[repr(packed(1))]
struct CountedString {
    len: Byte,
    data: [Byte; 0],
}

fn test() {
    let buffer: [u8; 6] = [5, b'h', b'e', b'l', b'l', b'o'];
    let counted_string_address: Cell = (&buffer).as_ptr() as Cell;
    let counted_string: &CountedString = unsafe {
        std::mem::transmute::<Cell, *const CountedString>(counted_string_address)
            .as_ref()
            .unwrap()
    };

    let counted_string_as_slice: &[u8] = unsafe {
        std::slice::from_raw_parts(counted_string.data.as_ptr(), counted_string.len as usize)
    };
    let counted_string_as_rust_string = std::str::from_utf8(counted_string_as_slice).unwrap();
    println!("{}", counted_string_as_rust_string);
}

This crate executes multiple kinds of UB.

This crate is an implementation for a programming language that isn't safe :P

In running Miri on this crate I run into other problems that I'm not sure how to fix.

I wasn't familiar with miri until now, looks interesting, I'll check it when I'll have some free time.
What kind of errors did Miri had?

[saethlin]

Try this simple code to see if your compiler issue reproduces.

When compiled with my patch that code crashes with a SIGILL at runtime.

This crate is an implementation for a programming language that isn't safe :P

Well yes, but presumably you want your interpreter to not crash :P

What kind of errors did Miri had?

By default, Miri has a more picky aliasing model (we're not sure what the aliasing model is for Rust yet, so the default is the one that's more paranoid). So Miri immediately complains about the &Header pattern (taking a reference to only part of an allocation then trying to widen it back out later), which Stacked Borrows forbids, but Tree Borrows is designed to allow. So MIRIFLAGS=-Zmiri-tree-borrows cargo +nightly miri test is a good start.

Miri immediately complains about an alignment issue; it looks like you're trying to treat an allocation which was allocated as a [u8; N] as a &[u64]. But the array can be (and often is, in Miri) allocated so that its base address is not sufficiently aligned to be treated as an array of u64. You probably just want to use an array of u64 there instead. But fixing that didn't look trivial to me, so I stopped trying to probe deeper.

All UB is fatal to Miri, because in almost all cases successfully executing UB in the interpreter would corrupts its internal data structures. Engineering an interpreter with the depths of checks that Miri has that can keep going past invalid execution is a bit beyond us at the moment.