New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Merging the avr-rust fork upstream #44052

Open
dylanmckay opened this Issue Aug 23, 2017 · 26 comments

Comments

Projects
None yet
9 participants
@dylanmckay
Copy link
Contributor

dylanmckay commented Aug 23, 2017

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

@dylanmckay dylanmckay referenced this issue Aug 23, 2017

Open

Merge changes into upstream Rust #29

1 of 2 tasks complete
@Restioson

This comment has been minimized.

Copy link

Restioson commented Aug 23, 2017

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

@dylanmckay

This comment has been minimized.

Copy link
Contributor Author

dylanmckay commented Aug 23, 2017

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

This comment has been minimized.

Copy link

Restioson commented Aug 23, 2017

Almost though 😄. Just a couple to go

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 23, 2017

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

This comment has been minimized.

Copy link
Contributor Author

dylanmckay commented Aug 23, 2017

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

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 23, 2017

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!

@dsvensson dsvensson referenced this issue Aug 25, 2017

Open

AVR support #3

3 of 8 tasks complete
@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Sep 8, 2017

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

This comment has been minimized.

Copy link
Member

shepmaster commented Sep 11, 2017

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

This comment has been minimized.

Copy link
Contributor

est31 commented Sep 11, 2017

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

This comment has been minimized.

Copy link
Member

shepmaster commented Sep 11, 2017

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

This comment has been minimized.

Copy link
Member

shepmaster commented Sep 11, 2017

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

This comment has been minimized.

Copy link
Contributor

est31 commented Sep 11, 2017

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

This comment has been minimized.

Copy link
Contributor Author

dylanmckay commented Sep 11, 2017

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

This comment has been minimized.

Copy link
Contributor

mattico commented Sep 29, 2017

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

This comment has been minimized.

Copy link
Member

shepmaster commented Sep 29, 2017

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

This comment has been minimized.

Copy link
Contributor Author

dylanmckay commented Sep 30, 2017

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

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Sep 30, 2017

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

@mattico

This comment has been minimized.

Copy link
Contributor

mattico commented Sep 30, 2017

@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 😄

@chocol4te

This comment has been minimized.

Copy link

chocol4te commented Jan 16, 2018

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

@Restioson

This comment has been minimized.

Copy link

Restioson commented Jan 16, 2018

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

This comment has been minimized.

Copy link
Contributor Author

dylanmckay commented Jan 17, 2018

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

This comment has been minimized.

Copy link
Contributor

kellerkindt commented Feb 11, 2018

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

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Feb 11, 2018

@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

This comment has been minimized.

Copy link

Jimmio92 commented Aug 14, 2018

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

This comment has been minimized.

Copy link
Contributor Author

dylanmckay commented Dec 18, 2018

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:

  • The AVR backend initialization functions LLVMInitializeAVR{Target,TargetInfo,AsmPrinter,AsmParser, ...} are declared and called.
  • An base-minimum avr target specification avr-unknown-unknown has been added. This models avr-gcc's default behavior of building for the lowest common denominator unless explicitly specified. Unlike avr-gcc which explicitly supports the -mmcu=<avr mcu name> argument, no AVR-specific command line argument has been added for AVR. This means that a custom target specification JSON file must be written for every project. This is the case for many Rust embedded projects though.
  • In the fork, the AVR LLVM backend is always compiled and linked in the default Rust checkout and added to the config.toml.example file. Should AVR be included in upstream Rust by default, or should it also be opt-in?
  • AVR specific logic added to compiler where required for all new targets
  • 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.
  • Support for a conditional compilation on CPU, a la #[cfg(target_cpu = "...")] has been added. The implementation can be found here. The implementation of this is target-independent, and thus it works for conditional compilation based on CPU for other architectures too, such as ARM. This allows the ruduino crate to conditionally include a device-specific module that exposes all of the IOs, registers, and modules that are supported in the silicon. This most definitely needs to go through the RFC process before upstream.

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

This comment has been minimized.

Copy link
Contributor Author

dylanmckay commented Dec 18, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment