Skip to content
This repository was archived by the owner on Jul 29, 2025. It is now read-only.

Workaround for loop {} until it is fixed #59

Merged
merged 1 commit into from
Nov 20, 2018
Merged

Conversation

korken89
Copy link
Contributor

Fixes #50 but is only a workaround until it is fixed in the compiler

@korken89 korken89 requested a review from a team November 18, 2018 14:05
@japaric
Copy link
Member

japaric commented Nov 18, 2018

I would prefer to use asm::nop in user-facing code; I think it would be less confusing.

@korken89
Copy link
Contributor Author

@japaric No problem, changed to asm::nop.

@therealprof
Copy link
Contributor

One thing I've been doing in my crates (to avoid clippy warnings about an empty loop) is to place a continue statement inside. Not sure it'll fix this problem but it would be less overhead.

@korken89
Copy link
Contributor Author

I tried it, but using only continue still generates the udf #254 instruction in release mode.

@therealprof
Copy link
Contributor

@korken89 On which arch is that? I can't reproduce the problem on armv6m:

With my usual code I get:

 80001d6:       e7fe            b.n     80001d6 <main+0x116>

With the cortex_m::asm::nop() this changes to:

 80001d6:       f000 f8ca       bl      800036e <__nop>
 80001da:       e7fc            b.n     80001d6 <main+0x116>

@therealprof
Copy link
Contributor

I'm curious what the original workaround looked like, if anything I'd prefer a wfi() instead of a nop() in there due to a nop() being maximum wasteful on resources.

@adamgreig
Copy link
Member

I think a wfi is not a great idea as it breaks debugging unless you also have regular interrupts going off; nop is only wasteful insomuch as it doesn't put the chip to sleep. For a quickstart template I think nop is a lot less surprising and confusing for new users.

@korken89
Copy link
Contributor Author

@therealprof I was using latest nightly and thumbv7em-none-eabihf.

The idea is simply that after filling out the template "It should just work™", which it does not do in some cases now - resulting in a CPU crash.

@therealprof
Copy link
Contributor

The idea is simply that after filling out the template "It should just work™", which it does not do in some cases now - resulting in a CPU crash.

I agree it should work out-of-the-box but I can't reproduce the failure with my own applications. Maybe the problem is specific to the empty main function of the quickstart example and not the loop itself?

@korken89
Copy link
Contributor Author

korken89 commented Nov 19, 2018

It is a known issue for a few years, you can read about it here: rust-lang/rust#28728

I can fix a small example when I'm back home :)

@therealprof
Copy link
Contributor

therealprof commented Nov 19, 2018

@korken89 I've read the issue. I'm still interested in actually reproducing the problem myself because I haven't seen it so far and if it really is a problem I'll have to deploy a fix in quite a few crates and create new releases.

So I'm looking forward to your example. ;)

@korken89
Copy link
Contributor Author

@therealprof, here you can download and run cargo build --release to see the issue: https://github.com/korken89/loop_crash
I also included the dump in the dmp file, look for the udf instruction, it causes the crash.

@korken89
Copy link
Contributor Author

korken89 commented Nov 19, 2018

I also had to use a similar trick in panic_halt that might be of interest, it has no overhead: https://github.com/korken89/panic-halt/blob/master/src/lib.rs

@therealprof
Copy link
Contributor

@korken89 I think that is a totally different issue: The compiler is assuming that since the loop is irrelevant it can just optimise it away, so it will. And since main is empty rustc will happily inline it into the Reset function and since it is marked as does not ever return, udf is basically the only thing left over.

As soon as #[entry] does something useful this will not happen anymore. You can easily achieve the same by placing the cortex_m::asm::nop() before the loop (instead of into it):

  00000400 <main>:
   400:   f000 f82e       bl      460 <__nop>
   404:   e7fe            b.n     404 <main+0x4>
   406:   d4d4            bmi.n   3b2 <__INTERRUPTS+0x372>

Can you try whether

#[entry]
fn main() -> ! {
    cortex_m::asm::nop();
    loop {
        continue;
    }
}

works for you?

@korken89
Copy link
Contributor Author

@therealprof I updated the loop_crash repo with your recommendation, but I still get the udf instruction.

@korken89
Copy link
Contributor Author

Ah sorry, no I read the wrong function. Having side effect in main is enough to not get the udf instruction.

@therealprof
Copy link
Contributor

@korken89 You should always get the udf instruction but it shouldn't be reachable.

@korken89
Copy link
Contributor Author

Yeah, exactly.

Back to the issue at hand, should we move this PR forward or go with a different approach?

@therealprof
Copy link
Contributor

@korken89 I'd prefer to put the nop() in front of the loop with a comment saying, that it should be replaced by the real code. And I'd like to see the continue in the loop to get rid of the clippy lint.

If we put other side-effects into the loop people are likely to copy it for no good reason. (Not that it really mattered but it's not exactly beautiful either).

@korken89
Copy link
Contributor Author

I agree on moving the nop, pushed that now.
However, I will leave the continue out, as this PR is only to make it work while keeping it minimal.
It will still run with the clippy warning, and it is a good hint for the user in where to add code. :)

@therealprof
Copy link
Contributor

I see what you did there, you're expecting the user code to end up in the loop. That kind of assumes you're not starting your #[entry] function with an if to get access to the peripherals but use unwrap() instead. Because if you do, the signature will not be -> ! anymore, so you have to put an empty loop at the end.

I think it is more idiomatic to suggest something something like:

#[entry]
fn main() -> ! {
    if let Some(_cp) = cortex_m::peripheral::Peripherals::take() {
        // Put your initialisation code here

        // Replace this nop with the code you want to run in main, perhaps using a loop
        cortex_m::asm::nop();
    }

    loop {
        continue;
    }
}

I'm still going to blanket approve your version because it is better than what we have at the moment.

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM

@korken89 korken89 merged commit 207bca3 into master Nov 20, 2018
@korken89 korken89 deleted the loop_crash_fix branch November 20, 2018 14:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release build crashes
4 participants