ICE in unaligned references MIR check

[Noratrieb]

https://miri.saethlin.dev/logs/hdf5-derive/0.8.1.html

Meta

rustc 1.69.0-nightly (e7acd078f 2023-02-09) running on x86_64-unknown-linux-gnu

Error output

thread 'rustc' panicked at 'internal error: entered unreachable code', compiler/rustc_mir_transform/src/check_packed_ref.rs:50:21

Backtrace

thread 'rustc' panicked at 'internal error: entered unreachable code', compiler/rustc_mir_transform/src/check_packed_ref.rs:50:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.69.0-nightly (e7acd078f 2023-02-09) running on x86_64-unknown-linux-gnu

note: compiler flags: -C embed-bitcode=no -C debuginfo=2 -C linker=clang -Z randomize-layout -C opt-level=0 -C debuginfo=0 -Z miri-disable-isolation -Z miri-ignore-leaks -Z miri-panic-on-unsupported

note: some of the compiler flags provided by cargo are hidden

query stack during panic:
#0 [mir_const] preparing `_IMPL_H5TYPE_FOR_P1::<impl at tests/test.rs:36:10: 36:16>::type_descriptor` for borrow checking
#1 [mir_promoted] processing MIR for `_IMPL_H5TYPE_FOR_P1::<impl at tests/test.rs:36:10: 36:16>::type_descriptor`
end of query stack
thread 'rustc' panicked at 'internal error: entered unreachable code', compiler/rustc_mir_transform/src/check_packed_ref.rs:50:21

error: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.69.0-nightly (e7acd078f 2023-02-09) running on x86_64-unknown-linux-gnu

note: compiler flags: -C embed-bitcode=no -C debuginfo=2 -C linker=clang -Z randomize-layout -C opt-level=0 -C debuginfo=0 -Z miri-disable-isolation -Z miri-ignore-leaks -Z miri-panic-on-unsupported

note: some of the compiler flags provided by cargo are hidden

query stack during panic:
#0 [mir_const] preparing `_IMPL_H5TYPE_FOR_P2::<impl at tests/test.rs:43:10: 43:16>::type_descriptor` for borrow checking
#1 [mir_promoted] processing MIR for `_IMPL_H5TYPE_FOR_P2::<impl at tests/test.rs:43:10: 43:16>::type_descriptor`
end of query stack

[Noratrieb]

cc @RalfJung

[RalfJung]

ICE originates from:

rust/compiler/rustc_mir_transform/src/check_packed_ref.rs

Lines 44 to 51 in 96834f0

	if let Some(impl_def_id) = self.tcx.impl_of_method(def_id)
	&& self.tcx.is_builtin_derive(impl_def_id)
	{
	// If we ever reach here it means that the generated derive
	// code is somehow doing an unaligned reference, which it
	// shouldn't do.
	unreachable!();
	} else {

@nnethercote this is related to derives on packed structs somehow, it seems.

An MCVE would be quite useful.

[RalfJung]

I think this is the code that causes the ICE, but clearly the H5Type derive macro is relevant here.

Is it possible that is_builtin_derive incorrectly returns true on the code in the non-builtin H5Type derive?

Probably needs a hacked compiler that prints some more info in the affected branch.

[clubby789]

// due to compiler wrongfully complaining re: Copy impl missing for packed struct
#![allow(unaligned_references)]
#[macro_use]
extern crate hdf5_derive;
#[derive(H5Type, Copy, Clone)]
#[repr(packed)]
struct P2(i8, u32);

reproduces the issue, and removing the H5Type derive doesn't ICE.

Expanded

#![feature(prelude_import)]
#![allow(unaligned_references)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
#[macro_use]
extern crate hdf5_derive;
#[repr(packed)]
struct P2(i8, u32);
#[allow(dead_code, unused_variables, unused_attributes)]
const _IMPL_H5TYPE_FOR_P2: () = {
    extern crate hdf5 as _h5;
    #[automatically_derived]
    unsafe impl _h5::types::H5Type for P2 {
        #[inline]
        fn type_descriptor() -> _h5::types::TypeDescriptor {
            let origin: *const P2 = ::std::ptr::null();
            let mut fields = <[_]>::into_vec(
                #[rustc_box]
                ::alloc::boxed::Box::new([
                    _h5::types::CompoundField {
                        name: "0".to_owned(),
                        ty: <i8 as _h5::types::H5Type>::type_descriptor(),
                        offset: unsafe { &((*origin).0) as *const _ as _ },
                        index: 0,
                    },
                    _h5::types::CompoundField {
                        name: "1".to_owned(),
                        ty: <u32 as _h5::types::H5Type>::type_descriptor(),
                        offset: unsafe { &((*origin).1) as *const _ as _ },
                        index: 0,
                    },
                ]),
            );
            for i in 0..fields.len() {
                fields[i].index = i;
            }
            let size = ::std::mem::size_of::<P2>();
            _h5::types::TypeDescriptor::Compound(_h5::types::CompoundType {
                fields,
                size,
            })
        }
    }
};
#[automatically_derived]
impl ::core::marker::Copy for P2 {}
#[automatically_derived]
impl ::core::clone::Clone for P2 {
    #[inline]
    fn clone(&self) -> P2 {
        let _: ::core::clone::AssertParamIsClone<i8>;
        let _: ::core::clone::AssertParamIsClone<u32>;
        *self
    }
}

[RalfJung]

That expanded code is definitely UB.

let origin: *const P2 = ::std::ptr::null();
// ...
unsafe { &((*origin).0) as *const _ as _ }

Here a NULL ptr is being dereferenced. This looks like an incorrect offset_of implementation. offset_of is tricky to implement, that's why we have a crate for it.

But still we shouldn't ICE of course. That code above would trigger the "created reference to unaligned field" error. But that doesn't explain why it enters the is_builtin_derive codepath...

Looking at the docs and code for is_builtin_derive, is it possible that "builtin" just doesnt mean anything here?

    /// If the given `DefId` belongs to a trait that was automatically derived, returns `true`.
    pub fn is_builtin_derive(self, def_id: DefId) -> bool {
        self.has_attr(def_id, sym::automatically_derived)
    }

automatically_derived is present on the H5Type derive. It is not a builtin automatic derive though.

Or is the issue here that H5Type uses that attribute when it shouldn't? https://doc.rust-lang.org/reference/attributes/derive.html isn't clear about whether or not it is okay for users to put this attribute on their code. But if this is a stable attribute then it can't really be "trusted" for anything in the compiler -- which the docs for is_builtin_derive don't explain though.

Not sure whom to ping for questions like this... @petrochenkov maybe?

// due to compiler wrongfully complaining re: Copy impl missing for packed struct

Is that from the HDF5 crate? That warning is now a hard error and allowing it was never a good idea. I wonder why they say that the complaint is incorrect. I don't think I have seen this as an issue so we were not aware of this problem.

[RalfJung]

Also Cc @aldanor -- this issue affects hdf5.

[clubby789]

Reproducer:

#[derive(Copy, Clone)]
#[repr(packed)]
pub struct Packed(i8, u32);

trait Foo {
    fn evil(&self);
}

#[automatically_derived]
impl Foo for Packed {
    fn evil(&self) {
        unsafe {
            &self.1;
        }
    }
}
fn main() {}

@rustbot label -E-needs-mcve

[saethlin]

Just FYI that link above will rot in this context: When this stops ICEing I will replace it with the non-ICEing output of the compiler.

[nnethercote]

Yeah, is_builtin_derive function would be better named has_automatically_derived_attr.

So, before #104429, an unaligned reference within a function marked with automatically_derived would trigger the unsafe_derive_on_repr_packed. #104429 marked that as an unreachable path because the compiler itself now never generates such unaligned references. But I didn't not consider the case where user code (either macro-generated or hand-written) had the automatically_derived attribute.

So, the question of whether user code can use automatically_derived is a good one.

A related question: why does hdf5-derive use automatically_derived?

I see several possibilities to address this ICE.

• Keep allowing user code to use automatically_derived, and reinstate the unsafe_derive_on_repr_packed warning.
  • Even though that contradicts the automatically_derived documentation; perhaps the documentation could be updated?
  • And perhaps unsafe_derive_on_repr_packed would be an error now rather than a warning, due to make unaligned_reference a hard error #102513.
• Disallow automatically_derived usage on user code, making it a hard error.
  • This would be a backwards incompatible change, but might be justified if the current behaviour is considered a compiler bug.
  • Probably would require a crater run to see how much real code is affected.
• Disallow automatically_derived usage on user code, but initially as a warning.
  • This is a variant of the previous case, which might be warranted if crater shows usage is non-trivial.
  • This would also require reinstating unsafe_derive_on_repr_packed until the warning becomes an error.

// due to compiler wrongfully complaining re: Copy impl missing for packed struct

Is that from the HDF5 crate? That warning is now a hard error and allowing it was never a good idea. I wonder why they say that the complaint is incorrect. I don't think I have seen this as an issue so we were not aware of this problem.

That may be in reference to the old "that does not derive Copy" error message within the unsafe_derived_on_repr_packed check, which is no longer relevant post-#104429.

[nnethercote]

I see several possibilities to address this ICE.

• Keep allowing user code to use automatically_derived, and reinstate the unsafe_derive_on_repr_packed warning.
  • Even though that contradicts the automatically_derived documentation; perhaps the documentation could be updated?
  • And perhaps unsafe_derive_on_repr_packed would be an error now rather than a warning, due to make unaligned_reference a hard error #102513.

Actually, it could be simpler than this. We could just remove the is_builtin_derive check and the unreachable path and use the same error message that is used for unaligned references in user code. I.e. we wouldn't need to special case the error message for builtin derived code any more.

[RalfJung]

Yeah, is_builtin_derive function would be better named has_automatically_derived_attr.

👍

We could just remove the is_builtin_derive check

Yeah I think that makes most sense.

[aldanor]

@RalfJung thanks for pinging and @clubby789 thanks for fixing!

(also, agreed re: UB-like offset-of logic, but it was most likely written before memoffset crate even existing, and certainly needs revisiting now)