Auto merge of #102513 - RalfJung:no-more-unaligned-reference, r=cjgil…

…lot,scottmcm

make unaligned_reference a hard error

The `unaligned_references` lint has been warn-by-default since Rust 1.53 (#82525) and deny-by-default with mention in cargo future-incompat reports since Rust 1.62 (#95372). Current nightly will become Rust 1.66, so (unless major surprises show up with crater) I think it is time we make this a hard error, and close this old soundness gap in the language.

EDIT: Turns out this will only land for Rust 1.67, so there is another 6 weeks of time here for crates to adjust.

Fixes #82523.

compiler/rustc_error_codes/src/error_codes.rs

@@ -5,7 +5,7 @@
 // /!\ IMPORTANT /!\
 //
 // Error messages' format must follow the RFC 1567 available here:
-// https://github.com/rust-lang/rfcs/pull/1567
+// https://rust-lang.github.io/rfcs/1567-long-error-codes-explanation-normalization.html
 
 register_diagnostics! {
 E0001: include_str!("./error_codes/E0001.md"),
@@ -510,6 +510,7 @@ E0789: include_str!("./error_codes/E0789.md"),
 E0790: include_str!("./error_codes/E0790.md"),
 E0791: include_str!("./error_codes/E0791.md"),
 E0792: include_str!("./error_codes/E0792.md"),
+E0793: include_str!("./error_codes/E0793.md"),
 ;
 //  E0006, // merged with E0005
 //  E0008, // cannot bind by-move into a pattern guard

compiler/rustc_error_codes/src/error_codes/E0793.md

@@ -0,0 +1,64 @@
+An unaligned references to a field of a [packed] struct got created.
+
+Erroneous code example:
+
+```compile_fail,E0793
+#[repr(packed)]
+pub struct Foo {
+    field1: u64,
+    field2: u8,
+}
+
+unsafe {
+    let foo = Foo { field1: 0, field2: 0 };
+    // Accessing the field directly is fine.
+    let val = foo.field1;
+    // A reference to a packed field causes a error.
+    let val = &foo.field1; // ERROR
+    // An implicit `&` is added in format strings, causing the same error.
+    println!("{}", foo.field1); // ERROR
+}
+```
+
+Creating a reference to an insufficiently aligned packed field is
+[undefined behavior] and therefore disallowed. Using an `unsafe` block does not
+change anything about this. Instead, the code should do a copy of the data in
+the packed field or use raw pointers and unaligned accesses.
+
+```
+#[repr(packed)]
+pub struct Foo {
+    field1: u64,
+    field2: u8,
+}
+
+unsafe {
+    let foo = Foo { field1: 0, field2: 0 };
+
+    // Instead of a reference, we can create a raw pointer...
+    let ptr = std::ptr::addr_of!(foo.field1);
+    // ... and then (crucially!) access it in an explicitly unaligned way.
+    let val = unsafe { ptr.read_unaligned() };
+    // This would *NOT* be correct:
+    // let val = unsafe { *ptr }; // Undefined Behavior due to unaligned load!
+
+    // For formatting, we can create a copy to avoid the direct reference.
+    let copy = foo.field1;
+    println!("{}", copy);
+    // Creating a copy can be written in a single line with curly braces.
+    // (This is equivalent to the two lines above.)
+    println!("{}", { foo.field1 });
+}
+```
+
+### Additional information
+
+Note that this error is specifically about *references* to packed fields.
+Direct by-value access of those fields is fine, since then the compiler has
+enough information to generate the correct kind of access.
+
+See [issue #82523] for more information.
+
+[packed]: https://doc.rust-lang.org/reference/type-layout.html#the-alignment-modifiers
+[undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html
+[issue #82523]: https://github.com/rust-lang/rust/issues/82523

compiler/rustc_lint/src/lib.rs

@@ -324,7 +324,6 @@ fn register_builtins(store: &mut LintStore) {
     store.register_renamed("exceeding_bitshifts", "arithmetic_overflow");
     store.register_renamed("redundant_semicolon", "redundant_semicolons");
     store.register_renamed("overlapping_patterns", "overlapping_range_endpoints");
-    store.register_renamed("safe_packed_borrows", "unaligned_references");
     store.register_renamed("disjoint_capture_migration", "rust_2021_incompatible_closure_captures");
     store.register_renamed("or_patterns_back_compat", "rust_2021_incompatible_or_patterns");
     store.register_renamed("non_fmt_panic", "non_fmt_panics");
@@ -487,6 +486,16 @@ fn register_builtins(store: &mut LintStore) {
         "converted into hard error, see issue #71800 \
          <https://github.com/rust-lang/rust/issues/71800> for more information",
     );
+    store.register_removed(
+        "safe_packed_borrows",
+        "converted into hard error, see issue #82523 \
+         <https://github.com/rust-lang/rust/issues/82523> for more information",
+    );
+    store.register_removed(
+        "unaligned_references",
+        "converted into hard error, see issue #82523 \
+         <https://github.com/rust-lang/rust/issues/82523> for more information",
+    );
 }
 
 fn register_internals(store: &mut LintStore) {

compiler/rustc_lint_defs/src/builtin.rs

@@ -1187,51 +1187,6 @@ declare_lint! {
     "lints that have been renamed or removed"
 }
 
-declare_lint! {
-    /// The `unaligned_references` lint detects unaligned references to fields
-    /// of [packed] structs.
-    ///
-    /// [packed]: https://doc.rust-lang.org/reference/type-layout.html#the-alignment-modifiers
-    ///
-    /// ### Example
-    ///
-    /// ```rust,compile_fail
-    /// #[repr(packed)]
-    /// pub struct Foo {
-    ///     field1: u64,
-    ///     field2: u8,
-    /// }
-    ///
-    /// fn main() {
-    ///     unsafe {
-    ///         let foo = Foo { field1: 0, field2: 0 };
-    ///         let _ = &foo.field1;
-    ///         println!("{}", foo.field1); // An implicit `&` is added here, triggering the lint.
-    ///     }
-    /// }
-    /// ```
-    ///
-    /// {{produces}}
-    ///
-    /// ### Explanation
-    ///
-    /// Creating a reference to an insufficiently aligned packed field is [undefined behavior] and
-    /// should be disallowed. Using an `unsafe` block does not change anything about this. Instead,
-    /// the code should do a copy of the data in the packed field or use raw pointers and unaligned
-    /// accesses. See [issue #82523] for more information.
-    ///
-    /// [undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html
-    /// [issue #82523]: https://github.com/rust-lang/rust/issues/82523
-    pub UNALIGNED_REFERENCES,
-    Deny,
-    "detects unaligned references to fields of packed structs",
-    @future_incompatible = FutureIncompatibleInfo {
-        reference: "issue #82523 <https://github.com/rust-lang/rust/issues/82523>",
-        reason: FutureIncompatibilityReason::FutureReleaseErrorReportNow,
-    };
-    report_in_external_macro
-}
-
 declare_lint! {
     /// The `const_item_mutation` lint detects attempts to mutate a `const`
     /// item.
@@ -3308,7 +3263,6 @@ declare_lint_pass! {
         PUB_USE_OF_PRIVATE_EXTERN_CRATE,
         INVALID_TYPE_PARAM_DEFAULT,
         RENAMED_AND_REMOVED_LINTS,
-        UNALIGNED_REFERENCES,
         CONST_ITEM_MUTATION,
         PATTERNS_IN_FNS_WITHOUT_BODY,
         MISSING_FRAGMENT_SPECIFIER,

compiler/rustc_mir_transform/src/check_packed_ref.rs

@@ -1,7 +1,7 @@
+use rustc_errors::struct_span_err;
 use rustc_middle::mir::visit::{PlaceContext, Visitor};
 use rustc_middle::mir::*;
 use rustc_middle::ty::{self, TyCtxt};
-use rustc_session::lint::builtin::UNALIGNED_REFERENCES;
 
 use crate::util;
 use crate::MirLint;
@@ -49,31 +49,22 @@ impl<'tcx> Visitor<'tcx> for PackedRefChecker<'_, 'tcx> {
                     // shouldn't do.
                     unreachable!();
                 } else {
-                    let source_info = self.source_info;
-                    let lint_root = self.body.source_scopes[source_info.scope]
-                        .local_data
-                        .as_ref()
-                        .assert_crate_local()
-                        .lint_root;
-                    self.tcx.struct_span_lint_hir(
-                        UNALIGNED_REFERENCES,
-                        lint_root,
-                        source_info.span,
-                        "reference to packed field is unaligned",
-                        |lint| {
-                            lint
-                                .note(
-                                    "fields of packed structs are not properly aligned, and creating \
-                                    a misaligned reference is undefined behavior (even if that \
-                                    reference is never dereferenced)",
-                                )
-                                .help(
-                                    "copy the field contents to a local variable, or replace the \
-                                    reference with a raw pointer and use `read_unaligned`/`write_unaligned` \
-                                    (loads and stores via `*p` must be properly aligned even when using raw pointers)"
-                                )
-                        },
-                    );
+                    struct_span_err!(
+                        self.tcx.sess,
+                        self.source_info.span,
+                        E0793,
+                        "reference to packed field is unaligned"
+                    )
+                    .note(
+                        "fields of packed structs are not properly aligned, and creating \
+                        a misaligned reference is undefined behavior (even if that \
+                        reference is never dereferenced)",
+                    ).help(
+                        "copy the field contents to a local variable, or replace the \
+                        reference with a raw pointer and use `read_unaligned`/`write_unaligned` \
+                        (loads and stores via `*p` must be properly aligned even when using raw pointers)"
+                    )
+                    .emit();
                 }
             }
         }

src/tools/miri/tests/fail/unaligned_pointers/reference_to_packed.rs

@@ -1,19 +1,26 @@
 // This should fail even without validation/SB
 //@compile-flags: -Zmiri-disable-validation -Zmiri-disable-stacked-borrows
 
-#![allow(dead_code, unused_variables, unaligned_references)]
+#![allow(dead_code, unused_variables)]
+
+use std::{ptr, mem};
 
 #[repr(packed)]
 struct Foo {
     x: i32,
     y: i32,
 }
 
+unsafe fn raw_to_ref<'a, T>(x: *const T) -> &'a T {
+    mem::transmute(x)
+}
+
 fn main() {
     // Try many times as this might work by chance.
     for _ in 0..20 {
         let foo = Foo { x: 42, y: 99 };
-        let p = &foo.x;
-        let i = *p; //~ERROR: alignment 4 is required
+        // There seem to be implicit reborrows, which make the error already appear here
+        let p: &i32 = unsafe { raw_to_ref(ptr::addr_of!(foo.x)) }; //~ERROR: alignment 4 is required
+        let i = *p;
     }
 }

src/tools/miri/tests/fail/unaligned_pointers/reference_to_packed.stderr

@@ -1,8 +1,8 @@
 error: Undefined Behavior: accessing memory with alignment ALIGN, but alignment ALIGN is required
   --> $DIR/reference_to_packed.rs:LL:CC
    |
-LL |         let i = *p;
-   |                 ^^ accessing memory with alignment ALIGN, but alignment ALIGN is required
+LL |         let p: &i32 = unsafe { raw_to_ref(ptr::addr_of!(foo.x)) };
+   |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ accessing memory with alignment ALIGN, but alignment ALIGN is required
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

tests/ui/binding/issue-53114-safety-checks.rs

@@ -21,13 +21,11 @@ fn let_wild_gets_unsafe_field() {
     let u2 = U { a: I(1) };
     let p = P { a: &2, b: &3 };
     let _ = &p.b;  //~ ERROR    reference to packed field
-    //~^  WARN will become a hard error
     let _ = u1.a;  // #53114: should eventually signal error as well
     let _ = &u2.a; //~ ERROR  [E0133]
 
     // variation on above with `_` in substructure
     let (_,) = (&p.b,);  //~ ERROR     reference to packed field
-    //~^  WARN will become a hard error
     let (_,) = (u1.a,);  //~ ERROR   [E0133]
     let (_,) = (&u2.a,); //~ ERROR   [E0133]
 }
@@ -37,13 +35,11 @@ fn match_unsafe_field_to_wild() {
     let u2 = U { a: I(1) };
     let p = P { a: &2, b: &3 };
     match &p.b  { _ => { } } //~ ERROR     reference to packed field
-    //~^  WARN will become a hard error
     match u1.a  { _ => { } } //~ ERROR   [E0133]
     match &u2.a { _ => { } } //~ ERROR   [E0133]
 
     // variation on above with `_` in substructure
     match (&p.b,)  { (_,) => { } } //~ ERROR     reference to packed field
-    //~^  WARN will become a hard error
     match (u1.a,)  { (_,) => { } } //~ ERROR   [E0133]
     match (&u2.a,) { (_,) => { } } //~ ERROR   [E0133]
 }