Workaround for loop {} until it is fixed

[korken89]

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

[japaric]

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

[korken89]

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

[therealprof]

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]

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

[therealprof]

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

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]

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]

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

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]

It is a known issue for a few years, you can read about it here: https://github.com/rust-lang/rust/issues/28728

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

[therealprof]

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

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

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]

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

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

[korken89]

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

[therealprof]

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

[korken89]

Yeah, exactly.

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

[therealprof]

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

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]

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.