Drop AT&T syntax from inline asm

[thalesfragoso]

This was breaking compilation on thumbv6 targets and miscompiling on thumbv7.

Fixes #272
Fixes #267

[rust-highfive]

r? @jonas-schievink

(rust_highfive has picked a reviewer for you, use r? to override)

[rust-highfive]

⚠️ Warning ⚠️

• Pull requests are usually filed against the master branch for this repo, but this one is against v0.6.x. Please double check that you specified the right target!

[jonas-schievink]

bors r+

[thalesfragoso]

Hmm, aligned depends on as-slice that depends on generic-array that uses MaybeUninit, which breaks our MSRV.

[bors]

Build failed:

• continuous-integration/travis-ci/push

[jonas-schievink]

Just bump it, master doesn't use those anymore

[thalesfragoso]

Just bump it, master doesn't use those anymore

We can't release 0.6.4 if we do that. I will try to get rid of aligned, should be easy™.

[jonas-schievink]

That's a breaking change, since we use it in public APIs. Why can't we bump the MSRV?

[thalesfragoso]

//! # Minimum Supported Rust Version (MSRV)
//!
//! This crate is guaranteed to compile on stable Rust 1.31 and up. It might
//! compile with older versions but that may change in any new patch release.

But you are right about removing aligned, hmmm

[jonas-schievink]

I don't think the intention was to make that part of our semver guarantees. We cannot give more guarantees than our dependencies there.

[adamgreig]

At least in principle we do require a semver bump for MSRV changes, which would prevent releasing 0.6.4 with an MSRV above 1.31.

Edit: I am aware we've talked about this before, e.g. rust-embedded/wg#449 (comment), and have at various times wanted to change that policy.

Can we require an older version of aligned?

[thalesfragoso]

Can we require an older version of aligned?

Wouldn't that also be breaking ?

[adamgreig]

Yes, I suppose it would. Shame for us that nothing in the aligned -> generic_array -> as_slice link made a semver bump when increasing their MSRV, but probably points to our current policy being untenable in general. Sounds like bumping the MSRV for 0.6.4 is probably the least bad option?

[thalesfragoso]

Sounds like bumping the MSRV for 0.6.4 is probably the least bad option?

I think so, should I change the version in the docs or just remove it entirely ? I think the later option is probably better, since we don't control all of our dependencies.

[adamgreig]

Throwing away the MSRV entirely seems a bit more drastic for a bug-fix patch release, even if it might become wrong if our dependencies increase it again. What MSRV will we need for this release?

[thalesfragoso]

Throwing away the MSRV entirely seems a bit more drastic for a bug-fix patch release, even if it might become wrong if our dependencies increase it again. What MSRV will we need for this release?

1.36. I think it doesn't make much sense to say that this crate is guaranteed to compile with some specific version if we can't guarantee that...

[therealprof]

If our dependencies bumped the MSRV without following semver best practises, doesn't that automatically mean they already made the decision for us (unless we want to pin a specific version of that dependency)?

[thalesfragoso]

Well, the aligned crate doesn't have a MSRV to start with, we can't blame them. The problem was that we added a crate with an incompatible MSRV. It seems to be a bit hard to include MSRV in semver, unless you control all of your dependencies.

[adamgreig]

If our dependencies bumped the MSRV without following semver best practises, doesn't that automatically mean they already made the decision for us (unless we want to pin a specific version of that dependency)?

I don't think it's well established what the semver "best practices" even are around MSRV, or at least I couldn't find anything when we last worried about this. It's possible we both have sensible but incompatible policies.

[therealprof]

Well, I was thinking about the general semver best practises (unrelated to Rust) but actually they really only seem to bother about API surface and not other factors like MSRV so that point is moot.

[adamgreig]

To get this fix in, shall we bump our reported MSRV for now (and for this 0.6.4 release) to 1.36, leave it in the documentation, and we can spend some more time considering what to do about MSRV generally afterwards? If a user still needs a lower MSRV, at least nothing in our crate stops it; they could always specify an earlier version of aligned in their own Cargo.toml and which is still compatible with cortex-m and should allow the build to continue on earlier rustc.

[thalesfragoso]

To get this fix in, shall we bump our reported MSRV for now (and for this 0.6.4 release) to 1.36, leave it in the documentation, and we can spend some more time considering what to do about MSRV generally afterwards? If a user still needs a lower MSRV, at least nothing in our crate stops it; they could always specify an earlier version of aligned in their own Cargo.toml and which is still compatible with cortex-m and should allow the build to continue on earlier rustc.

I don't feel very comfortable leaving the MSRV notice as it's now for 0.6.4, maybe change the wording ? We can bring it back on 0.7, given that we can control our dependencies (we already dropped aligned) or at least ask them for setting a MSRV. That is, if we decide on going forward with having a MSRV aware semver policy.

[thalesfragoso]

Ok, I don't think this fix should be blocked by this, so I updated the MSRV and left things as it was. I can change more things if it's required, leaving this decision to you folks.

[thalesfragoso]

bors try

[bors]

try

Build succeeded:

• continuous-integration/travis-ci/push

[therealprof]

LGTM, too.

bors r+

[bors]

Build succeeded:

• continuous-integration/travis-ci/push

[adamgreig]

I don't feel very comfortable leaving the MSRV notice as it's now for 0.6.4, maybe change the wording ? We can bring it back on 0.7, given that we can control our dependencies (we already dropped aligned) or at least ask them for setting a MSRV. That is, if we decide on going forward with having a MSRV aware semver policy.

Sorry, I didn't mean to leave this sitting. I still think it makes sense to get 0.6.4 out now, but I think overall you're right about the MSRV, as this example clearly illustrates. I still think there's value in stating an MSRV at least so that we're aware of when we change it and can consider if a PR merits a large bump, especially for a crate like cortex-m which is going to be quite widely used in embedded. Maybe it doesn't make sense for bumping the MSRV to be a semver breaking change, though, and maybe we will need to be more careful of dependencies too.