From 012413d2fc392f9a064576553349d0d42c199427 Mon Sep 17 00:00:00 2001 From: Joe Richey Date: Sun, 11 Oct 2020 16:04:33 -0700 Subject: [PATCH] clippy_lints: Enable empty_loop lint for no_std crates We use a different message for no_std crates, so that we avoid recomending functions the user cannot use. We also update the documentation to note that the remediations are different for `std` and `no_std` crates. Furthermore, we move the `empty_loop` lint to a "Correctness" lint, as writing `loop {}` can cause miscompilations w/ LLVM. Signed-off-by: Joe Richey --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/loops.rs | 65 +++++++++++++++---- src/lintlist/mod.rs | 2 +- tests/ui/crashes/ice-360.stderr | 5 +- tests/ui/empty_loop.stderr | 11 +++- .../{issue-3746.rs => empty_loop_no_std.rs} | 7 +- tests/ui/empty_loop_no_std.stderr | 11 ++++ 7 files changed, 82 insertions(+), 21 deletions(-) rename tests/ui/{issue-3746.rs => empty_loop_no_std.rs} (68%) create mode 100644 tests/ui/empty_loop_no_std.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 26a727687b1d..334970b6d03a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1566,7 +1566,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY), LintId::of(&len_zero::LEN_ZERO), LintId::of(&literal_representation::INCONSISTENT_DIGIT_GROUPING), - LintId::of(&loops::EMPTY_LOOP), LintId::of(&loops::FOR_KV_MAP), LintId::of(&loops::NEEDLESS_RANGE_LOOP), LintId::of(&loops::SAME_ITEM_PUSH), @@ -1752,6 +1751,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&inline_fn_without_body::INLINE_FN_WITHOUT_BODY), LintId::of(&let_underscore::LET_UNDERSCORE_LOCK), LintId::of(&literal_representation::MISTYPED_LITERAL_SUFFIXES), + LintId::of(&loops::EMPTY_LOOP), LintId::of(&loops::FOR_LOOPS_OVER_FALLIBLES), LintId::of(&loops::ITER_NEXT_LOOP), LintId::of(&loops::NEVER_LOOP), diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index b606db179efd..759a911b84e7 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -293,18 +293,55 @@ declare_clippy_lint! { declare_clippy_lint! { /// **What it does:** Checks for empty `loop` expressions. /// - /// **Why is this bad?** Those busy loops burn CPU cycles without doing - /// anything. Think of the environment and either block on something or at least - /// make the thread sleep for some microseconds. + /// **Why is this bad?** Due to [an LLVM codegen bug](https://github.com/rust-lang/rust/issues/28728) + /// these expressions can be miscompiled or 'optimized' out. Also, these busy loops + /// burn CPU cycles without doing anything. /// - /// **Known problems:** None. + /// It is _almost always_ a better idea to `panic!` than to have a busy loop. + /// + /// If panicking isn't possible, think of the environment and either: + /// - block on something + /// - sleep the thread for some microseconds + /// - yield or pause the thread + /// + /// For `std` targets, this can be done with + /// [`std::thread::sleep`](https://doc.rust-lang.org/std/thread/fn.sleep.html) + /// or [`std::thread::yield_now`](https://doc.rust-lang.org/std/thread/fn.yield_now.html). + /// + /// For `no_std` targets, doing this is more complicated, especially because + /// `#[panic_handler]`s can't panic. To stop/pause the thread, you will + /// probably need to invoke some target-specific intrinsic. Examples include: + /// - [`x86_64::instructions::hlt`](https://docs.rs/x86_64/0.12.2/x86_64/instructions/fn.hlt.html) + /// - [`cortex_m::asm::wfi`](https://docs.rs/cortex-m/0.6.3/cortex_m/asm/fn.wfi.html) + /// + /// If you just care about fixing the LLVM bug (and don't care about burning + /// CPU), you can: + /// - On nightly, insert a non-`pure` `asm!` statement. For example: + /// ```rust + /// loop { + /// unsafe { asm!("") }; + /// } + /// ``` + /// Alternatively, you can compile your code with `-Zinsert-sideeffect`, + /// which will prevent the LLVM from miscompiling your code. + /// - On stable, insert a [`read_volatile`](https://doc.rust-lang.org/core/ptr/fn.read_volatile.html) + /// operation in the loop body. For example: + /// ```rust + /// let dummy = 0u8; + /// loop { + /// unsafe { core::ptr::read_volatile(&dummy) }; + /// } + /// ``` + /// + /// **Known problems:** This is a correctness lint due to the LLVM codegen + /// bug. Once the bug is fixed, this will go back to being a style lint. /// /// **Example:** /// ```no_run /// loop {} /// ``` pub EMPTY_LOOP, - style, + correctness, "empty `loop {}`, which should block or sleep" } @@ -502,14 +539,16 @@ impl<'tcx> LateLintPass<'tcx> for Loops { // (even if the "match" or "if let" is used for declaration) if let ExprKind::Loop(ref block, _, LoopSource::Loop) = expr.kind { // also check for empty `loop {}` statements - if block.stmts.is_empty() && block.expr.is_none() && !is_no_std_crate(cx.tcx.hir().krate()) { - span_lint( - cx, - EMPTY_LOOP, - expr.span, - "empty `loop {}` detected. You may want to either use `panic!()` or add \ - `std::thread::sleep(..);` to the loop body.", - ); + if block.stmts.is_empty() && block.expr.is_none() { + let msg = "empty `loop {}` is currently miscompiled and wastes CPU cycles"; + let help = if is_no_std_crate(cx.tcx.hir().krate()) { + "You should either use `panic!()` or add some call \ + to sleep or pause the thread to the loop body." + } else { + "You should either use `panic!()` or add \ + `std::thread::sleep(..);` to the loop body." + }; + span_lint_and_help(cx, EMPTY_LOOP, expr.span, msg, None, help); } // extract the expression from the first statement (if any) in a block diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 624223ff7062..4bae7a1c457d 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -482,7 +482,7 @@ vec![ }, Lint { name: "empty_loop", - group: "style", + group: "correctness", desc: "empty `loop {}`, which should block or sleep", deprecation: None, module: "loops", diff --git a/tests/ui/crashes/ice-360.stderr b/tests/ui/crashes/ice-360.stderr index 84e31eaf2e9f..49f40cbbb2ea 100644 --- a/tests/ui/crashes/ice-360.stderr +++ b/tests/ui/crashes/ice-360.stderr @@ -12,13 +12,14 @@ LL | | } | = note: `-D clippy::while-let-loop` implied by `-D warnings` -error: empty `loop {}` detected. You may want to either use `panic!()` or add `std::thread::sleep(..);` to the loop body. +error: empty `loop {}` is currently miscompiled and wastes CPU cycles --> $DIR/ice-360.rs:10:9 | LL | loop {} | ^^^^^^^ | - = note: `-D clippy::empty-loop` implied by `-D warnings` + = note: `#[deny(clippy::empty_loop)]` on by default + = help: You should either use `panic!()` or add `std::thread::sleep(..);` to the loop body. error: aborting due to 2 previous errors diff --git a/tests/ui/empty_loop.stderr b/tests/ui/empty_loop.stderr index e44c58ea770a..ec9b6ce41366 100644 --- a/tests/ui/empty_loop.stderr +++ b/tests/ui/empty_loop.stderr @@ -1,22 +1,27 @@ -error: empty `loop {}` detected. You may want to either use `panic!()` or add `std::thread::sleep(..);` to the loop body. +error: empty `loop {}` is currently miscompiled and wastes CPU cycles --> $DIR/empty_loop.rs:9:5 | LL | loop {} | ^^^^^^^ | = note: `-D clippy::empty-loop` implied by `-D warnings` + = help: You should either use `panic!()` or add `std::thread::sleep(..);` to the loop body. -error: empty `loop {}` detected. You may want to either use `panic!()` or add `std::thread::sleep(..);` to the loop body. +error: empty `loop {}` is currently miscompiled and wastes CPU cycles --> $DIR/empty_loop.rs:11:9 | LL | loop {} | ^^^^^^^ + | + = help: You should either use `panic!()` or add `std::thread::sleep(..);` to the loop body. -error: empty `loop {}` detected. You may want to either use `panic!()` or add `std::thread::sleep(..);` to the loop body. +error: empty `loop {}` is currently miscompiled and wastes CPU cycles --> $DIR/empty_loop.rs:15:9 | LL | 'inner: loop {} | ^^^^^^^^^^^^^^^ + | + = help: You should either use `panic!()` or add `std::thread::sleep(..);` to the loop body. error: aborting due to 3 previous errors diff --git a/tests/ui/issue-3746.rs b/tests/ui/empty_loop_no_std.rs similarity index 68% rename from tests/ui/issue-3746.rs rename to tests/ui/empty_loop_no_std.rs index 879d1d5d916e..10b037546dad 100644 --- a/tests/ui/issue-3746.rs +++ b/tests/ui/empty_loop_no_std.rs @@ -10,11 +10,16 @@ use core::panic::PanicInfo; #[start] fn main(argc: isize, argv: *const *const u8) -> isize { - loop {} + // This loop should not cause a warning. + let dummy = 0u8; + loop { + unsafe { core::ptr::read_volatile(&dummy) }; + } } #[panic_handler] fn panic(_info: &PanicInfo) -> ! { + // But this loop will cause a warning. loop {} } diff --git a/tests/ui/empty_loop_no_std.stderr b/tests/ui/empty_loop_no_std.stderr new file mode 100644 index 000000000000..a0ae4d26bee8 --- /dev/null +++ b/tests/ui/empty_loop_no_std.stderr @@ -0,0 +1,11 @@ +error: empty `loop {}` is currently miscompiled and wastes CPU cycles + --> $DIR/empty_loop_no_std.rs:23:5 + | +LL | loop {} + | ^^^^^^^ + | + = note: `-D clippy::empty-loop` implied by `-D warnings` + = help: You should either use `panic!()` or add some call to sleep or pause the thread to the loop body. + +error: aborting due to previous error +