Skip to content

make unaligned_references future-incompat lint warn-by-default #82525

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) {
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");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I called this a "rename" now because the lints are fairly similar... but maybe "remove" would be more appropriate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No opinion, seems fine as is.


// These were moved to tool lints, but rustc still sees them when compiling normally, before
// tool lints are registered, so `check_tool_name_for_backwards_compat` doesn't work. Use
Expand Down
65 changes: 11 additions & 54 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1057,6 +1057,7 @@ declare_lint! {
/// unsafe {
/// let foo = Foo { field1: 0, field2: 0 };
/// let _ = &foo.field1;
/// println!("{}", foo.field1); // An implicit `&` is added here, triggering the lint.
/// }
/// }
/// ```
Expand All @@ -1065,20 +1066,20 @@ declare_lint! {
///
/// ### Explanation
///
/// Creating a reference to an insufficiently aligned packed field is
/// [undefined behavior] and should be disallowed.
///
/// This lint is "allow" by default because there is no stable
/// alternative, and it is not yet certain how widespread existing code
/// will trigger this lint.
///
/// See [issue #27060] for more discussion.
/// 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 #27060]: https://github.com/rust-lang/rust/issues/27060
/// [issue #82523]: https://github.com/rust-lang/rust/issues/82523
pub UNALIGNED_REFERENCES,
Allow,
Warn,
"detects unaligned references to fields of packed structs",
@future_incompatible = FutureIncompatibleInfo {
reference: "issue #82523 <https://github.com/rust-lang/rust/issues/82523>",
edition: None,
};
report_in_external_macro
}

Expand Down Expand Up @@ -1150,49 +1151,6 @@ declare_lint! {
"detects attempts to mutate a `const` item",
}

declare_lint! {
/// The `safe_packed_borrows` lint detects borrowing a field in the
/// interior of a packed structure with alignment other than 1.
///
/// ### Example
///
/// ```rust
/// #[repr(packed)]
/// pub struct Unaligned<T>(pub T);
///
/// pub struct Foo {
/// start: u8,
/// data: Unaligned<u32>,
/// }
///
/// fn main() {
/// let x = Foo { start: 0, data: Unaligned(1) };
/// let y = &x.data.0;
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// This type of borrow is unsafe and can cause errors on some platforms
/// and violates some assumptions made by the compiler. This was
/// previously allowed unintentionally. This is a [future-incompatible]
/// lint to transition this to a hard error in the future. See [issue
/// #46043] for more details, including guidance on how to solve the
/// problem.
///
/// [issue #46043]: https://github.com/rust-lang/rust/issues/46043
/// [future-incompatible]: ../index.md#future-incompatible-lints
pub SAFE_PACKED_BORROWS,
Warn,
"safe borrows of fields of packed structs were erroneously allowed",
@future_incompatible = FutureIncompatibleInfo {
reference: "issue #46043 <https://github.com/rust-lang/rust/issues/46043>",
edition: None,
};
}

declare_lint! {
/// The `patterns_in_fns_without_body` lint detects `mut` identifier
/// patterns as a parameter in functions without a body.
Expand Down Expand Up @@ -2953,7 +2911,6 @@ declare_lint_pass! {
RENAMED_AND_REMOVED_LINTS,
UNALIGNED_REFERENCES,
CONST_ITEM_MUTATION,
SAFE_PACKED_BORROWS,
PATTERNS_IN_FNS_WITHOUT_BODY,
MISSING_FRAGMENT_SPECIFIER,
LATE_BOUND_LIFETIME_ARGUMENTS,
Expand Down
6 changes: 0 additions & 6 deletions compiler/rustc_middle/src/mir/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,9 @@ pub enum UnsafetyViolationKind {
General,
/// Permitted both in `const fn`s and regular `fn`s.
GeneralAndConstFn,
/// Borrow of packed field.
/// Has to be handled as a lint for backwards compatibility.
BorrowPacked,
/// Unsafe operation in an `unsafe fn` but outside an `unsafe` block.
/// Has to be handled as a lint for backwards compatibility.
UnsafeFn,
/// Borrow of packed field in an `unsafe fn` but outside an `unsafe` block.
/// Has to be handled as a lint for backwards compatibility.
UnsafeFnBorrowPacked,
}

#[derive(Copy, Clone, PartialEq, TyEncodable, TyDecodable, HashStable, Debug)]
Expand Down
89 changes: 69 additions & 20 deletions compiler/rustc_mir/src/transform/check_packed_ref.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_middle::mir::visit::{PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, TyCtxt};
use rustc_session::lint::builtin::UNALIGNED_REFERENCES;
use rustc_span::symbol::sym;

use crate::transform::MirPass;
use crate::util;

pub(crate) fn provide(providers: &mut Providers) {
*providers = Providers { unsafe_derive_on_repr_packed, ..*providers };
}

pub struct CheckPackedRef;

impl<'tcx> MirPass<'tcx> for CheckPackedRef {
Expand All @@ -24,6 +31,41 @@ struct PackedRefChecker<'a, 'tcx> {
source_info: SourceInfo,
}

fn unsafe_derive_on_repr_packed(tcx: TyCtxt<'_>, def_id: LocalDefId) {
let lint_hir_id = tcx.hir().local_def_id_to_hir_id(def_id);

tcx.struct_span_lint_hir(UNALIGNED_REFERENCES, lint_hir_id, tcx.def_span(def_id), |lint| {
// FIXME: when we make this a hard error, this should have its
// own error code.
let message = if tcx.generics_of(def_id).own_requires_monomorphization() {
"`#[derive]` can't be used on a `#[repr(packed)]` struct with \
type or const parameters (error E0133)"
.to_string()
} else {
"`#[derive]` can't be used on a `#[repr(packed)]` struct that \
does not derive Copy (error E0133)"
.to_string()
};
lint.build(&message).emit()
});
}

fn builtin_derive_def_id(tcx: TyCtxt<'_>, def_id: DefId) -> Option<DefId> {
debug!("builtin_derive_def_id({:?})", def_id);
if let Some(impl_def_id) = tcx.impl_of_method(def_id) {
if tcx.has_attr(impl_def_id, sym::automatically_derived) {
debug!("builtin_derive_def_id({:?}) - is {:?}", def_id, impl_def_id);
Some(impl_def_id)
} else {
debug!("builtin_derive_def_id({:?}) - not automatically derived", def_id);
None
}
} else {
debug!("builtin_derive_def_id({:?}) - not a method", def_id);
None
}
}

impl<'a, 'tcx> Visitor<'tcx> for PackedRefChecker<'a, 'tcx> {
fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
// Make sure we know where in the MIR we are.
Expand All @@ -40,26 +82,33 @@ impl<'a, 'tcx> Visitor<'tcx> for PackedRefChecker<'a, 'tcx> {
fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, _location: Location) {
if context.is_borrow() {
if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
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,
|lint| {
lint.build("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)",
)
.emit()
},
);
let def_id = self.body.source.instance.def_id();
if let Some(impl_def_id) = builtin_derive_def_id(self.tcx, def_id) {
// If a method is defined in the local crate,
// the impl containing that method should also be.
self.tcx.ensure().unsafe_derive_on_repr_packed(impl_def_id.expect_local());
} 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,
|lint| {
lint.build("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)",
)
.emit()
},
);
}
}
}
}
Expand Down
Loading