panic on latest nightly

[ehuss]

While running on the libgit2-sys crate, I'm getting a panic:

thread 'main' panicked at 'attempted to leave type extern "C" fn(u32, *const i8, *const libgit2_sys::git_diff_file, *const libgit2_sys::git_diff_file, *const libgit2_sys::git_diff_file, *mut core::ffi::c_void) -> i32 uninitialized, which is invalid', /rustc/c20d7eecbc0928b57da8fe30b2ef8528e2bdd5be/src/libcore/mem/mod.rs:536:5

This is caused by https://github.com/rust-lang/rust/pull/66059. I'm guessing ctest will need to be rewritten to use MaybeUninit?

rustc 1.43.0-nightly (c20d7eecb 2020-03-11)

[RalfJung]

Switching from fn types to Option<fn> types would circumvent the panic for now. That type can at least legally be zero-initialized, unlike fn.

But leaving Option<fn> uninitialized still UB, and eventually the panic will catch up with that and be precise enough to test this (the planned timeframe for that extra precision is 6-12 weeks).

Also I am not sure if doing so even makes any sense, given the purpose of this crate.

@ehuss do you have a full backtrace?

[ehuss]

Alex made everything Option<fn> which seems to fix the immediate problem (https://github.com/rust-lang/git2-rs/commit/d70b64599a080ea91f6052cb234a4a485f5dcdff).

@RalfJung It looks like a lot of what ctest uses uninitialized for is computing struct layout. Do you happen if there is a "modern" way to do that? The only thing I'm aware of is eddyb's offset_of macro (internals), but I don't know if there is a newer/better way. Also, it looks like it needs the size and padding of the fields as well, which isn't immediately obvious to me how to compute.

The original code looked like:

#[allow(non_snake_case, unused_mut, unused_variables, deprecated)]
#[inline(never)]
fn roundtrip_padding_git_checkout_notify_cb() -> Vec<u8> {
  // stores (offset, size) for each field
  let mut v = Vec::<(usize, usize)>::new();
  let foo: git_checkout_notify_cb = unsafe { std::mem::uninitialized() };
  let foo = &foo as *const _ as *const git_checkout_notify_cb;

  // This vector contains `1` if the byte is padding
  // and `0` if the byte is not padding.
  let mut pad = Vec::<u8>::new();
  // Initialize all bytes as:
  //  - padding if we have fields, this means that only
  //  the fields will be checked
  //  - no-padding if we have a type alias: if this
  //  causes problems the type alias should be skipped
  pad.resize(mem::size_of::<git_checkout_notify_cb>(), 0);
  for (off, size) in &v {
      for i in 0..*size {
          pad[off + i] = 0;
      }
  }
  pad
}

Backtrace

thread 'main' panicked at 'attempted to leave type `extern "C" fn(u32, *const i8, *const libgit2_sys::git_diff_file, *const libgit2_sys::git_diff_file, *const libgit2_sys::git_diff_file, *mut core::ffi::c_void) -> i32` uninitialized, which is invalid', /rustc/c20d7eecbc0928b57da8fe30b2ef8528e2bdd5be/src/libcore/mem/mod.rs:536:5
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.44/src/backtrace/libunwind.rs:86
   1: backtrace::backtrace::trace_unsynchronized
             at /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.44/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:78
   3: ::fmt
             at src/libstd/sys_common/backtrace.rs:59
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1063
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1426
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:62
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:204
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:224
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:470
  11: rust_begin_unwind
             at src/libstd/panicking.rs:378
  12: core::panicking::panic_fmt
             at src/libcore/panicking.rs:111
  13: core::panicking::panic
             at src/libcore/panicking.rs:54
  14: core::mem::uninitialized
             at /rustc/c20d7eecbc0928b57da8fe30b2ef8528e2bdd5be/src/libcore/mem/mod.rs:536
  15: systest::roundtrip_padding_git_checkout_notify_cb
             at /Users/eric/Proj/rust/git2-rs/target/debug/build/systest-d5ec5154a009511d/out/all.rs:5460
  16: systest::roundtrip_git_checkout_notify_cb
             at /Users/eric/Proj/rust/git2-rs/target/debug/build/systest-d5ec5154a009511d/out/all.rs:5494
  17: systest::run_group0
             at /Users/eric/Proj/rust/git2-rs/target/debug/build/systest-d5ec5154a009511d/out/all.rs:45828
  18: systest::run_all
             at /Users/eric/Proj/rust/git2-rs/target/debug/build/systest-d5ec5154a009511d/out/all.rs:47097
  19: systest::main
             at /Users/eric/Proj/rust/git2-rs/target/debug/build/systest-d5ec5154a009511d/out/all.rs:10
  20: std::rt::lang_start::{{closure}}
             at /rustc/c20d7eecbc0928b57da8fe30b2ef8528e2bdd5be/src/libstd/rt.rs:67
  21: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:52
  22: std::panicking::try::do_call
             at src/libstd/panicking.rs:303
  23: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:86
  24: std::panicking::try
             at src/libstd/panicking.rs:281
  25: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  26: std::rt::lang_start_internal
             at src/libstd/rt.rs:51
  27: std::rt::lang_start
             at /rustc/c20d7eecbc0928b57da8fe30b2ef8528e2bdd5be/src/libstd/rt.rs:67
  28: ::pretty

[RalfJung]

@ehuss memoffset can compute offset and size of fields (and thus infer padding based on the gaps between them). Does that help?

Note that there is currently no sound way to do that, but memoffset uses the "least unsound" way. Also we will make sure to update it once a sound way exists. That's why it is better to depend on memoffset than copy its current implementation.