Update empty_loop documentation/message.

[josephlr]

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]

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]

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]

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]

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]

You can.

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

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

[jyn514]

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]

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]

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]

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]

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]

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

[therealprof]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

@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]

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]

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]

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

Yes, I am.

[flip1995]

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

1. 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.

2. It should also lint for no_std crates

3. 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]

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]

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]

@bors retry

[bors]

⌛ Testing commit 012413d with merge a280d3b...

[bors]

💔 Test failed - checks-action_test

[flip1995]

@bors retry (please?)

[bors]

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

[bors]

💥 Test timed out

[jyn514]

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

[ebroto]

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]

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

[jyn514]

In that case someone should r- before bors merges 😆

[bors]

💥 Test timed out

[ebroto]

@bors r-

(Not sure what triggered that last build 🤔)

[josephlr]

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]

@bors r+

Thanks!

[bors]

📌 Commit 3807634 has been approved by flip1995

[bors]

⌛ Testing commit 3807634 with merge fd62c18...

[bors]

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