Fix downstream errors due to inline assembly

[hannobraun]

Even though this crate is intended for ARM Cortex-M microcontrollers,
its code must be compilable on other platforms, for example when running
unit tests in a downstream crate.

This could lead to compile or link errors, due to inline assembly that
was invalid on the host platform. Those errors wouldn't show up when
compiling this crate, as inline assembly within functions marked as
#[inline(always)] bypasses validation, unless the function is used.1

This commit fixes the issue, by only compiling inline assembly when
running on ARM. In some cases, some replacement code had to be added, to
make the compiler happy.

[hannobraun]

@japaric Have you had a chance to take a look at this? I'm currently forced to override cortex-m in my downstream library, as I wouldn't be able to have a CI build otherwise.

[hannobraun]

@japaric I hate to be a bother, but have you had a chance to look into this? Since this is a non-breaking change, I'd love to get this merged (and ideally released) before #65 breaks everything downstream.

[pftbest]

I also had some problems with tests recently caused by inline assembly, so I like this change

[japaric]

Thanks for the PR, @hannobraun.

I'm not 100% sure this is the layer where this "problem" should be "fixed".

For starters I don't think these functions are a problem at all. If you are testing your code on
x86_64 (or some other non-ARM architecture) then, IMO, you should be testing your application logic,
not the IO functionality (which doesn't exist on that architecture). IOW, you should not be
running these functions on x86_64 during your testing.

Now, perhaps you are testing some other function that uses these low level functions that directly
map to the hardware registers. In that case it may seem appealing to try to replace the function at
the lowest layer to be no-ops then you have "fixed once and for all", right? I think that's a
mistake because then you are ignoring the semantics of these functions. For example if the function
you are testing busy waits until some (core) register reaches some value then that function will
turn into an infinite loop when you execute it on x86_64. What you should do in that case is replace
(cfg) the call to the low level function for some code that's semantically equivalent on x86_64
(e.g. thread::sleep), or perhaps that piece of code is not relevant to the unit test and can be
omitted. In any case, the point here is that the person writing the unit test should adjust the
calls to these low level function to achieve the desired semantics; it should not be the case that
the lower layer dictates bogus semantics -- that's (likely) going to result in problems during
testing.

Or, perhaps the problem here is that your unit tests are not using these functions at all but
you are still hitting compiler / linker errors. In that case we should find some solution other than
stubbing the function as done here (i.e. reads return 0). In that case, I think the most sensible
replacement for the implementations would be unimplemented!. That forces the unit test writer to
think of a (semantically equivalent) replacement while writing the tests, or to think of a unit
testing design that doesn't do any I/O to begin with.

[hannobraun]

Thank you for your reply, @japaric. I am not calling these functions from my test code. That would be wrong for all the reasons you outlined. The compiler/linker errors are triggered by simply compiling the code on x86_64, as might be necessary when trying to run cargo test for unrelated code in the same crate. I'm sorry for not being more clear about that.

To be more clear about what's happening, I think if everything was working correctly, cortex-m itself should not compile on anything but ARM, as the ARM-specific assembly should trigger compiler errors. It does compile, however, because the ARM assembly is not validated because the functions are #[inline(always)] (see rust-lang/rust#36718). This is the only reason you can run cargo test for cortex-m without errors.

As soon as a crate depending on cortex-m contains calls to one of the functions that contain ARM assembly, running cargo test for that crate will no longer work on non-ARM platforms, as the compiler will no longer ignore the ARM assembly.

So to summarize, this is definitely a problem in cortex-m, and it needs to be fixed in cortex-m. The only reason this isn't extremely obvious, is because the problem is masked by rust-lang/rust#36718.

However, I do agree with you that unimplemented! is a much better choice for fixing this problem. I will update the pull request accordingly. Please note that I was following the example laid out by existing code when I decided to stub the functions (see interrupt.rs). I will update these functions as well.

[japaric]

I will update the pull request accordingly.

Thanks. While you are at it could you also update the cfg-d functions that are not using unimplemented! to also use unimplemented!?

[hannobraun]

Yes, no problem.

I noticed you added the v0.4.0 milestone. Are you in any hurry there? If so, let me know. I'm already in holiday mode and won't be back in the office until early January, but I can sneak this in without too much trouble, if you want to release before then.

[japaric]

Well, I'd love to get the ecosystem bump done sooner than later since it seems that more interested people are popping out right now (probably due to the holidays) but I'm also (supposed to be) in holiday mode so I can only make so much progress on a daily basis. So every bit of help is welcome!

[hannobraun]

Great work over the last few days! Nice to see this much progress. Sorry I couldn't be more helpful so far.