Update empty_loop documentation/message. #6162

josephlr · 2020-10-11T23:28:50Z

Originally part of #6161, but now this PR only deals with std crates

This change:

Updates the std message .
Updates the docs to mention how the busy loops should be fixed
- Gives examples of how to do this for no_std targets
Updates the tests/stderr files to test this change.

changelog: Update empty_loop lint documentation

rust-highfive · 2020-10-11T23:28:53Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @phansch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

phansch · 2020-10-12T14:08:51Z

Thanks for the PR! The implementation looks good to me but I'm not using no_std myself, so I've asked on Zulip for a second pair of eyes.

Lokathor · 2020-10-12T16:36:36Z

Using spin_loop_hint does not resolve the LLVM bug.

Further, spin_loop_hint is likely to do nothing at all on any sort of no_std where there's actually no threading available (as opposed to a no_std lib that is then called by std using code that manages the threads).

jyn514 · 2020-10-12T21:28:23Z

Yeah really what this needs is a std::hint::make_forward_progress() intrinsic, but that doesn't exist AFAIK. Maybe you can emulate it with a volatile read/write?

Lokathor · 2020-10-12T21:35:24Z

You can.

let x = 0;
unsafe { (&x as *const i32).read_volatile() };

All the "forward progress" you'd ever need.

jyn514 · 2020-10-12T21:43:39Z

Nice, that looks about right.

@josephlr either way, this should not have a spin_loop_hint() suggestion; I would recommend the volatile read unconditionally since this pops up for both std and no-std.

ebroto · 2020-10-13T14:33:16Z

I would recommend the volatile read unconditionally since this pops up for both std and no-std

The thing is that this lint is serving two purposes now: 1) don't burn CPU 2) don't risk hitting the case where LLVM changes the semantics of your code.

In non-no_std code, you usually want to sleep or block, so I think we should not suggest the volatile read there. For no_std that could work, but personally I'm a bit concerned about suggesting unsafe instead of the (supposedly) safe code of loop {}.

jyn514 · 2020-10-13T14:58:22Z

In non-no_std code, you usually want to sleep or block, so I think we should not suggest the volatile read there.

Sure, if you want to suggest thread::sleep(...) instead that sounds better.

For no_std that could work, but personally I'm a bit concerned about suggesting unsafe instead of the (supposedly) safe code of loop {}.

🤷 this is the situation we're in. It's better to have visible unsafe than invisible unsoundness. Right now loop {} looks the same to LLVM as unsafe { unreachable_unchecked() } which is clearly worse.

ebroto · 2020-10-13T15:08:16Z

It's better to have visible unsafe than invisible unsoundness

I guess you're right. We can always suggest it as a last resort.

Lokathor · 2020-10-13T18:27:39Z

no_std has no portable way to sleep. you can't suggest thread::sleep, because there's no such thing.

It's maybe a drag, but as jyn514 said, this is the world we live in.

jonas-schievink · 2020-10-13T18:59:55Z

You can also suggest compiling with -Zinsert-sideeffect on nightly

therealprof · 2020-10-13T19:15:50Z

How about making sure that spin_loop_hint always has a side effect?

The documentation clearly states its intended use:

Signals the processor that it is inside a busy-wait spin-loop ("spin lock").

Upon receiving spin-loop signal the processor can optimize its behavior by, for example, saving power or switching hyper-threads.

If it cannot be used for an empty loop I'd consider that a bug in spin_loop_hint.

BTW: It works just fine for the majority of Rust supported embedded systems (running on ARM Cortex-M MCUs) by turning into a yield instruction.

Lokathor · 2020-10-13T20:00:01Z

It's a hint. A hint never changes the correctness (or not) of a program.

For example, I do my embedded work on the GBA, and spin_loop_hint does nothing because the ARM7TDMI CPU of a GBA has no built in assembly instruction for a power saving mode like that.

therealprof · 2020-10-13T20:12:19Z

It's a hint. A hint never changes the correctness (or not) of a program.

The program is correct with or without the hint. However since empty loops are frowned upon and miscompiled by the compiler there should be a proper and official way to declare an intentionally empty loop.

For example, I do my embedded work on the GBA, and spin_loop_hint does nothing because the ARM7TDMI CPU of a GBA has no built in assembly instruction for a power saving mode like that.

Fair enough, but what would be the issue using a nop instead? The yield instruction on most Cortex-M MCUs is also just a fancy way to encode a nop.

Lokathor · 2020-10-13T20:18:28Z

The fact that the codegen will miscompile the program means that the program is not correct with or without the hint.

The intended semantics of loop{} is "that's fine" the actual semantics of loop{} is "LLVM will burn you". We have to live by the semantics we get.

The official, unstable, way to handle this problem is the -Zinsert-sideeffect flag that jonas-schievink mentioned.

The stable way to handle this is to volatile read a stack variable, like I said.

Fair enough, but what would be the issue using a nop instead?

Well, because that's not a loop.

therealprof · 2020-10-13T20:36:17Z

The fact that the codegen will miscompile the program means that the program is not correct with or without the hint.
The intended semantics of loop{} is "that's fine" the actual semantics of loop{} is "LLVM will burn you". We have to live by the semantics we get.

You're sort of contraticting yourself. A program following the intended semantics is a correct program. Linting about difference actual vs intended semantics and telling the user how to bridge the gap is exactly the job of clippy.

The official, unstable, way to handle this problem is the -Zinsert-sideeffect flag that jonas-schievink mentioned.
The stable way to handle this is to volatile read a stack variable, like I said.

Empty loops are an essential part of any embedded application (and quite a few other applications, too); telling a user to use a weird unrelated unsafe magic or go unstable is not a solution.

Fair enough, but what would be the issue using a nop instead?

Well, because that's not a loop.

This is about preventing the loop from being turned into mush by creating a side effect that forces the compiler to keep it. Of course the nop goes into loop and does not replace it...

Lokathor · 2020-10-13T20:38:56Z

Empty loops are an essential part of any embedded application (and quite a few other applications, too); telling a user to use a weird unrelated unsafe magic or go unstable is not a solution.

Anything else is UB.

And volatile isn't "weird unrelated unsafe magic", every embedded developer needs to understand when to use volatile.

therealprof · 2020-10-13T20:55:01Z

Empty loops are an essential part of any embedded application (and quite a few other applications, too); telling a user to use a weird unrelated unsafe magic or go unstable is not a solution.

Anything else is UB.

It's not supposed/documented to be. And at the moment there's also no lint for it which is what this issue is all about. People are regularly running into problems with miscompilations due to this.

And volatile isn't "weird unrelated unsafe magic", every embedded developer needs to understand when to use volatile.

The matter of fact is a lot of people don't know it and they shouldn't have to (if we did our job right and abstracted all of that away which we do!). Requiring people to know things like that is a substantial hazard to Rust adoption and requiring such a magic block of unsafe code in every application is just ridiculous.

josephlr · 2020-10-13T21:28:01Z

Looking at all the comments it seems like we will need to cover multiple use-cases including no_std applications and no_std libraries. I also agree with the above comments that our advice should make sense even if we didn't have this LLVM bug. This seems reasonable, as a hot CPU deadloop is almost always bad style.

Here would be my plan:

Remove references to spin_loop_hint, as it doesn't (yet) do the right thing for all targets.
- Once core is fixed to make spin_loop_hint have a side-effect on all platforms, we recommend that.
Update the no_std prompt to say something like:
```
empty `loop {}` detected. You may want to either use `panic!()`
or add a platform-specific yield intrinsic to the loop body.
```
- This handles the no_std library case (where libraries should panic instead of deadlooping)
- This also handles the no_std application case (where a binary should do something in it's panic handler other than deadlooping).
Update the docs to mention various remediations:
- Mention that most deadloops outside of the panic handler should panic instead of deadlooping
- Call out the LLVM bug
- Calling some platform intrinsic
- Using -Zinsert-sideeffect
- spin_loop_hint

jyn514 · 2020-10-13T21:47:28Z

a hot CPU deadloop is almost always bad style

This is exactly why I think this should not treat this as if the LLVM bug does not exist. Linting on loop {} because it burns up the CPU is performance issue; linting on it because it causes a miscompilation is a correctness issue. If we lump them together I think people will just ignore the lint.

josephlr · 2020-10-13T21:51:42Z

Linting on loop {} because it burns up the CPU is performance issue; linting on it because it causes a miscompilation is a correctness issue.

That makes sense. Are you suggesting changing the lint category from "Style" to "Correctness"?

Lokathor · 2020-10-13T21:51:58Z

@therealprof

It's not supposed/documented to be.

But it is how things are. There's an open bug about it. No amount of "it shouldn't be that way" will change the facts.

Sometimes Rust just isn't good yet and you have to wait for it to improve. If you want this fixed, help patch LLVM to allow side-effect free loops, or help move insert-sideeffect to be on by default and stable.

Clippy should give accurate information about the current state of the compiler. If the compiler improves later then the lint can be updated later to match the improvement.

@josephlr

Once core is fixed to make spin_loop_hint have a side-effect on all platforms, we recommend that.

Is there even an issue about this ever happening? Because I don't think this will otherwise happen.

josephlr · 2020-10-13T21:55:35Z

Is there even an issue about this ever happening? Because I don't think this will otherwise happen.

I'm working on a patch for libcore right now.

therealprof · 2020-10-13T22:01:04Z

But it is how things are. There's an open bug about it. No amount of "it shouldn't be that way" will change the facts.

Not sure where you got this notion from. We're having this discussion on the clippy repository because we want to tell people about the facts and how to remedy them (in a sane way).

or help move insert-sideeffect to be on by default and stable.

I am not convinced that this is a solution, at best it's a workaround that has the potential to pessimize generated code even further.

Clippy should give accurate information about the current state of the compiler. If the compiler improves later then the lint can be updated later to match the improvement.

I fully agree.

jyn514 · 2020-10-13T22:05:40Z

Are you suggesting changing the lint category from "Style" to "Correctness"?

Yes, I am.

flip1995 · 2020-10-13T22:40:40Z

After reading all the comments in this thread and most of the comments in rust-lang/rust#28728 I conclude:

The lint should be correctness, since loop {} can produce UB. We should also state this in the lint error message, because:

Clippy should give accurate information about the current state of the compiler. If the compiler improves later then the lint can be updated later to match the improvement.
It should also lint for no_std crates
Suggestion is kind of hard, since there is no good solution, that works on all platforms. So we could do the following:
a. Mention every ~~solution~~ workaround in the lint message (meh)
b. Try to detect the platform and suggest the best workaround (how?, which?)
c. Just point to the documentation and write up a summary of the workarounds there

3.a. seems to be too much (useless) information in the lint message.
3.b. seems hard to do. @josephlr suggestion

Update the no_std prompt to say something like:
empty `loop {}` detected. You may want to either use `panic!()`
or add a platform-specific yield intrinsic to the loop body.
This handles the no_std library case (where libraries should panic instead of deadlooping)

This also handles the no_std application case (where a binary should do something in it's panic handler other than deadlooping).

may work for no_std, but I wouldn't know what to do and would have to read some documentation anyway.

So I would go with 3.c. What workarounds should we suggest? (please add to the list, if I forgot something)

Use -Zinsert-sideeffect. From reading LLVM loop optimization can make safe programs crash rust#28728, this seems to me to be the correct solution for this, but requires unstable features, which may not be possible for some people
Use some volatile read operation. In LLVM loop optimization can make safe programs crash rust#28728 it was suggested to add unsafe {llvm_asm!("" :::: "volatile")}, which prevents this bug, but again needs the unstable feature llvm_asm. The read_volatile() produces unnecessary code though (including an allocation).
Calling some platform intrinsic

I'm also ok with that, with a little more explanation and best case with an example. Is there such a thing for all platforms?
spin_loop_hint and similar functions, with the hint, that they may not work on all platforms.

jyn514 · 2020-10-13T22:45:04Z

spin_loop_hint and similar functions, with the hint, that they may not work on all platforms.

These won't work on any platform, they don't involve forward progress or side effects. @josephlr suggested changing them so that they emit some sort of intrinsic to LLVM, but there's no guarentee that change is going to be accepted; personally, it seems like solving this in the wrong place to me.

jyn514 · 2020-10-13T22:46:48Z

Calling some platform intrinsic

I'm also dubious this will work, precisely because the bug is in LLVM and not the platform.

I think suggesting -Z insert-sideeffect on nightly and read_volatile elsewhere is unfortunately the best clippy can do currently.

flip1995 · 2020-10-16T14:59:45Z

@bors retry

bors · 2020-10-16T14:59:57Z

⌛ Testing commit 012413d with merge a280d3b...

Enable empty_loop for no_std crates. Fixes #6161 This change: - Makes it so the `empty_loop` lint will fire for `no_std` crates, but with a `no_std`-specific message (mentioning [`spin_loop_hint`](https://doc.rust-lang.org/core/sync/atomic/fn..html)). - Updates the `std` message to also mention [`std::thread::yield_now`](https://doc.rust-lang.org/std/thread/fn.yield_now.html). - Updates the docs to also mention the [LLVM infinite loop bug](rust-lang/rust#28728) - Updates the tests/stderr files to test this change. changelog: Update `empty_loop` lint to work with `no_std` crates

bors · 2020-10-16T15:05:05Z

💔 Test failed - checks-action_test

flip1995 · 2020-10-17T16:21:50Z

@bors retry (please?)

bors · 2020-10-17T16:21:57Z

⌛ Testing commit 012413d with merge 53a59e5...

Enable empty_loop for no_std crates. Fixes #6161 This change: - Makes it so the `empty_loop` lint will fire for `no_std` crates, but with a `no_std`-specific message (mentioning [`spin_loop_hint`](https://doc.rust-lang.org/core/sync/atomic/fn..html)). - Updates the `std` message to also mention [`std::thread::yield_now`](https://doc.rust-lang.org/std/thread/fn.yield_now.html). - Updates the docs to also mention the [LLVM infinite loop bug](rust-lang/rust#28728) - Updates the tests/stderr files to test this change. changelog: Update `empty_loop` lint to work with `no_std` crates

bors · 2020-10-17T17:51:57Z

💥 Test timed out

jyn514 · 2020-10-18T07:24:27Z

So, after all that, rust-lang/rust#77972 just landed. Maybe we no longer need to sound the alarm on this?

ebroto · 2020-10-18T07:41:45Z

IMO we could keep the more detailed documentation regarding the case of burning CPU, but I think the lint should go back to warn-by-default and ignore no_std

bors · 2020-10-19T11:21:49Z

⌛ Testing commit 012413d with merge 6c4257a...

Enable empty_loop for no_std crates. Fixes #6161 This change: - Makes it so the `empty_loop` lint will fire for `no_std` crates, but with a `no_std`-specific message (mentioning [`spin_loop_hint`](https://doc.rust-lang.org/core/sync/atomic/fn..html)). - Updates the `std` message to also mention [`std::thread::yield_now`](https://doc.rust-lang.org/std/thread/fn.yield_now.html). - Updates the docs to also mention the [LLVM infinite loop bug](rust-lang/rust#28728) - Updates the tests/stderr files to test this change. changelog: Update `empty_loop` lint to work with `no_std` crates

jyn514 · 2020-10-19T12:29:25Z

In that case someone should r- before bors merges 😆

bors · 2020-10-19T12:51:49Z

💥 Test timed out

ebroto · 2020-10-19T13:24:20Z

@bors r-

(Not sure what triggered that last build 🤔)

We also update the documentation to note that the remediations are different for `std` and `no_std` crates. Signed-off-by: Joe Richey <[email protected]>

josephlr · 2020-10-23T05:11:36Z

IMO we could keep the more detailed documentation regarding the case of burning CPU, but I think the lint should go back to warn-by-default and ignore no_std

@ebroto @jyn514 I've updated this PR to do just this. But I've added a TODO to address #6161. I think we would ideally have this still trigger for no_std crates (as it's still bad style, in general). If we could somehow avoid firing this lint for #[panic_handler], we could enable it everywhere else.

But this sort of discussion can be continued in #6161.

EDIT: It looks like adding this #[panic_handler] check is fairly straightforward. I've got an implementation up at #6205. Let me know if you want to merge that PR into this one.

flip1995 · 2020-10-25T09:46:18Z

@bors r+

Thanks!

bors · 2020-10-25T09:46:19Z

📌 Commit 3807634 has been approved by flip1995

bors · 2020-10-25T09:56:21Z

⌛ Testing commit 3807634 with merge fd62c18...

bors · 2020-10-25T10:16:20Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing fd62c18 to master...

Enable empty_loop lint for no_std crates Depends on #6162. Fixes #6161 We skip the lint if the `loop {}` is in the `#[panic_handler]` as the main recommendation we give is to panic, which obviously isn't possible in a panic handler. changelog: Enable empty_loop lint for no_std crates

rust-highfive assigned phansch Oct 11, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 11, 2020

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 18, 2020

clippy_lints: Update empty_loop lint

3807634

We also update the documentation to note that the remediations are different for `std` and `no_std` crates. Signed-off-by: Joe Richey <[email protected]>

josephlr changed the title ~~Enable empty_loop for no_std crates.~~ Update empty_loop documentation/message. Oct 23, 2020

josephlr force-pushed the empty-loop-no-std branch from 012413d to 3807634 Compare October 23, 2020 05:07

This was referenced Oct 23, 2020

Lint: enable empty_loop for no_std crates #6161

Closed

Enable empty_loop lint for no_std crates #6205

Merged

bors merged commit fd62c18 into rust-lang:master Oct 25, 2020

josephlr deleted the empty-loop-no-std branch October 26, 2020 10:10

Update empty_loop documentation/message. #6162

Update empty_loop documentation/message. #6162

Conversation

josephlr commented Oct 11, 2020 • edited Loading

rust-highfive commented Oct 11, 2020

phansch commented Oct 12, 2020

Lokathor commented Oct 12, 2020

jyn514 commented Oct 12, 2020

Lokathor commented Oct 12, 2020

jyn514 commented Oct 12, 2020

ebroto commented Oct 13, 2020

jyn514 commented Oct 13, 2020

ebroto commented Oct 13, 2020

Lokathor commented Oct 13, 2020

jonas-schievink commented Oct 13, 2020

therealprof commented Oct 13, 2020

Lokathor commented Oct 13, 2020

therealprof commented Oct 13, 2020

Lokathor commented Oct 13, 2020 • edited Loading

therealprof commented Oct 13, 2020

Lokathor commented Oct 13, 2020 • edited Loading

therealprof commented Oct 13, 2020 • edited Loading

josephlr commented Oct 13, 2020

jyn514 commented Oct 13, 2020 • edited Loading

josephlr commented Oct 13, 2020

Lokathor commented Oct 13, 2020

josephlr commented Oct 13, 2020

therealprof commented Oct 13, 2020 • edited Loading

jyn514 commented Oct 13, 2020 • edited Loading

flip1995 commented Oct 13, 2020

jyn514 commented Oct 13, 2020 • edited Loading

jyn514 commented Oct 13, 2020

flip1995 commented Oct 16, 2020

bors commented Oct 16, 2020

bors commented Oct 16, 2020

flip1995 commented Oct 17, 2020

bors commented Oct 17, 2020

bors commented Oct 17, 2020

jyn514 commented Oct 18, 2020

ebroto commented Oct 18, 2020

bors commented Oct 19, 2020

jyn514 commented Oct 19, 2020

bors commented Oct 19, 2020

ebroto commented Oct 19, 2020

josephlr commented Oct 23, 2020 • edited Loading

flip1995 commented Oct 25, 2020

bors commented Oct 25, 2020

bors commented Oct 25, 2020

bors commented Oct 25, 2020

josephlr commented Oct 11, 2020 •

edited

Loading

Lokathor commented Oct 13, 2020 •

edited

Loading

Lokathor commented Oct 13, 2020 •

edited

Loading

therealprof commented Oct 13, 2020 •

edited

Loading

jyn514 commented Oct 13, 2020 •

edited

Loading

therealprof commented Oct 13, 2020 •

edited

Loading

jyn514 commented Oct 13, 2020 •

edited

Loading

jyn514 commented Oct 13, 2020 •

edited

Loading

josephlr commented Oct 23, 2020 •

edited

Loading