Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

[RFC] remove build dependency on arm-none-eabi-gcc (binary blob alternative) #95

Merged
merged 8 commits into from
Aug 26, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Aug 25, 2018

Before this commit we used gcc to assemble external assembly files into object
files that we linked into our Rust program.

This commit drops the dependency on gcc by shipping the already assembled object
files with this crate source code.


This is an alternative to RFC #91 that doesn't require a breaking change or
adding a new Cargo feature and can be implemented right now.

See #91 for the rationale of dropping the dependency on gcc.

This approach can be applied to other Cortex-M crates like cortex-m-semihosting
and cortex-m (would subsume RFC rust-embedded/cortex-m#107).

This seems like an overall better approach to me, but before I go opening more
PRs I want to hear your thoughts, @rust-embedded/cortex-m

closes #91

Before this commit we used gcc to assemble external assembly files into object
files that we linked into our Rust program.

This commit drops the dependency on gcc by shipping the already assembled object
files with this crate source code.
@japaric japaric requested a review from a team as a code owner August 25, 2018 13:27
@japaric japaric added needs-decision RFC This issue needs your input! labels Aug 25, 2018
@therealprof
Copy link
Contributor

Very nice, I like it!

@korken89
Copy link
Contributor

I agree, this looks good!

@adamgreig
Copy link
Member

Nice idea! I think this is a great fix to get custom asm in for Rust 2018 without requiring end users have gcc installed, but I'm not sure it's the best long term solution compared to somehow having Rust generate these files. It does seem better than removing functionality (ExceptionFrame in this case), and would easily support #77. It's a shame it presumably won't help the performance hit of non-inline cortex-m::asm having to make a function call for each intrinsic, but does let us use any instructions we want which is a big win.

Maybe we could have a gcc feature that causes the asm files to be rebuilt on crate compilation, rather than using the prebuilt images? That way users who do have gcc installed could opt to automatically build the asm themselves instead, the same way it used to work. The only advantage I can think of for doing this is if you're actually editing those same files in a local cortex-m-rt checkout , but it still seems nicer...

we no longer need to test clang because this crate never invokes a compiler /
assembler
@japaric
Copy link
Member Author

japaric commented Aug 25, 2018

I'm not sure it's the best long term solution

IMO, the best long term solution is to have asm! and global_asm! in stable Rust. Both this approach and the external assembly files approach break when using custom targets. The cc crate decides which compiler and compiler flags to use based on the target name but custom targets can have any name. And this approach only supports a fixed number of built-in targets -- this mean we'll have to do work to support newer targets like ARMv8-M. *asm! has none of this problems; in the long run we should move to stable *asm!.

The only advantage I can think of for doing this is if you're actually editing those same files in a local cortex-m-rt checkout

I'm not too keen on adding a public Cargo feature that's only useful while developing (and not in all the cases). Mainly because of the maintenance effort.

@adamgreig
Copy link
Member

IMO, the best long term solution is to have asm! and global_asm! in stable Rust.

👍

I'm not too keen on adding a public Cargo feature that's only useful while developing (and not in all the cases).

That's fair. I'm still a little uneasy shipping binary images as part of a foundational crate with no easy way to say "please build these from the source actually". But it's not a show-stopper and this way is better than the alternatives.

Is it possible to ensure the built binaries are up-to-date before cargo publish bundles them up? Are the builds reproducible / could we have Travis rebuild the .a files and check they match whatever's in Git, to prevent updated .s files being committed (or worse, published) alongside outdated .a files?

crawford
crawford previously approved these changes Aug 25, 2018
@japaric
Copy link
Member Author

japaric commented Aug 25, 2018

Is it possible to ensure the built binaries are up-to-date before cargo publish bundles them up?

@adamgreig done. See https://travis-ci.org/rust-embedded/cortex-m-rt/jobs/420519538#L541

@thejpster
Copy link
Contributor

Putting binaries in the source tree goes against all my principles, but actually, it may be the least bad option here. Provided we have those shell scripts for rebuilding, I don't see the need to bake it into the Cargo.toml because it's not something people will do often.

@japaric
Copy link
Member Author

japaric commented Aug 25, 2018

Current vote count:

Let's give @ithinuel some time to review proposal.

@therealprof
Copy link
Contributor

@thejpster I agree that binaries are awkward, however since we ship the source and it's trivial to build yourself it's not too bad in this particular case.

bors bot added a commit that referenced this pull request Aug 25, 2018
96: reduce the size of default handlers r=adamgreig a=japaric

this commit replaces the two default handler (DefaultDefaultHandler and
DefaultUserHardFault) by a single handler named `EndlessLoop`. This results in
one less symbol being generated.

Also this new handler uses `compiler_fence` to avoid the "infinite loops w/o
side effects are undef values" bug in LLVM. `compiler_fence` is guaranteed to
generate no code so this reduces the size of the handler (when compiler in
release).

---

As far as I could test this new handler doesn't generate abort instructions when
optimized so it seems like a safe replacement. If we are feeling paranoid then
once #95 we could implement EndlessLoop in assembly.

r? @rust-embedded/cortex-m (anyone)

Co-authored-by: Jorge Aparicio <jorge@japaric.io>
@japaric
Copy link
Member Author

japaric commented Aug 26, 2018

This has a majority of approvals and no remaining concerns so I'm going to land this and open follow PRs to use this approach on the cortex-m and cortex-m-semihosting crates. Those PRs don't need to undergo voting because the approach will be the same.

bors r+

bors bot added a commit that referenced this pull request Aug 26, 2018
95: [RFC] remove build dependency on arm-none-eabi-gcc (binary blob alternative) r=japaric a=japaric

Before this commit we used gcc to assemble external assembly files into object
files that we linked into our Rust program.

This commit drops the dependency on gcc by shipping the already assembled object
files with this crate source code.

---

This is an alternative to RFC #91 that doesn't require a breaking change or
adding a new Cargo feature and can be implemented right now.

See #91 for the rationale of dropping the dependency on gcc.

This approach can be applied to other Cortex-M crates like cortex-m-semihosting
and cortex-m (would subsume RFC rust-embedded/cortex-m#107).

This seems like an overall better approach to me, but before I go opening more
PRs I want to hear your thoughts, @rust-embedded/cortex-m

closes #91

Co-authored-by: Jorge Aparicio <jorge@japaric.io>
@japaric
Copy link
Member Author

japaric commented Aug 26, 2018

@dvc94ch you may want to use this trick in the riscv crates. Basically it gives you stable global_asm! w/o relying on gcc.

@bors
Copy link
Contributor

bors bot commented Aug 26, 2018

Build succeeded

@bors bors bot merged commit dd0c08a into master Aug 26, 2018
@bors bors bot deleted the no-gcc branch August 26, 2018 21:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
RFC This issue needs your input!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] [breaking-change] don't depend on GCC by default
6 participants