Skip to content

Commit

Permalink
Auto merge of #10827 - y21:large-stack-frames, r=dswij
Browse files Browse the repository at this point in the history
new lint: `large_stack_frames`

This implements a lint that looks for functions that use a lot of stack space.

It uses the MIR because conveniently every temporary gets its own local and I think it maps best to stack space used in a function.
It's probably going to be quite inaccurate in release builds, but at least for debug builds where opts are less aggressive on LLVM's side I think this is accurate "enough".

(This does not work for generic functions yet. Not sure if I should try to get it working in this PR or if it could land without it for now and be added in a followup PR.)

I also put it under the nursery category because this probably needs more work...

changelog: new lint: [`large_stack_frames`]
  • Loading branch information
bors committed Jun 12, 2023
2 parents a3b185b + 6ad7c6f commit ebafbfc
Show file tree
Hide file tree
Showing 9 changed files with 264 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4883,6 +4883,7 @@ Released 2018-09-13
[`large_futures`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_futures
[`large_include_file`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_include_file
[`large_stack_arrays`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_stack_arrays
[`large_stack_frames`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_stack_frames
[`large_types_passed_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_types_passed_by_value
[`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty
[`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
Expand Down
10 changes: 10 additions & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,16 @@ The maximum allowed size for arrays on the stack
* [`large_const_arrays`](https://rust-lang.github.io/rust-clippy/master/index.html#large_const_arrays)


## `stack-size-threshold`
The maximum allowed stack size for functions in bytes

**Default Value:** `512000` (`u64`)

---
**Affected lints:**
* [`large_stack_frames`](https://rust-lang.github.io/rust-clippy/master/index.html#large_stack_frames)


## `vec-box-size-threshold`
The size of the boxed type in bytes, where boxing in a `Vec` is allowed

Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::large_futures::LARGE_FUTURES_INFO,
crate::large_include_file::LARGE_INCLUDE_FILE_INFO,
crate::large_stack_arrays::LARGE_STACK_ARRAYS_INFO,
crate::large_stack_frames::LARGE_STACK_FRAMES_INFO,
crate::len_zero::COMPARISON_TO_EMPTY_INFO,
crate::len_zero::LEN_WITHOUT_IS_EMPTY_INFO,
crate::len_zero::LEN_ZERO_INFO,
Expand Down
162 changes: 162 additions & 0 deletions clippy_lints/src/large_stack_frames.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
use std::ops::AddAssign;

use clippy_utils::diagnostics::span_lint_and_note;
use clippy_utils::fn_has_unsatisfiable_preds;
use rustc_hir::def_id::LocalDefId;
use rustc_hir::intravisit::FnKind;
use rustc_hir::Body;
use rustc_hir::FnDecl;
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::declare_tool_lint;
use rustc_session::impl_lint_pass;
use rustc_span::Span;

declare_clippy_lint! {
/// ### What it does
/// Checks for functions that use a lot of stack space.
///
/// This often happens when constructing a large type, such as an array with a lot of elements,
/// or constructing *many* smaller-but-still-large structs, or copying around a lot of large types.
///
/// This lint is a more general version of [`large_stack_arrays`](https://rust-lang.github.io/rust-clippy/master/#large_stack_arrays)
/// that is intended to look at functions as a whole instead of only individual array expressions inside of a function.
///
/// ### Why is this bad?
/// The stack region of memory is very limited in size (usually *much* smaller than the heap) and attempting to
/// use too much will result in a stack overflow and crash the program.
/// To avoid this, you should consider allocating large types on the heap instead (e.g. by boxing them).
///
/// Keep in mind that the code path to construction of large types does not even need to be reachable;
/// it purely needs to *exist* inside of the function to contribute to the stack size.
/// For example, this causes a stack overflow even though the branch is unreachable:
/// ```rust,ignore
/// fn main() {
/// if false {
/// let x = [0u8; 10000000]; // 10 MB stack array
/// black_box(&x);
/// }
/// }
/// ```
///
/// ### Known issues
/// False positives. The stack size that clippy sees is an estimated value and can be vastly different
/// from the actual stack usage after optimizations passes have run (especially true in release mode).
/// Modern compilers are very smart and are able to optimize away a lot of unnecessary stack allocations.
/// In debug mode however, it is usually more accurate.
///
/// This lint works by summing up the size of all variables that the user typed, variables that were
/// implicitly introduced by the compiler for temporaries, function arguments and the return value,
/// and comparing them against a (configurable, but high-by-default).
///
/// ### Example
/// This function creates four 500 KB arrays on the stack. Quite big but just small enough to not trigger `large_stack_arrays`.
/// However, looking at the function as a whole, it's clear that this uses a lot of stack space.
/// ```rust
/// struct QuiteLargeType([u8; 500_000]);
/// fn foo() {
/// // ... some function that uses a lot of stack space ...
/// let _x1 = QuiteLargeType([0; 500_000]);
/// let _x2 = QuiteLargeType([0; 500_000]);
/// let _x3 = QuiteLargeType([0; 500_000]);
/// let _x4 = QuiteLargeType([0; 500_000]);
/// }
/// ```
///
/// Instead of doing this, allocate the arrays on the heap.
/// This currently requires going through a `Vec` first and then converting it to a `Box`:
/// ```rust
/// struct NotSoLargeType(Box<[u8]>);
///
/// fn foo() {
/// let _x1 = NotSoLargeType(vec![0; 500_000].into_boxed_slice());
/// // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Now heap allocated.
/// // The size of `NotSoLargeType` is 16 bytes.
/// // ...
/// }
/// ```
#[clippy::version = "1.71.0"]
pub LARGE_STACK_FRAMES,
nursery,
"checks for functions that allocate a lot of stack space"
}

pub struct LargeStackFrames {
maximum_allowed_size: u64,
}

impl LargeStackFrames {
#[must_use]
pub fn new(size: u64) -> Self {
Self {
maximum_allowed_size: size,
}
}
}

impl_lint_pass!(LargeStackFrames => [LARGE_STACK_FRAMES]);

#[derive(Copy, Clone)]
enum Space {
Used(u64),
Overflow,
}

impl Space {
pub fn exceeds_limit(self, limit: u64) -> bool {
match self {
Self::Used(used) => used > limit,
Self::Overflow => true,
}
}
}

impl AddAssign<u64> for Space {
fn add_assign(&mut self, rhs: u64) {
if let Self::Used(lhs) = self {
match lhs.checked_add(rhs) {
Some(sum) => *self = Self::Used(sum),
None => *self = Self::Overflow,
}
}
}
}

impl<'tcx> LateLintPass<'tcx> for LargeStackFrames {
fn check_fn(
&mut self,
cx: &LateContext<'tcx>,
_: FnKind<'tcx>,
_: &'tcx FnDecl<'tcx>,
_: &'tcx Body<'tcx>,
span: Span,
local_def_id: LocalDefId,
) {
let def_id = local_def_id.to_def_id();
// Building MIR for `fn`s with unsatisfiable preds results in ICE.
if fn_has_unsatisfiable_preds(cx, def_id) {
return;
}

let mir = cx.tcx.optimized_mir(def_id);
let param_env = cx.tcx.param_env(def_id);

let mut frame_size = Space::Used(0);

for local in &mir.local_decls {
if let Ok(layout) = cx.tcx.layout_of(param_env.and(local.ty)) {
frame_size += layout.size.bytes();
}
}

if frame_size.exceeds_limit(self.maximum_allowed_size) {
span_lint_and_note(
cx,
LARGE_STACK_FRAMES,
span,
"this function allocates a large amount of stack space",
None,
"allocating large amounts of stack space can overflow the stack",
);
}
}
}
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ mod large_enum_variant;
mod large_futures;
mod large_include_file;
mod large_stack_arrays;
mod large_stack_frames;
mod len_zero;
mod let_if_seq;
mod let_underscore;
Expand Down Expand Up @@ -1042,6 +1043,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
min_ident_chars_threshold,
})
});
let stack_size_threshold = conf.stack_size_threshold;
store.register_late_pass(move |_| Box::new(large_stack_frames::LargeStackFrames::new(stack_size_threshold)));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
4 changes: 4 additions & 0 deletions clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,10 @@ define_Conf! {
///
/// The maximum allowed size for arrays on the stack
(array_size_threshold: u64 = 512_000),
/// Lint: LARGE_STACK_FRAMES.
///
/// The maximum allowed stack size for functions in bytes
(stack_size_threshold: u64 = 512_000),
/// Lint: VEC_BOX.
///
/// The size of the boxed type in bytes, where boxing in a `Vec` is allowed
Expand Down
2 changes: 2 additions & 0 deletions tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect
semicolon-inside-block-ignore-singleline
semicolon-outside-block-ignore-multiline
single-char-binding-names-threshold
stack-size-threshold
standard-macro-braces
suppress-restriction-lint-in-const
third-party
Expand Down Expand Up @@ -109,6 +110,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect
semicolon-inside-block-ignore-singleline
semicolon-outside-block-ignore-multiline
single-char-binding-names-threshold
stack-size-threshold
standard-macro-braces
suppress-restriction-lint-in-const
third-party
Expand Down
44 changes: 44 additions & 0 deletions tests/ui/large_stack_frames.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#![allow(unused, incomplete_features)]
#![warn(clippy::large_stack_frames)]
#![feature(unsized_locals)]

use std::hint::black_box;

fn generic<T: Default>() {
let x = T::default();
black_box(&x);
}

fn unsized_local() {
let x: dyn std::fmt::Display = *(Box::new(1) as Box<dyn std::fmt::Display>);
black_box(&x);
}

struct ArrayDefault<const N: usize>([u8; N]);

impl<const N: usize> Default for ArrayDefault<N> {
fn default() -> Self {
Self([0; N])
}
}

fn many_small_arrays() {
let x = [0u8; 500_000];
let x2 = [0u8; 500_000];
let x3 = [0u8; 500_000];
let x4 = [0u8; 500_000];
let x5 = [0u8; 500_000];
black_box((&x, &x2, &x3, &x4, &x5));
}

fn large_return_value() -> ArrayDefault<1_000_000> {
Default::default()
}

fn large_fn_arg(x: ArrayDefault<1_000_000>) {
black_box(&x);
}

fn main() {
generic::<ArrayDefault<1_000_000>>();
}
37 changes: 37 additions & 0 deletions tests/ui/large_stack_frames.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
error: this function allocates a large amount of stack space
--> $DIR/large_stack_frames.rs:25:1
|
LL | / fn many_small_arrays() {
LL | | let x = [0u8; 500_000];
LL | | let x2 = [0u8; 500_000];
LL | | let x3 = [0u8; 500_000];
... |
LL | | black_box((&x, &x2, &x3, &x4, &x5));
LL | | }
| |_^
|
= note: allocating large amounts of stack space can overflow the stack
= note: `-D clippy::large-stack-frames` implied by `-D warnings`

error: this function allocates a large amount of stack space
--> $DIR/large_stack_frames.rs:34:1
|
LL | / fn large_return_value() -> ArrayDefault<1_000_000> {
LL | | Default::default()
LL | | }
| |_^
|
= note: allocating large amounts of stack space can overflow the stack

error: this function allocates a large amount of stack space
--> $DIR/large_stack_frames.rs:38:1
|
LL | / fn large_fn_arg(x: ArrayDefault<1_000_000>) {
LL | | black_box(&x);
LL | | }
| |_^
|
= note: allocating large amounts of stack space can overflow the stack

error: aborting due to 3 previous errors

0 comments on commit ebafbfc

Please sign in to comment.