Set -Zrandomize-layout?

[RalfJung]

rustc now has a flag to randomize the layout of repr(Rust) types which can help detect code that makes incorrect layout assumptions.

@rust-lang/miri Is that something Miri should enable? Sounds like it, doesn't it?

[RalfJung]

Doing this causes a test failure in liballoc:

  0.005647   ---- collections::btree::node::tests::test_sizes stdout ----
  0.000079   thread 'main' panicked at 'assertion failed: `(left == right)`
  0.000008     left: `200`,
  0.000006    right: `192`', alloc_miri_test/../library/alloc/src/collections/btree/node/tests.rs:99:5
  0.000005   
  0.005277   
  0.000074   failures:
  0.021831       collections::btree::node::tests::test_sizes

[bjorn3]

That is just a type size test to avoid size regressions. Should be fine to disable.

https://github.com/rust-lang/rust/blob/9b735a7132acd58b3bd34c084e9ca5b4ca7450a2/library/alloc/src/collections/btree/node/tests.rs#L99

In any case I don't think it should be enabled for the standard library as it probably breaks layout assumptions in the standard library.

[RalfJung]

The stdlib tests all pass except for that one. So seems fine to enable for the standard library? Might be a good way to discover these layout assumptions, and maybe add #[rustc_layout_assumption] attributes to opt-out certain types from randomization and also document the assumptions.

[saethlin]

Just rehashing the usual discussion about this flag here so we're all on the same page.

• These size assertion tests aren't common, but more crates than rustc have such tests, and it is annoying to break them.
• -Zrandomize-layout doesn't add padding to structs with one field: Added randomized field padding to -Z randomize-layout rust#97861 (If I recall correctly, kixiron needs help pushing this through). This change would probably find a lot more issues than currently.
• -Zrandomize-layout doesn't reorder the components of fat pointers, but there is ambient talk about the fact that we should stabilize that.
• The errors you tend get out of Miri when you accidentally rely on field layout are strange, for example:

error: Undefined Behavior: constructing invalid value at .borrow.borrow: encountered a dangling reference (address 0x221b48 is unallocated)
   --> src/cell.rs:318:45
    |
318 |         let _ignore: Ref<String> = unsafe { mem::transmute((pointer, cell)) };
    |                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value at .borrow.borrow: encountered a dangling reference (address 0x221b48 is unallocated)
    |

(this is a crate trying to rely on the layout of std::cell::Ref)

In aggregate, I am ambivalent-to-no on this. It's not hard to turn on the flag yourself if you want to, and it finds almost no bugs in its current form. The cost and benefit are both rather small, but I think in its current form the flag is more likely to annoy authors of size assertions (because they tend to be written by people who care) than it is to find accidental layout dependencies (because I have debugged a few of those, and they seem to both be rare and mostly written by people who do not care so much about soundness).

[RalfJung]

Based on the above, I have removed the flag from cargo-careful, and it seems like we will not set it in Miri (for now). Thanks for the feedback!