Skip to content

Always link pre-built asm, required for new cache management functions #246

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

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

adamgreig
Copy link
Member

In #234 we added new __enable_icache and __enable_dcache assembly routines, but without providing an inline assembly version, to reduce duplication and since there wasn't an expected performance impact. However, our build.rs currently only links the pre-built object if the inline-asm feature is disabled, which means currently you can't call enable_icache() and use inline-asm at the same time.

This PR makes us always link against the pre-built objects (for thumb targets) even if inline-asm is used; the pre-built object would only be used for the cache management routines at present but we may want to put more routines into the assembly blob only in the future.

Closes #245.

@adamgreig adamgreig requested a review from a team as a code owner July 16, 2020 11:46
@rust-highfive
Copy link

r? @jonas-schievink

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

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Jul 16, 2020
@jonas-schievink
Copy link
Contributor

What was the original motivation for not linking them when inline-asm is turned off? Are there any impacts to binary size or something?

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

bors r+

@diondokter
Copy link
Contributor

Compiled based on the commit and it works for me.

@adamgreig
Copy link
Member Author

What was the original motivation for not linking them when inline-asm is turned off? Are there any impacts to binary size or something?

Everything in them was previously implemented with inline asm, so there wasn't any need to have them. LTO seems to remove the unused functions fine.

@bors
Copy link
Contributor

bors bot commented Jul 16, 2020

Build succeeded:

@bors bors bot merged commit d91e843 into master Jul 16, 2020
@bors bors bot deleted the always-asm branch July 16, 2020 11:59
adamgreig pushed a commit that referenced this pull request Jan 12, 2022
246: Fix doctests and run them in CI r=therealprof a=jonas-schievink

This moves them from the macros crate to the base crate, avoiding a "weird cyclic dev-dependency". It also makes all of them buildable, and does that in CI.

Co-authored-by: Jonas Schievink <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable_icache gives linker error
5 participants