Skip to content

Add a lint for static items with large type alignment #8593

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

Closed
wants to merge 4 commits into from
Closed
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 CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4064,6 +4064,7 @@ Released 2018-09-13
[`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
[`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
[`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive
[`static_items_large_align`]: https://rust-lang.github.io/rust-clippy/master/index.html#static_items_large_align
[`std_instead_of_alloc`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_alloc
[`std_instead_of_core`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_core
[`str_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,7 @@ store.register_lints(&[
single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS,
size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT,
slow_vector_initialization::SLOW_VECTOR_INITIALIZATION,
static_items_large_align::STATIC_ITEMS_LARGE_ALIGN,
std_instead_of_core::ALLOC_INSTEAD_OF_CORE,
std_instead_of_core::STD_INSTEAD_OF_ALLOC,
std_instead_of_core::STD_INSTEAD_OF_CORE,
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_pedantic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
LintId::of(ref_option_ref::REF_OPTION_REF),
LintId::of(return_self_not_must_use::RETURN_SELF_NOT_MUST_USE),
LintId::of(semicolon_if_nothing_returned::SEMICOLON_IF_NOTHING_RETURNED),
LintId::of(static_items_large_align::STATIC_ITEMS_LARGE_ALIGN),
LintId::of(strings::STRING_ADD_ASSIGN),
LintId::of(transmute::TRANSMUTE_PTR_TO_PTR),
LintId::of(types::LINKEDLIST),
Expand Down
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ mod single_char_lifetime_names;
mod single_component_path_imports;
mod size_of_in_element_count;
mod slow_vector_initialization;
mod static_items_large_align;
mod std_instead_of_core;
mod strings;
mod strlen_on_c_strings;
Expand Down Expand Up @@ -902,6 +903,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| Box::new(unused_peekable::UnusedPeekable));
store.register_early_pass(|| Box::new(multi_assignments::MultiAssignments));
store.register_late_pass(|| Box::new(bool_to_int_with_if::BoolToIntWithIf));
let page_size = conf.page_size;
store.register_late_pass(move || Box::new(static_items_large_align::StaticItemsLargeAlign { page_size }));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
186 changes: 186 additions & 0 deletions clippy_lints/src/static_items_large_align.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
use clippy_utils::diagnostics::span_lint_and_then;
use rustc_data_structures::fx::FxHashSet;
use rustc_hir::{Item, ItemKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt, TypeAndMut};
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::Span;
use rustc_typeck::hir_ty_to_ty;

pub struct StaticItemsLargeAlign {
pub page_size: u64,
}

declare_clippy_lint! {
/// ### What it does
/// Check for usage of static items which have type alignment larger than page size.
///
/// ### Why is this bad?
/// Due to some known unsound issues, the type alignment may not be fulfilled.
/// For more information, see:
/// <https://github.com/rust-lang/rust/issues/70022> and
/// <https://github.com/rust-lang/rust/issues/70143>.
///
/// ### Example
/// ```rust
/// #[repr(align(0x100000))]
/// struct Aligned(u8);
///
/// static X: Aligned = Aligned(0); // Bad
///
/// fn main() {
/// let x = Aligned(0); // Good
/// println!("{:#x}", &x as *const _ as usize);
/// println!("{:#x}", &X as *const _ as usize);
/// let b = Box::new(Aligned(0)); // Good
/// println!("{:#x}", Box::into_raw(b) as usize);
/// }
/// ```
#[clippy::version = "1.61.0"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[clippy::version = "1.61.0"]
#[clippy::version = "1.66.0"]

This will most likely be merged in the 1.66 cycle.

pub STATIC_ITEMS_LARGE_ALIGN,
Copy link
Member

Choose a reason for hiding this comment

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

The lint name sounds a bit weird to me. Maybe static_items_with_large_alignment but then again, that's a bit wordy... Maybe the current lint name is a good compromise.

pedantic,
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be a good candidate for correctness or at least the suspicious group. Or do you have concerns about drastic false positives this lint might trigger?

"static items with large type alignment, which may trigger unsound problems"
}
impl_lint_pass!(StaticItemsLargeAlign => [STATIC_ITEMS_LARGE_ALIGN]);

impl LateLintPass<'_> for StaticItemsLargeAlign {
fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
if_chain! {
if let ItemKind::Static(hir_ty, _, _) = item.kind;
let ty = hir_ty_to_ty(cx.tcx, hir_ty);
let mut visited_tys = FxHashSet::default();
let mut intermediate_tys = Vec::new();
if let Some(la_ty) = self.check_ty_alignment(cx.tcx, ty, &mut visited_tys, &mut intermediate_tys);
then {
let mut span_notes: Vec<(Span, String)> = Vec::new();
if !intermediate_tys.is_empty() {
let top_ty = intermediate_tys[0].ty;
if !top_ty.is_adt() {
span_notes.push((
hir_ty.span,
format!("this {} contains an inner type with large alignment", top_ty.prefix_string(cx.tcx)),
));
}
intermediate_tys.iter()
.filter_map(|im_ty| Self::report_im_ty(cx, im_ty))
.for_each(|ss| span_notes.push(ss));
}
span_notes.push(self.report_la_ty(cx, &la_ty));

span_lint_and_then(
cx,
STATIC_ITEMS_LARGE_ALIGN,
item.span,
"this static item (itself or its subfield) has a type alignment,\n\
which is larger than page size and may not be fulfilled,\n\
for more information, see <https://github.com/rust-lang/rust/issues/70022>.",
Comment on lines +74 to +76
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"this static item (itself or its subfield) has a type alignment,\n\
which is larger than page size and may not be fulfilled,\n\
for more information, see <https://github.com/rust-lang/rust/issues/70022>.",
"static item with large type alignment",

The lint message is not meant to explain the whole world :P Clippy will add a link to its documentation to the first lint emission anyway, so adding links is not necessary.

move |diag| {
for (span, s) in span_notes {
diag.span_note(span, s.as_str());
}
}
);
}
}
}
}

impl StaticItemsLargeAlign {
/// It checks a type with the following steps:
/// 1. Check if the type is already visited (for a static item),
/// if not, continue;
/// otherwise, return `None` early.
/// 2. Push the type in the checked types.
/// 3. Pick out this type if its kind is among adt, tuple, array, ref or raw ptr to them;
/// otherwise return `None`.
/// 4. Check if its (or its inner fields') alignment are larger than page size.
/// 5. Return one of them;
/// otherwise pop the current checked type and return `None`.
fn check_ty_alignment<'tcx>(
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't rustc have a query that just gives you the type alignment? 🤔 Implementing type alignment calculation in Clippy feels a bit wrong to me.

&self,
tcx: TyCtxt<'tcx>,
ty: Ty<'tcx>,
visited_tys: &mut FxHashSet<Ty<'tcx>>,
intermediate_tys: &mut Vec<IntermediateTy<'tcx>>,
) -> Option<LargeAlignmentTy<'tcx>> {
if visited_tys.contains(&ty) {
return None;
}
visited_tys.insert(ty);
intermediate_tys.push(IntermediateTy { ty });

let ret = match ty.kind() {
ty::Adt(adt_def, subst_ref) => {
if let Some(align) = adt_def.repr().align &&
align.bytes() > self.page_size
{
intermediate_tys.pop(); // the last element is already in the return value
Some(LargeAlignmentTy {
adt: *adt_def,
name: ty.sort_string(tcx).into_owned(),
align: align.bytes(),
})
} else {
adt_def.all_fields()
.map(|field_ref| field_ref.ty(tcx, subst_ref))
.find_map(|ty| self.check_ty_alignment(tcx, ty, visited_tys, intermediate_tys))
}
},
ty::Tuple(ty_list) => ty_list
.iter()
.find_map(|ty| self.check_ty_alignment(tcx, ty, visited_tys, intermediate_tys)),
ty::Array(ty, _) | ty::Ref(_, ty, _) | ty::RawPtr(TypeAndMut { ty, .. }) => {
self.check_ty_alignment(tcx, *ty, visited_tys, intermediate_tys)
},
_ => None,
};

if ret.is_none() {
intermediate_tys.pop();
}
ret
}

fn report_im_ty(cx: &LateContext<'_>, im_ty: &IntermediateTy<'_>) -> Option<(Span, String)> {
let ty = im_ty.ty;
if let ty::Adt(adt_def, substs_ref) = ty.kind() {
Some((
cx.tcx.def_span(adt_def.did()),
if substs_ref.is_empty() {
format!("{} contains an inner type with large alignment", ty.sort_string(cx.tcx))
} else {
// TODO - can we use :#?
Copy link
Member

Choose a reason for hiding this comment

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

Can we? Do you have a test for this? This TODO comment should be resolved before merging.

format!(
"{} with substitutions {:#?},\n\
contains an inner type with large alignment",
Comment on lines +154 to +155
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"{} with substitutions {:#?},\n\
contains an inner type with large alignment",
"{} with substitutions {:#?}, contains an inner type with large alignment",

no newline.

ty.sort_string(cx.tcx),
substs_ref
)
},
))
} else {
None
}
}

fn report_la_ty(&self, cx: &LateContext<'_>, la_ty: &LargeAlignmentTy<'_>) -> (Span, String) {
(
cx.tcx.def_span(la_ty.adt.did()),
format!(
"{} has alignment {:#x}, which is larger than {:#x},\n\
if you know what you are doing, config the default page size clippy uses in clippy.toml",
Comment on lines +170 to +171
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be any line breaks (or full stops) in a help message. The help message should just be: "{} has alignment {:#x}, which is larger than {:#x}"

The clippy.toml doesn't have to be mentioned here, because the config option will be listed in the lint documentation, which is linked to by the first lint emission.

la_ty.name, la_ty.align, self.page_size,
),
)
}
}

struct IntermediateTy<'tcx> {
ty: Ty<'tcx>,
}

struct LargeAlignmentTy<'tcx> {
adt: AdtDef<'tcx>,
name: String,
align: u64,
}
5 changes: 5 additions & 0 deletions clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,11 @@ define_Conf! {
///
/// The maximum size of the `Err`-variant in a `Result` returned from a function
(large_error_threshold: u64 = 128),
/// Lint: STATIC_ITEMS_LARGE_ALIGN.
///
/// The page size of the target platform. It is useful when we know the exact page size and know that
/// it could be fulfilled (e.g., when we are targeting embedded platforms).
(page_size: u64 = 0x1000),
}

/// Search for the configuration file.
Expand Down
1 change: 1 addition & 0 deletions src/docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,7 @@ docs! {
"skip_while_next",
"slow_vector_initialization",
"stable_sort_primitive",
"static_items_large_align",
"std_instead_of_alloc",
"std_instead_of_core",
"str_to_string",
Expand Down
24 changes: 24 additions & 0 deletions src/docs/static_items_large_align.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
### What it does
Check for usage of static items which have type alignment larger than page size.

### Why is this bad?
Due to some known unsound issues, the type alignment may not be fulfilled.
For more information, see:
<https://github.com/rust-lang/rust/issues/70022> and
<https://github.com/rust-lang/rust/issues/70143>.

### Example
```
#[repr(align(0x100000))]
struct Aligned(u8);

static X: Aligned = Aligned(0); // Bad

fn main() {
let x = Aligned(0); // Good
println!("{:#x}", &x as *const _ as usize);
println!("{:#x}", &X as *const _ as usize);
let b = Box::new(Aligned(0)); // Good
println!("{:#x}", Box::into_raw(b) as usize);
}
```
1 change: 1 addition & 0 deletions tests/ui-toml/static_items_large_align/clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
page-size = 0x100000
37 changes: 37 additions & 0 deletions tests/ui-toml/static_items_large_align/static_items_large_align.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// The example is adapted from https://github.com/rust-lang/rust/issues/70022

#![warn(clippy::static_items_large_align)]

#[repr(align(0x100000))]
#[derive(Clone, Copy)]
struct Aligned(u8);

struct AlignedWrapper {
f: u8,
g: Aligned,
}

enum AlignedEnum {
A(Aligned),
}

struct AlignedGeneric<T>(T);

static X: Aligned = Aligned(0);

static X_REF: &Aligned = &Aligned(0);

static ARRAY: [Aligned; 10] = [Aligned(0); 10];

static TUPLE: (u8, Aligned) = (0, Aligned(0));

static XW: AlignedWrapper = AlignedWrapper { f: 0, g: Aligned(0) };

static XE: AlignedEnum = AlignedEnum::A(Aligned(0));

static XG: AlignedGeneric<Aligned> = AlignedGeneric(Aligned(0));

fn main() {
let x = Aligned(0);
println!("{:#x}", Box::into_raw(Box::new(Aligned(0))) as usize);
}
1 change: 1 addition & 0 deletions tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown fie
max-suggested-slice-pattern-length
max-trait-bounds
msrv
page-size
pass-by-value-size-limit
single-char-binding-names-threshold
standard-macro-braces
Expand Down
75 changes: 75 additions & 0 deletions tests/ui/static_items_large_align.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// The example is adapted from https://github.com/rust-lang/rust/issues/70022

#![warn(clippy::static_items_large_align)]

#[repr(align(0x100000))]
#[derive(Clone, Copy)]
struct Aligned(u8);

struct AlignedWrapper {
f: u8,
g: Aligned,
}

enum AlignedEnum {
A(Aligned),
}

struct AlignedPtr(*const Aligned);

unsafe impl Sync for AlignedPtr {}

struct AlignedGeneric<T>(T);

static X: Aligned = Aligned(0);

static X_REF: &Aligned = &Aligned(0);

static X_PTR: AlignedPtr = AlignedPtr(&Aligned(0) as *const _);

static XW: AlignedWrapper = AlignedWrapper { f: 0, g: Aligned(0) };

static ARRAY: [Aligned; 10] = [Aligned(0); 10];

static TUPLE: (u8, (Aligned, u8)) = (0, (Aligned(0), 0));

static XE: AlignedEnum = AlignedEnum::A(Aligned(0));

static XG: AlignedGeneric<Aligned> = AlignedGeneric(Aligned(0));

static XGW: AlignedGeneric<AlignedWrapper> = AlignedGeneric(AlignedWrapper { f: 0, g: Aligned(0) });

fn main() {
let x = Aligned(0);
println!("{:#x}", Box::into_raw(Box::new(Aligned(0))) as usize);
}

////////////////////////////////////////////////////////////////
/////////////// below is a more involved example ///////////////
////////////////////////////////////////////////////////////////

#[repr(align(0x100000))]
struct AlignedA(u8);

#[repr(align(0x100000))]
struct AlignedB(u8);

struct FnPtr<T>(fn() -> Box<T>);

struct AG<T>(T);

type AGAlias<T> = AG<T>;

struct AGWithArgs<A, B> {
not_aligned: FnPtr<A>,
aligned: AGAlias<B>,
}

static XG_ARGS: AGWithArgs<AlignedA, AlignedB> = AGWithArgs {
not_aligned: FnPtr(box_aligned),
aligned: AG(AlignedB(0)),
};

fn box_aligned() -> Box<AlignedA> {
Box::new(AlignedA(0))
}
Loading