search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.4k stars 12.73k forks source link

RFC: Merging the avr-rust fork upstream #44052

Closed dylanmckay closed 4 years ago

dylanmckay commented 7 years ago

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/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 commented 7 years ago

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

dylanmckay commented 7 years ago

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 commented 7 years ago

Almost though 😄. Just a couple to go

alexcrichton commented 7 years ago

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:

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Something to remember for merging, need to update the c_int type used in the main() signature: https://github.com/rust-lang/rust/pull/44906#discussion_r141843808

Edit: opened an issue for this since it affects MSP430 as well: https://github.com/rust-lang/rust/issues/44934

shepmaster commented 7 years ago

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 commented 7 years ago

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

shepmaster commented 7 years ago

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

mattico commented 7 years ago

@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 commented 6 years ago

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

Restioson commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@dylanmckay LLVM6 has been merged https://github.com/rust-lang/rust/pull/47828 - what does that mean to this RFC?

shepmaster commented 6 years ago

@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 commented 6 years ago

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 commented 5 years ago

So, good news!

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.

This is now the case!

The current avr-rust fork does not contain any modifications to libcore.

The modifications required to support AVR from stock Rus are:

It's probably about time I send an RFC to LLVM-dev regarding promotion of the backend to non-experimental status.

You can see the full set of changes from upstream Rust to avr-rust here.

dylanmckay commented 5 years ago

There's still a couple LLVM patches from the last two months we've cherry-picked at the moment, but the upstream Rust efforts to separate the emscripten version of LLVM from the version of LLVM used for all other targets has made it really easy to bring these changes in from rust-lang/llvm repo, as it is now updated upstream regularly.

The <4 cherry-picked LLVM patches we have are currently in review in upstream LLVM, and so once a reviewer finds enough time, those patches will automatically float into upstream Rust. Upstream Rust also has target-specific Rust-specific patches, and so cherry-picked LLVM patches probably isn't really a blocker for merging avr-rust upstream

NewbiZ commented 5 years ago

Any update on the status of Rust on AVR?

edvorg commented 5 years ago

Also interested to know! For now I swtiched to hacking on STM32 blue pill, but I'd definitely want to get back to arduino once support for avr in rust is ready.

flosse commented 5 years ago

We @slowtec would also love to use Rust for our AVR projects and of course we would open source a lot of our production code :)

jonahbron commented 5 years ago

Bump, would love to see support made official. Gonna try using the fork for a project.

dylanmckay commented 5 years ago

Update: At the moment, we're upgrading the fork to a newer version of Rust (avr-rust/rust#137). There are two bugs we've found that we've hit.

LLVM ERROR: ran out of registers during register allocation

This has been fixed in LLVM trunk in llvm/llvm-project@45eb4c7e55341c0b83a21dedecc092e273795eda. I'm cherry-picking this into our LLVM fork now. This bug was historically the biggest pain point in the backend, arising in most pointer-heavy code.

LLVM ERROR: Cannot select: t2: i16 = addrspacecast[1 -> 0] undef:i16 t1: i16 = undef In function: ZN4core3ptr87$LT$impl$u20$core..fmt..Debug$u20$for$u20$unsafe$u20$fn$LP$A$RP$$u20$.$GT$$u20$Ret$GT$3fmt17h0fdf8ca7140def9b

This one is harder to fix. It is caused by Rust mishandling function pointers on Harvard architectures where there are two separate pointer address spaces for accessing main memory and program memory.

The explanation is best made by @ecstatic-morse

The underlying issue is that const T always has addrspace(0). I think explicit casts (ptr as const T) should preserve the address space of their input here.

The bug is exposed by the std::fmt::Debug implementation for functions and function pointers. Rust emits a pointer dereference with a target pointer type of addrspace(0)* i16, using LLVM routines that will implicitly insert a bitcast (iN -> iM bits) or an address space cast if necessary. In the case of function pointers, Rust should codegen a addrspace(1)* i16 pointer, so that LLVM doesn't have to map (addrspacecast) PROGMEM pointers to RAM pointers an impossible task because there is no memory mapping and the address spaces do not overlap.

This bug is the main blocker.

I am hoping upstream Rust pulls from LLVM master (AFAICT, it's pretty much at the 8.0 release) so we can remove a bunch of cherry-picks.

I recently got the https://github.com/dylanmckay/avr-compiler-integration-tests/ tests succeeding using an AVR emulator, which is great because there is no other test suite that actually executes the AVR assembly spit out by LLVM. I set up a GitLab runner to run tests for every AVR-Rust commit (via a GitLab repository mirror), but it isn't super useful because GitLab doesn't support CI builds on pull requests, as the actual code is hosted in a forked repository.

rubberduck203 commented 5 years ago

Thanks for the update @dylanmckay. We all appreciate the effort you’ve put into this.

dylanmckay commented 5 years ago

We've now upgraded the fork ontop of Rust base rust-lang/rust@fb7cca33f, the blink program successfully compiles.

likeabbas commented 5 years ago

Did this issue stall?

H2CO3 commented 5 years ago

Do you have any updates on this? Is the PR still stalled on the address space issue?

edvorg commented 5 years ago

Hey @dylanmckay , sorry for bugging with this.. is there any update on the issue?

dylanmckay commented 4 years ago

Hello all, here's a few comments from me

Cherry-picking fixes

Almost all of the required fixed to get libcore working have been upstreamed to LLVM master and currently exist in the rust-lang/llvm fork. Yesterday I started a PR to update the AVR fork to 1.39.0 (avr-rust/rust#155), I only had to cherry-pick one fix that was not already there - avr-rust/llvm@4350776601bc671e6e055bfbe32add34b70d2635.

libcore cannot be compiled without modification

We no longer require a custom libcore fork to use AVR. There is currently only one modification to libcore in the avr-rust fork - avr-rust/rust@44240ac59c5949b8a9fd191f5cd666d0206fbe85 - rewrite a pointer cast to get the right IR generating.

We are dependent on xargo for compiling AVR-Rust binaries - this will become easier with the std-aware Cargo initiatives going on (including the working group). Currently, AVR requires a compiled sysroot for each MCU type - we use Xargo at the moment to compile this lazily, but it will be much more seamless when Cargo becomes able to natively compile core/std for desired target specifications.

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

This has been fixed.

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

This is still a painful issue. I suspect the best fix will be to write custom lowering code in the LLVM AVR backend that lowers these shifts to pure assembly, removing any ABI dependency on compiler-rt or libgcc. I am unsure how much larger the generated code size could be, it may not be a good idea.

dylanmckay commented 4 years ago

Questions before upstream

RFCs and unstable calling conventions

I posted this comment up the thread

The "avr-interrupt" calling convention has been added. This allows extern "avr-interrupt" fn my_isr_handler() { .. }. This would probably need to go through the RFC process to become stabilized, but I could be wrong.

Can unstable calling conventions be added without going through the RFC process?

Currently, we have the "avr-interrupt" and "avr-non-blocking-interrupt" under the #![feature(abi_avr_interrupt)] feature gate. Could these be upstreamed as unstable calling conventions, pending a future stabilization RFC?

Buildbots

Upstreaming an LLVM backend requires setting up a dedicated buildbot that runs tests for that backend, and maintaining it. Is this the case with Rust? How do we ensure that the test suite runs in AVR mode on every push? What did other backends (like WASM) do?

Distribution

Does "Merging the avr-rust fork upstream" mean simply merging the two forks into one, but still requiring a build-from-source? Perhaps it's possible for a backend to be distributed, but nightly-only? What did the other backends do?

Other than that, the avr-rust specific patches are very thin, without any gnarly hacks nowadays.

The full patchset can be seen here https://github.com/rust-lang/rust/compare/master...avr-rust:avr-support

My WIP 1.39.0 rebase patchset (which is mostly identical) can be found here https://github.com/rust-lang/rust/compare/master...avr-rust:avr-support-1.39.0-4560ea788c. This should be merged to avr-rust/master in the next few days.

I can't think of anything specific that's blocking this - perhaps, it's time to submit patches and see how it goes?

dylanmckay commented 4 years ago

https://github.com/rust-lang/rust/issues/44036

I mentioned this issue above, but I don't think it blocks upstreaming of the AVR backend. We should be able to land a working, useful version of rustc with AVR without it. I am sure there are workarounds and hacks we can do re. device detection in AVR crates in the meantime after the backend has been hypothetically merged, and before an official target-cpu querying API lands in official Rust.

jonahbron commented 4 years ago

He lives! Thanks for the update @dylanmckay , exciting progress.

FrankvdStam commented 4 years ago

Great work @dylanmckay! Thanks for keeping us informed.

edvorg commented 4 years ago

A bit off-topic, but: would rustc for avr be able to do FFI with C libraries? There are plenty of mature libraries already available for avr but written in C/C++. It would be great to be able to create some rust-style wrappers for them and reuse them in our rust avr projects.

H2CO3 commented 4 years ago

Rust already can do FFI with C in general.

edvorg commented 4 years ago

Yes and that's really great! The question is though: does it translate to rust-avr?

CryZe commented 4 years ago

As long as LLVM's C ABI for AVR matches that of gcc, it should work

shepmaster commented 4 years ago

Can unstable calling conventions be added without going through the RFC process?

My vote is yes, as the change to support the AVR calling conventions is basically just giving them a name and mapping them to the LLVM number. There's no fundamental code changes needed as we already have support for unstable conventions.

Upstreaming an LLVM backend requires setting up a dedicated buildbot that runs tests for that backend, and maintaining it. Is this the case with Rust? How do we ensure that the test suite runs in AVR mode on every push? What did other backends (like WASM) do?

Rust's tier system is a bit loosely defined, but I think that the right thing is to merge the AVR-specific code. Then AVR will be tier 3, which means it is not automatically built or tested, just hanging out in the code. At the very least, this means that those files will be kept up-to-date with compiler changes.

Does "Merging the avr-rust fork upstream" mean simply merging the two forks into one, but still requiring a build-from-source?

To my knowledge, it would be to submit the one commit as a PR. Our custom fork would effectively die / become a place to track AVR-specific details until it becomes a more widely-used target.

perhaps, it's time to submit patches

I think so.

dylanmckay commented 4 years ago

@edvorg very on topic IMO

A bit off-topic, but: would rustc for avr be able to do FFI with C libraries? There are plenty of mature libraries already available for avr but written in C/C++.

Yes.. mostly. The AVR calling convention implemented by the AVR backend is designed to match AVR-GCC's (documented here). There are some quirks, but the LLVM patch D68524 I need to review should fix them.

Rust callsites will always be able to correctly invoke Rust or avr-clang compiled functions (as avr-clang C uses the same extern "C" calling convention implementation as the Rust frontend), and inter operation with AVR-GCC should work, especially for simple function signatures, but it can choke at times (see the description of D68524 for more details).

luqasz commented 4 years ago

Any update on this ? Just curious.

dylanmckay commented 4 years ago

Submitted request to make LLVM AVR backend an official backend - https://reviews.llvm.org/D75099

Foo42 commented 4 years ago

@dylanmckay If accepted, what are the remaining pieces to closing this issue?