Skip to content
New issue

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

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

Already on GitHub? # to your account

Clarify / Document / Redesign the Peripherals::{take, steal} API #186

Open
jonas-schievink opened this issue Jan 9, 2020 · 7 comments · Fixed by rust-embedded/bare-metal#27
Labels
Milestone

Comments

@jonas-schievink
Copy link
Contributor

In #169, I wanted to add a function SCB::steal that unsafely obtains an instance of the SCB peripheral. While this API isn't really necessary since Peripherals::steal().SCB does the same, it is slightly more convenient.

One issue that came up was that Peripherals::steal() is not documented to have any relationship with Peripherals::take(). In reality, calling steal() will cause take() to return None, and this is relied on by RTFM for soundness! We definitely need to document this, but it might also be useful to have an unsafe steal()-like API that does not affect the "taken flag" at all (this is what my SCB::steal() would've been, for example).

Another possible improvement on this API would be to move it to a trait, which could help with https://github.com/rust-embedded/cortex-m-rt/issues/180. If we had that, we could have cortex-m-rt steal and pass arbitrary implementors of that trait to the #[entry] point of the app, moving one nice pattern enabled by RTFM to the non-RTFM embedded world.

@jonas-schievink
Copy link
Contributor Author

Strawman proposal:

trait StaticResource: Sized {
    /// Obtains ownership of this resource singleton and makes it unavailable to
    /// future callers of `take()`.
    ///
    /// If `take()` or `steal()` have been called before, this returns `None`.
    fn take() -> Option<Self>;
    
    /// Obtains an instance of this resource and makes all future calls to
    /// `take()` return `None`.
    ///
    /// This will not check if `take()` or `steal()` have already been called
    /// before. It is the caller's responsibility to use the returned instance
    /// in a safe way that does not conflict with other instances.
    unsafe fn steal() -> Self;
    
    /// Unsafely obtains an instance of this resource.
    ///
    /// This will not check if `take()` or `steal()` have already been called
    /// before. It is the caller's responsibility to use the returned instance
    /// in a safe way that does not conflict with other instances.
    unsafe fn conjure() -> Self;
}

@jonas-schievink
Copy link
Contributor Author

We could also make it an unsafe trait, and require implementors to ensure that the crate containing the "taken flag" uses a well-known links = "..." value in its manifest. This would make it impossible to link multiple crates (or crate versions) together that provide the same static resource (which would allow unsoundly acquiring multiple instances of the same hardware resource).

@jonas-schievink
Copy link
Contributor Author

Added the trait to bare-metal in rust-embedded/bare-metal#27

@jonas-schievink
Copy link
Contributor Author

rust-embedded/bare-metal#27 has been reverted

@agausmann
Copy link

I was just passing by, but I had a realization that might be helpful - The current definition of steal(), which affects future calls to take(), can be implemented as take().unwrap_or_else(|| conjure()).

Impacts:

Peripherals::steal() is not documented to have any relationship with Peripherals::take() ... this is relied on by RTFM for soundness

If conjure() was provided instead of steal(), there would be no relationship to document; if you wanted to affect future calls to take(), you can do so explicitly with the expression I gave above. Especially when soundness is a concern, why rely on documentation when the code can describe itself?

@agausmann
Copy link

Either way it's probably good to state in the docs how it affects take(), whether or not it does.

adamgreig pushed a commit that referenced this issue Jan 12, 2022
186: do not KEEP the .stack_sizes section r=therealprof a=japaric

this undoes PR #118

I found a problem with keeping this section. Turns out that the input
`.stack_sizes` sections contain references to all functions compiled with `-Z
emit-stack-sizes` (the section contains the addresses of all those functions
after all) so keeping those section prevents the linker from removing *any* of
those functions. This is not a problem today because `-Z emit-stack-sizes` is
*opt-in* and only used to analyze a program.

However, I am proposing a rust-lang/rust PR to build the `compiler-builtins`
crate with `-Z emit-stack-sizes`. That change will cause *all* the functions in
that crate to be kept in binaries that link to `cortex-m-rt`, regardless of
whether the crate author uses `-Z emit-stack-sizes` or not, leading a increase
in binary size of about 14 KB (`.text` section).

To prevent issues with that rust-lang/rust PR I propose we undo PR #118. On the
bright side, the tools that were depending on this (`cargo-stack-sizes` and
`cargo-call-stack`) no longer do so in their latest releases so nothing is lost
on the tooling front with this change.

r? @rust-embedded/cortex-m

Co-authored-by: Jorge Aparicio <jorge@japaric.io>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants