Import cortex-m-semihosting and panic-semihosting into this repo

[jonas-schievink]

Motivation:

• Allows writing QEMU tests for cortex-m's functionality that use semihosting to control QEMU. Previously these crates would pull in another cortex-m version, which doesn't work. Now they have a path dependency on the root crate.
• Lets us share the outline-inline-assembly setup and cargo-xtask in general.
• Lets us share CI and bot setup between more crates.
• 2 fewer repos to triage and keep track of (I'll transfer their issues after this is merged).

I also want to import cortex-m-rt, but I'll do that in a later PR.

CI was updated to build-test all crates with all or most feature combinations, like it did before.

[rust-highfive]

r? @adamgreig

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

[therealprof]

I am still very much not a fan of this and would like to put this up for a vote.

[adamgreig]

PR looks fine content-wise. @therealprof, I know you weren't keen on having semihosting be part of cortex-m itself, but what's the issue with having the crates live inside this repo? We save on management and overhead, but it doesn't change anything for users. Is the objection just to having any semihosting support at all?

[therealprof]

I think it doesn't feel right from an architectural standpoint to depend on the semihosting crate and a panic handler just for CI purposes.

Moving them into this repository blesses the semihosting approach and highlights it in a weird way.

I'd rather we only move the syscall into cortex-m and use it directly.

[adamgreig]

How else would you test inside qemu?

[therealprof]

How else would you test inside qemu?

Not even sure what we want to test in cortex-m that has no other choice than to use semihosting. Even this PR doesn't seem to actually add any semihosting dependency for testing purposes (despite saying so in the motivation) so it's hard to tell how to eliminate such a dependency.

[jonas-schievink]

Semihosting is, to my knowledge, the only way to terminate QEMU with a success error code, and the only clean way to terminate it with an error code (the non-clean way is to cause a lockup). It is also the simplest way to print to stdout, which is useful for doing snapshot testing outside of QEMU (I'm not aware of any others that work on all targets we support).

I did not add any tests to this PR because I wanted to land the basic prerequisites first (and tests are quite distinct from what this PR aims to do), but if even that ends up being so controversial I'm not sure if my time isn't better spent elsewhere.

Moving them into this repository blesses the semihosting approach and highlights it in a weird way.

I think it is now highlighted much less than before, when it had its own dedicated repo and issue tracker.

[therealprof]

Semihosting is, to my knowledge, the only way to terminate QEMU with a success error code, and the only clean way to terminate it with an error code (the non-clean way is to cause a lockup).

I don't see why we couldn't just use the syscall functionality to that effect.

I did not add any tests to this PR because I wanted to land the basic prerequisites first (and tests are quite distinct from what this PR aims to do), but if even that ends up being so controversial I'm not sure if my time isn't better spent elsewhere.

Yeah, sorry about that (although my criticism shouldn't at all come as a big surprise). I'm all for bringing in test scaffolding and low level primitives for testing, I am still not conviced this is the only or right approach to do this.

Moving them into this repository blesses the semihosting approach and highlights it in a weird way.
I think it is now highlighted much less than before, when it had its own dedicated repo and issue tracker.

I respectfully disagree that c-m-sh sharing the repository and issue tracker with the central Rust Cortex-M crate is a demotion.

[adamgreig]

I don't see why we couldn't just use the syscall functionality to that effect.

That's what this does, essentially; it's providing the wrapper code around syscall that allows it to be used to communicate with qemu (or a debug probe). Semihosting's an official ARM platform standard, so I think it makes perfect sense to support it inside cortex-m.

I respectfully disagree that c-m-sh sharing the repository and issue tracker with the central Rust Cortex-M crate is a demotion.

What issues do you think this will cause compared to having them separate, either for us or for users? I know you don't like people using semihosting, not least because it often causes confusion when code doesn't run without a debugger present, but organisationally this move seems to save us work and effort and remove duplication of CI/test infra, without any impact on users that I'm aware of.

[thalesfragoso]

I'm in favor of this change. I don't think this is putting semihosting at a spotlight or something, the people looking at our CI probably already know its disadvantages and alternatives.

If we wish to discourage beginners to use semihosting, our time would be better spent removing it from examples, which is arguably already happening and we have a lot of other tools in the spotlight now.

Also, I don't understand why we should go out of our way to look for possible alternatives when the work is already done and it matches our requirements.

Anyway, this is my opinion on the matter, thanks for your work Jonas.

[therealprof]

I don't see why we couldn't just use the syscall functionality to that effect.

That's what this does, essentially; it's providing the wrapper code around syscall that allows it to be used to communicate with qemu (or a debug probe).

The wrapper and panic handler go way beyond providing lowlevel primitives which is what this repo should focus on IMHO. The only motiviation mentioned to break out of this idea is to enable CI which this PR doesn't even do.

At the very least I'd like to see how this is going to be used to maybe come up with a possible alternative.

Semihosting's an official ARM platform standard, so I think it makes perfect sense to support it inside cortex-m.

For me cortex-m is lowest level primitives and should remain at that level, this PR provides a higher level abstraction. There're tons of other ARM platform standards too, if we add higher level semihosting support those should have a rigt to be added as well, the very first thing coming to mind here are other panic-handler implementations (e.g. panic-itm), so panic-semihosting doesn't create the impression of being a blessed cocept.

What issues do you think this will cause compared to having them separate, either for us or for users?

I do think it makes it harder for us to discourage the use of semihosting if we cannot keep the distance ourselves and accept bug reports for it in our central platform support repository.

but organisationally this move seems to save us work and effort and remove duplication of CI/test infra

While this might save us an unquantified bit of duplicate work it also increases the risk of CI breakage due to some semihosting related breakage. All of this stuff is very low-level and somewhat fragile to compiler changes and whatnot; increasing the complexity is not going to help here.

[adamgreig]

The wrapper and panic handler go way beyond providing lowlevel primitives which is what this repo should focus on IMHO. The only motiviation mentioned to break out of this idea is to enable CI which this PR doesn't even do.

We already provide iprintln and other ITM-related functionality here; hprintln and related machinery are at the same level. Once these changes land, it becomes possible for the first time to add some CI tests which actually execute cortex-m code on an emulated thumb platform.

the very first thing coming to mind here are other panic-handler implementations (e.g. panic-itm)

Especially given as ITM stuff is already in cortex-m, I'd be entirely in favour of putting panic-itm inside cortex-m. It makes it even easier for cortex-m users to specify a provided panic handler. We could even put panic-abort inside and thus reduce the dependency count of many apps one further. I see cortex-m not as the "lowest possible level" of abstractions - that would be a sort of "cortex-m-pac" which doesn't exist - but as a comprehensive library of functionality for the platform. We already provide a lot of high level utilities, and I don't see the advantage to splitting the others out into separate crates, either to us as maintainers or to users.

While this might save us an unquantified bit of duplicate work it also increases the risk of CI breakage due to some semihosting related breakage. All of this stuff is very low-level and somewhat fragile to compiler changes and whatnot; increasing the complexity is not going to help here.

We already support cortex-m-semihosting as an official working group crate and repository, so any such breakage would just happen there instead and still need to be dealt with. Importing it here seems like a net win.

[therealprof]

Sorry, completely forgot about this still being open.

I could find solace by moving a variety panic handlers into cortex-m. However are they all Cortex-M only?

There would be a few more points I could bring to the table re maintenance and (undesirably) bringing a spotlight to semihosting but if we integrate all sorts of panic handlers I could live with the maintenance complications and my main concern would be resolved.

[adamgreig]

I could find solace by moving a variety panic handlers into cortex-m. However are they all Cortex-M only?

They're all so simple that honestly just having a copy in cortex-m doesn't seem like a problem. It provides a useful feature to users of the crate and makes quickly adopting some sort of default panic handler easier.

I'd propose in addition to semihosting we include itm (using existing iprintln), abort/halt (infinite loop), and perhaps fault (jump to hardfault). Could those be a new PR so we can get this one moving?

[therealprof]

I'd propose in addition to semihosting we include itm (using existing iprintln), abort/halt (infinite loop), and perhaps fault (jump to hardfault). Could those be a new PR so we can get this one moving?

Fine. Who's going to do all the maintainance work and how do we handle the republishing of the crates with the correct new repository URL?

[adamgreig]

Ok, here's a proposal based on the meeting yesterday:

• Update links as suggested in this review
• Fix merge errors by rebuilding precompiled libs
• Merge this PR
• Apply https://github.com/rust-embedded/cortex-m-semihosting/pull/41/files in a new PR
• Check for breakage and update cortex-m dependency in cortex-m-semihosting and panic-semihosting to allow cortex-m <0.8 instead of <0.7
• Release cortex-m-semihosting 0.4.0 with those changes, publishing the new links and ensuring compatibility with the new cortex-m
• Release panic-semihosting 0.5.4, same deal
• Mark the two old repositories as archived
• Merge https://github.com/rust-embedded/panic-itm into this repository as well
• Update links, bump cortex-m dependency to allow 0.7.0, release v0.4.2
• Mark panic-itm repository as archived
• Release cortex-m 0.7

We could also consider:

• Apply formatting from Code formatting cortex-m-semihosting#54 but to all of cortex-m; this is a large but non-breaking change that we could probably land before 0.7 and might want to do before 1.0.

[adamgreig]

Suggested change

	repository = "https://github.com/rust-embedded/panic-semihosting"
	repository = "https://github.com/rust-embedded/cortex-m"

[adamgreig]

Suggested change

	repository = "https://github.com/rust-embedded/cortex-m-semihosting"
	repository = "https://github.com/rust-embedded/cortex-m"

[adamgreig]

Can we just delete this file? It already exists in the repository root. And the LICENCE files too in fact. Same in the panic-semihosting directory.

[adamgreig]

If we do delete the redundant licence files, these links would need updating to point to the file in the directory above, as far as I can tell that should still work even on crates.io though.

[adamgreig]

This link would also need updating (and in panic-semihosting's README).

[adamgreig]

@jonas-schievink any thoughts on my review comments? If we can get those ticked off and rebuild the binary files I think we're clear to merge this and start on the rest of the checklist.

[adamgreig]

bors merge

[bors]

Build succeeded:

• continuous-integration/travis-ci/push