RFC: Merging the avr-rust fork upstream

[dylanmckay]

Hello all,

I would like to know the general opinions on merging the avr-rust fork into Rust proper.

The fork itself has became a lot more stable with less bugs in the recent months. It has also started attracting a number of people interested in using it.

You can find one of the more interesting projects using avr-rust on GitHub.

Blockers

LLVM 5.0

Rust is currently on LLVM 4.0, which contains a working AVR backend, but there have been many bugfixes since then. We would need to wait for LLVM 5.0 to be supported (nearly finished: #43370) before we get a version of the AVR backend that has a few important bugs fixed.

This is no longer a blocker. Upstream Rust is at LLVM 6.0 as of 2018-02-20.

Questions

Cherry-picking fixes

If AVR support was built into mainline, we'd need to be able to cherry-pick patches into Rust's LLVM fork. I don't imagine this would be much of a problem, as we already cherry-pick a number of important fixes into there.

All of the bugfixes cherry-picked into the avr-rust repository have already been upstreamed into LLVM trunk, which would continue to be the case if we merged the fork, as I am not a fan of the LLVM fork diverging too far from trunk.

Cherry-picking is necessary because of LLVM's 6-month release cycle.

Current issues in the AVR backend

There aren't any known bugs in the avr-rust/rust repository - all of the known bugs are issues in the AVR LLVM backend; here are some of the more interesting/impactful ones.

libcore cannot be compiled without modification

There is a milestone set up to track what bugs need to be fixed in order to get libcore compiling successfully without modification.

This hasn't been much of a problem for users so far, as xargo will transparently compile libcore on the fly when needed, and we can override libcore in Xargo.toml.

I am unsure what the Rust team thinks of merging a target which can't use the stock libcore.

Any operations on function pointers other than 'call' access RAM, not program memory (avr-rust#68)

This is a symptom of AVR being the very first in-tree LLVM backend for a Harvard architecture. LLVM currently assumes that all functions reside in "the generic address space", which corresponds to RAM. Because of this, if you attempt to load/store through a function pointer, it will access RAM instead of the program memory.

Good news is that I have pending upstream LLVM patches to fix it (D37052, D37053, D37054, D37057).

32-bit shifts generate calls to a compiler-rt routine that doesn't exist (avr-llvm/llvm#163)

Because there aren't many (if any) targets that don't support 32-bit shifts natively, libgcc and compiler-rt do not have 32-bit versions of shift routine, even though LLVM still happily generates a call to it.

This causes an undefined symbol error whilst linking. This will only happen if you actually write code or use code that performs 32-bit shifts, as the linker is quite good at removing all dead code.

Note that I've had one user hit the missing shift bug due to compiling in release mode, which promoted a multiplication to a shift as an "optimisation".

Actual changes to merge

You can find all AVR-specific differences by looking at this diff.

Note that over half of that diff is just the README and Travis CI configuration - the actual code being upstreamed is very small; just some glue code to enable the AVR backend and a target specification.

This diff also conditionally disables parts of libcore for AVR - these fixes would not upstreamed, and are not strictly required as downstream users can use Xargo to compile a minified libcore for AVR).

Links

AVR-Rust on Gitter
AVR-Rust on GitHub

[Restioson]

+1! Avr rust built into the compiler proper would be super useful. It's almost free of bugs now.

[dylanmckay]

Not completely free of bugs :)

I will update the description to include some information about the state of the backend w.r.t bugs

[Restioson]

Almost though 😄. Just a couple to go

[alexcrichton]

In general we're quite welcoming of platforms upstream in rust-lang/rust so long as they don't place a maintenance burden on us! Some specific thoughts from what you're thinking:

• Cherry-picking LLVM commits every so often is totally OK, I think y'all've gone through the process a few times already as well :)
• Missing intrinsics in compiler-rt is fine by us, you have the option of implementing them in Rust as well through the compiler-builtins project.
• The patch you have looks pretty good, although I think we'd want to work more through the various #[cfg] in libcore. Are these items left out due to "bugs in LLVM" or because they're fundamentally not supported at all on AVR? The former would be best done by "fixing LLVM" somehow whereas the latter makes things more tricky.

In general we currently have all platforms with the same uniform interface of libcore/libstd, but it's not clear that'll remain true into as we keep picking up more platforms.

[dylanmckay]

The patch you have looks pretty good, although I think we'd want to work more through the various #[cfg] in libcore. Are these items left out due to "bugs in LLVM" or because they're fundamentally not supported at all on AVR?

It is because of bugs in LLVM.

The only question in my mind is on support of i128/u128 types - I think that for AVR, these aren't really useful. The i128 support in libcore is currently commented out because of a bug, but there may be other undiscovered bugs as it is not a well tested codepath, and really exercises the register allocator as the AVR only has 32 bytes worth of general purpose registers.

It is still quite likely that we could get i128 working on AVR though, I don't know too much of the specific problems it triggers in the backend.

I think that the best way forward would be to propose the patch for real once libcore does compile without modification, or at least without many.

[alexcrichton]

I think that the best way forward would be to propose the patch for real once libcore does compile without modification, or at least without many.

Sounds reasonable to me!

[est31]

The only question in my mind is on support of i128/u128 types - I think that for AVR, these aren't really useful.

My main fear of not supporting i128 on AVR is that it will have a chilling effect on adoption of 128 bit integers for the sake of compatibility with AVR or other embedded platforms. E.g. if there is a library that uses 128 bit integers, AVR users that will want to use it will file issues to drop the usage. This might make 128 bit integers not "safe" to use in Rust code that strives to be portable, which I wouldn't like to happen.

[shepmaster]

a chilling effect on adoption of 128 bit integers for the sake of compatibility with AVR or other embedded platforms

I don't think that it's a terribly huge deal here. Small embedded devices already have giant limitations (no stdin / stdout, generally no allocator, etc.) that make it real hard to just drop in an arbitrary library. I recently learned that AVR GCC's double is actually an alias for float! I don't know if we are going to keep that strangeness or not, but it would affect crates way more than i128.

I think we are always going to have special features that are used to make a crate suitable for embedded, just like we do for no-std.

[est31]

no stdin / stdout, generally no allocator, etc.

You are describing the #![no_std] ecosystem. There are libraries that target this ecosystem. And the general rule for that ecosystem is to take libcore as a given, which also includes i128. Each target that doesn't support i128 has a larger chilling effect inside this ecosystem, because an embedded target's "market share" is bigger inside the subset of the entire Rust ecosystem where the x86 family is not a very relevant player.

I don't know if we are going to keep that strangeness or not, but it would affect crates way more than i128.

Interesting! I do agree that if we'd alias f64 to f32 (or not provide it), it would affect the ecosystem more. However, if we can strive for consistency, why shouldn't we? It is definitely possible for us to implement i128.

[shepmaster]

It is definitely possible for us to implement i128.

Absolutely, and I realize I didn't clearly state that I think we should implement i128 for AVR. However, any code that actually uses an i128 on an AVR is going to be in for a world of pain.

However, if we can strive for consistency, why shouldn't we?

Consistency with what, is the question. Consistency with GCC (f64 == f32) or with every other Rust target (f64 != f32)?

You are describing the #![no_std] ecosystem.

Yep, which is why I said "special features that are used to make a crate suitable for embedded, just like we do for no-std." 😇

[shepmaster]

A larger problem that's been in the back of my mind since we originally landed the 16-bit usize patch is that, fundamentally, Rust and Rust programmers tend to assume that usize is the "native" size of a register. AFAIK, this is true for all the other platforms Rust targets, but not for AVR.

[est31]

Consistency with what, is the question. Consistency with GCC (f64 == f32) or with every other Rust target (f64 != f32)?

The name f64 literally indicates that it has 64 bits. If you don't abide by the name then it becomes meaningless just like it is in C.

[dylanmckay]

Good points here, I can see the concern around 128-bit integers. I definitely think that they should be supported, even though we shouldn't encourage their usage. I would hate to see crates having to add feature flags for things like trait impls on i128 types. This shouldn't really be an issue because all of the unused i128 stuff should be trimmed out by the linker.

The f32/64 problem is an interesting one. My main concern with making f64 actually 64-bits is that it means that C FFI could be very brittle. If developers don't know that AVR-GCC uses 32-bit doubles, then calls through FFI could read uninitialised memory or segfault.

I imagine we could solve this more-or-less by expecting users to use types from the libc crate instead. We could add AVR-specific functionality to set c_double to f32. I think we can reasonably expect people to use the libc crate in their FFI bindings.

[mattico]

Something to remember for merging, need to update the c_int type used in the main() signature: #44906 (comment)

Edit: opened an issue for this since it affects MSP430 as well: #44934

[shepmaster]

used in the main() signature

@mattico Maybe I've just been doing things in a weird way, but none of my code has made use of Rust's main:

#[no_mangle]
pub extern fn main() {

More importantly, we can't really return because there's nothing to return to. Every program needs to run forever.

[dylanmckay]

@mattico We will still definitely have to modify libc so the types match GCC for AVR

[shepmaster]

Oh, absolutely, I just don't know that main will impact us at all.

[mattico]

@shepmaster Even on non-embedded platforms, the size of argc in main doesn't matter given that we've had it wrong all this time and nobody has noticed except when inspecting the IR. There may be some RTOS that uses a standard main() entrypoint for its processes? Regardless, it's easy enough to fix.

It probably would've taken the same amount of time to submit a fix as it did to type this out, now that I think about it 😄

[fmckeogh]

Is it just the libcore issues preventing this merge? Just so we know where to concentrate efforts.

[Restioson]

The only issues I've had out of libcore are weird linker errors caused by I don't know what and also 32 bit bitshifting errors (missing intrinsic I think). I don't know if those are blocking the merge though.

[dylanmckay]

chocol4te: Is it just the libcore issues preventing this merge? Just so we know where to concentrate efforts.

Yes - all of the required work here needs to be done within LLVM.

Restioson: The only issues I've had out of libcore are weird linker errors caused by I don't know what and also 32 bit bitshifting errors (missing intrinsic I think).

That's because all of the code that makes the AVR backend choke is commented out :)

Restioson: I don't know if those are blocking the merge though.

Not directly, but it'd be good to have fixed before the backend is merged. There are a couple of really annoying bugs like this that we should consider fixing before we merge, but they may not necessarily hold it up.

[kellerkindt]

@dylanmckay LLVM6 has been merged #47828 - what does that mean to this RFC?

[shepmaster]

@kellerkindt all of the issues listed in "Current issues in the AVR backend" are still true. It's likely that the current HEAD of avr-rust could be rebased and the interesting Rust-specific code merged, but that still doesn't get working code.

I'm personally still in favor of

I think that the best way forward would be to propose the patch for real once libcore does compile without modification, or at least without many.

Although having to avoid extra rebasing is nice.

[Jimmio92]

I wonder the current state of Rust on AVR, now that we're half of a year later in development. I run a little Arduino projects group in my town, and I would love to be able to use Rust instead.

[dylanmckay]

The fork has been merged! I will write a proper update this afternoon

#69478

[wezm]

I will write a proper update this afternoon

I am interested in said update. Where do you plan to post it @dylanmckay (so I know where to look 🙂)?

[nixpulvis]

I've been eagerly awaiting this addition to the Rust ecosystem for literally years now! My naive attempts to use this on master seem flawed however.

I built master rust with the standard ./x.py build, linked as toolchain master, then attempted to build the avr-rust/blink example (after updating src/main.rs to use llvm_asm!):

RUST_TARGET_PATH=`pwd`
XARGO_RUST_SRC=/home/nixpulvis/Projects/rust

rustup run master xargo build --target avr-atmega328p --release

This fails with:

error: no matching package named `core` found
location searched: registry `https://github.com/rust-lang/crates.io-index`
required by package `sysroot v0.0.0 (/tmp/xargo.oXlxlujoXvXJ)`
error: `"cargo" "build" "--release" "--manifest-path" "/tmp/xargo.oXlxlujoXvXJ/Cargo.toml" "--target" "avr-atmega328p" "-p" "core"` failed with exit code: Some(101)
note: run with `RUST_BACKTRACE=1` for a backtrace

I'm assuming there's configuration still required which I'm missing.

[shepmaster]

@nixpulvis see #44052 (comment)

I wouldn’t expect much of anything to work now as libcore is part of what is miscompiled.

[dylanmckay]

The AVR rustc fork has been merged upstream.

The upstream fork is not usable in general yet. There are a few things that need to happen first.

AVR LLVM upstream fixes that Rust doesn't have yet need to be cherry-picked

To answer @nikic's question, there are a maximum of 20 LLVM commits that need to be cherry-picked.

I have been using a script to automatically cherry-pick all AVR commits from LLVM upstream into a local fork.
This is a large hammer and there will be cherry-picked fixes that are unnecessary - there are upstream LLVM fixes that need to be pulled to upstream Rust before the backend becomes usable. they will need to be filtered out and cherry-picked to upstream Rust.

Compiling the AVR LED blink program from the upstream Rust master branch

As the upstream effort has been going on for a few months now, the downstream avr-rust fork
is still on a particularly old version of Rust. Since then there has been at least one change in upstream Rust that requires generalization to support AVR without hitting LLVM assertion errors. I've opened pull request #73270 to fix it.

I am not sure if there have been other changes in upstream Rust which need changes for AVR - likely not.

Once I've gotten the blink program working, I will post another update because at that point, the upstream AVR support should be ready for use/experimentation.

Where should AVR communication occur/be posted

@wezm you raise a good point - there isn't necessarily a "blessed" communication channel for updates. This ticket has served well, but it will be inevitably closed soon.

The AVR-Rust Gitter channel is the most natural existing choice. I like the idea of a mailing list (but I don't like the idea of hosting a mail server if possible). The Gitter channel does have discussion in it so perhaps something like a mailing list would be helpful.
Perhaps setting up a blog would be good to centralize announcements/updates. I would like to have an "official medium" so that people who want casual updates don't miss out. I suppose a blog requires constant checking for new posts, whereas a mailing list inherently notifies
everybody who is interested. Suggestions welcome.

Open questions

• Should GitHub issues on the avr-rust fork be moved to the upstream Rust repository?
  ** Regardless, new issues should be raised on the upstream repository - the old issue tracker will need to be wound down.
• Where should the instructions for compiling Rust with AVR live?

The most important stuff for the future (read: priorities)

• Packaging and distribution of AVR Rust binaries. Compiling Rust can be a big headache to the casual user,
  many distros seem to have issues from time to time in certain workflows. Several OOM errors misreported as
  compiler bugs. The barrier to entry is unnecessarily high and needs to be lower - just download and go.
  Tracked by Package the compiler and expose on a public internet interface avr-rust/rust-legacy-fork#162
• Building a list of "supported", or at least tested, configurations. We need a table of (rustc version, Xargo version) so
  that changes to private Rust APIs that Xargo depends on do not break the AVR Rust toolchain upon upgrade.
  Xargo is in maintanence mode - cargo-xbuild seems quite limited (I tried it a few weeks ago, I can't remember the reason),
  we may need to fork Xargo. Perhaps better to add the tooling directly into Cargo (there is a tracking issue for this).
• Setting up some kind of homepage for the project, including links to download
• Integration testing (on an emulated/simulated AVR, upon each commit)

For the moment, I will make sure to repost updates into this GitHub issue, until a better medium can be found. I will post them in Gitter too.

[wezm]

Perhaps setting up a blog would be good to centralize announcements/updates. I would like to have an "official medium" so that people who want casual updates don't miss out. I suppose a blog requires constant checking for new posts, whereas a mailing list inherently notifies
everybody who is interested. Suggestions welcome.

My personal preference is strongly in favour of a blog (with an RSS feed). I think blogs typically show up better in search engine results and are nicer to link to than mailing list threads. The RSS feed solves the checking/notification aspect.

[rubberduck203]

I’m not 100% sure it’s the best place for it, but there is the Embedded WG blog. Might be a low effort channel for communication.

https://rust-embedded.github.io/blog/

[aykevl]

Perhaps also a Twitter account? It can be used to share new blog posts (to stay up to date).

[jonas-schievink]

I think the embedded working group would be happy to help here. We have a Twitter account @rustembedded and can certainly include AVR-related news in the newsletter too.

I've also created the O-AVR label a while back, so feel free to use the Rust issue tracker for AVR-specific issues too (but be mindful that there are 1.5k people watching the repo). In addition to that, you might want to coordinate on the rust-lang Zulip, since that's where most of the Rust teams reside. We're also in the process of ramping up "target groups" that focus on specific Rust targets (eg. Windows or Arm), AVR might be a good fit for that as well. Feel free to reach out on Zulip for this.

[dylanmckay]

Update.

There are two things remaining before libcore can be compiled for AVR on a stock rust-lang/master branch:

1. There is one pull request that fixes a series of "address space cast invalid" bugs on AVR, relating how function pointers for Harvard architectures must be tagged as addrspace(1) compared to Von-Neumann targets which are fine with Rust's current default of addrspace(0). [AVR] Correctly set the pointer address space when constructing pointers to functions #73270 - it is currently in code review.

2. There is one other bug blocking the blink example from working - avr-rust#92. There is a not-yet-upstreamed patch which will fix it - https://reviews.llvm.org/D68524. This will be upstreamed and then cherry-picked into Rust's LLVM fork very soon once it passes against the unit and integration tests. it has been merged into upstream LLVM

Once these two things are done, the upstreamed Rust AVR target will be able to compile AVR code to the same level as the current avr-rust/rust fork, and we can then begin the process of updating the blink example, arduino crate, documentation, guides, for the upstream branch. Then the upstream branch should be ready for experimental use.

[dylanmckay]

One more TODO:

3. Add AVR plumbing for the new inline assembly syntax in upstream Rust
   • Once the new inline assembly syntax is supported, the Arduino crate must be updated to use it
     once the Arduino crate has been updated, update the blink crate to the new version. We can use the old macro via llvm_asm! for now as it still exists in nightly.
4. Cherry-pick AVR correctness patches (mostly Ayke's) from LLVM master to upstream Rust LLVM fork (EDIT: PR: [AVR] Cherry-pick 17 upstream AVR backend fixes into the Rust LLVM fork llvm-project#66)

Here's the branch with everything: https://github.com/dylanmckay/rust/commits/dylan/avr-workable-upstream-candidate. This branch is sufficient to compile libcore.

It is built from upstream rust-lang/master but also includes the not-yet upstreamed LLVM patch D68524, not-yet-merged Rust PR #73270, and 46 AVR related commits from upstream LLVM automatically cherry picked via script (LLVM branch: https://github.com/dylanmckay/llvm-project/commits/dylan/avr-workable-upstream-candidate). Note that not all 46 are required, the final list will be smaller.

[dylanmckay]

Update: Opened a pull request that includes all of the upstream LLVM fixes.

Here is an exhaustive list of the remaining work before the LED blink program can be compiled and run successfully.

Remaining work

• Land Rust PR [AVR] Correctly set the pointer address space when constructing pointers to functions #73270. This pull request fixes a series of "address space cast invalid" bugs on AVR, relating how function pointers for Harvard architectures must be tagged as addrspace(1) compared to Von-Neumann targets which are fine with Rust's current default of addrspace(0). It is currently in code review.
• Land Rust LLVM PR [AVR] Cherry-pick 17 upstream AVR backend fixes into the Rust LLVM fork llvm-project#66. This pulls in all of the required AVR fixes from upstream LLVM into Rust's LLVM fork. In the end, there were 17 16 LLVM fixes that needed to be cherry-picked.
• Land Rust PR [AVR] Update the rust-lang/llvm-project submodule to include AVR fixes recently merged #73658 that updates the version of the LLVM submodule. The Rust compiler docs have instructions for doing this. It is currently in code review.
• Land Rust PR [AVR] Disable passing of he '--eh-frame-hdr' argument to the linker #74625 rustc_target: Add a target spec option for disabling --eh-frame-hdr #74631. In 79a42e3 Rust started unconditionally passing the --eh-frame-hdr argument to the linker. The AVR-GCC linker does not support this flag and so a special case to avoid passing it is added for AVR, similar to the existing exclusions for Windows. Solaris, and UEFI.

[nixpulvis]

Perhaps better to add the tooling directly into Cargo (there is a tracking issue for this).

I'm assuming this is the correct issue: rust-lang/cargo#4959.

[shepmaster]

cargo build -Z build-std=core has worked well for my AVR example cases.

[nixpulvis]

@shepmaster that seems to be getting me closer anyway, thanks! I just seem to be stuck on the bitcast stuff now, so I'll wait for that to be merged in (since I appear to be missing a file needed to build the PR, and IDK what I'm doing).

In the process of using -Z build-std=core I needed to supply a target triple, so I ran rustc +master --print target-list | grep avr, and found avr-unknown-unknown. But the archived issue avr-llvm/llvm#35 seems to make it sound like the triple should in fact be avr-atmel-none (which makes more sense to me anyway). Does something need to be updated here, or am I missing something?

[shepmaster]

avr-unknown-unknown is correct.

[PointyFluff]

Open questions

* Should GitHub issues on the avr-rust fork be moved to the upstream Rust repository?
  ** Regardless, new issues should be raised on the upstream repository - the old issue tracker will need to be wound down.

* Where should the instructions for compiling Rust with AVR live?

I don't think this matters too much on the user side. This was easy enough to find through ddg and/or this-week-in-rust.
Whatever makes it easier for the devs.

[trembel]

Open questions

* Should GitHub issues on the avr-rust fork be moved to the upstream Rust repository?
  ** Regardless, new issues should be raised on the upstream repository - the old issue tracker will need to be wound down.

* Where should the instructions for compiling Rust with AVR live?

I don't think this matters too much on the user side. This was easy enough to find through ddg and/or this-week-in-rust.
Whatever makes it easier for the devs.

I think the instructions for compiling Rust with AVR should be somehow in https://docs.rust-embedded.org/

[dylanmckay]

Alright, update time.

All of the required patches for AVR exist in the current Rust nightly compiler as of today's nighty rustc 1.47.0-nightly (0820e54a8 2020-07-23). The Rust nightly compiler, without modification, can now compile the LED blink example successfully and generate an AVR ELF file.

• New, centralized project landing page created at https://avr-rust.com/
• A new book - The AVR-Rust Guidebook has been created, hosted on GitHub pages at book.avr-rust.com.
• The avr-rust/rust fork repository has been deprecated. The repository has not yet been archived as there are existing issues that should be migrated before they are permanently locked and closed.
• Xargo is no longer required - the -Z build-std flag in upstream Rust replaces the need for it on AVR. A cargo fork is no longer required - upstream cargo will do.

The Rust nightly compiler can now be considered the recommended channel for Rust with AVR support.

I am closing this issue now - we did it!

Steps for reporting bugs can be found in the AVR Guidebook.

The AVR Guidebook and the blink example at https://github.com/avr-rust/blink are the best resources to start using the target.

A deep, deep thanks to everybody who discussed and supported the project through this upstreaming effort - it is very appreciated.

FIN

[H2CO3]

Wow wow wow.

Thanks for everyone who contributed to this – I've been looking forward to this day since forever!

[edvorg]

Dylan McKay before porting rust to avr

And after

Thank you for all the hard work man! :-) Take a good rest!