Skip to content
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

Check in "Rust for Embedded C Programmers" #2474

Merged
merged 1 commit into from
Jun 25, 2020
Merged

Check in "Rust for Embedded C Programmers" #2474

merged 1 commit into from
Jun 25, 2020

Conversation

mcy
Copy link
Contributor

@mcy mcy commented Jun 11, 2020

This document is intended to be an introduction to Rust tailored for
engineers with an embedded C background, in particular for those ramping
up on Tock.

I'd like to use this PR to do a final proof-read of the doc. I think we can include more sections we think are pertinent in the future, but I'd like to avoid making content additions in this PR.

Signed-off-by: Miguel Young de la Sota mcyoung@google.com

@mcy mcy requested review from lenary and gkelly June 11, 2020 17:42
@mcy mcy requested review from asb and sjgitty as code owners June 11, 2020 17:42
@gkelly
Copy link
Contributor

gkelly commented Jun 12, 2020

The footnotes aren't rendering properly, I think they need to be on the same line as their definition.

image

@mcy
Copy link
Contributor Author

mcy commented Jun 12, 2020

Yup, you're right. Kinda weird, since that's what the gdoc-to-md extension produces, and it worked fine with another markdown viewer I used.

Fixed!.

Copy link
Contributor

@lenary lenary left a comment

Choose a reason for hiding this comment

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

Here are some comments I have so far. I have got to the "Pattern Matching" header so far.

doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
@lenary
Copy link
Contributor

lenary commented Jun 15, 2020

Overall, this is looking good! Thanks @mcy!

Copy link
Contributor

@lenary lenary left a comment

Choose a reason for hiding this comment

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

Another batch of typos. I got to the end this time.

doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
@mcy
Copy link
Contributor Author

mcy commented Jun 18, 2020

Thanks for the thorough review, Sam.

Copy link

@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

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

This document is incredible, I really wish I has been able to read it before I first started working on Tock. Even having written plenty of embedded Rust now I still learned a few things :)

I did not quite make it to the end yet, but I found a few typos and suggested a few changes up until the unsafe rust section. Please feel free to ignore any or all of them! Also it seems you pushed a commit after I started reviewing but before I finished, so my apologies if my suggestions duplicate changes you already made.

doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
Globals are guaranteed to live in `.rodata`, `.data`, or `.bss`, depending on their mutability and initialization.
Unlike constants, they have unique addresses, but as with constants, they must be initialized with constant expressions.[^44]

Mutable globals are particularly dangerous, since they can be a source of data races in multicore systems.

Choose a reason for hiding this comment

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

static mut variables can still be a source of data races in single core systems (thanks to other forms of concurrency such as interrupts), and on single core systems can still lead to UB because they make it easy to create multiple mutable references to a variable, but the compiler will still assume each mutable reference is unique and optimize accordingly.

Mostly I just think it is important to indicate that even on single core systems mutable globals should only be used with great caution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh, good point. I don't think those are data races in the traditional sense, but I think you can probably observe nonatomicity at the compiler level (e.g. pre-loading into registers, speculative reads, etc).

doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
Comment on lines 543 to 554
All kinds of loops may be annotated with labels (the only place where Rust allows labels):
```rust
'a: loop { .. }
```
`break` and `continue` may be used with those labels (e.g. `break 'a`), which will break or continue the enclosing loop with that label (rather than the closest enclosing loop).
While C lacks this feature, most languages without `goto` have it.

Choose a reason for hiding this comment

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

woah, I never knew Rust had this. Very cool!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A fairly old RFC'd feature is being able to annotate any block with 'a:, which can be broken-with-value to. (This is sort of like call/cc but with compile-time continuation locations.)

Comment on lines 592 to 601
Rust does not quite support inline assembly yet.
Clang's inline assembly syntax is available behind the unstable macro `asm!()`, which will eventually be replaced with a Rust-specific syntax that better integrates with the language.
`global_asm!()` is the same, but usable in global scope, for defining whole functions.
Naked functions can be created using `#[naked]`. See [https://doc.rust-lang.org/1.8.0/book/inline-assembly.html](https://doc.rust-lang.org/1.8.0/book/inline-assembly.html).

Choose a reason for hiding this comment

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

maybe worth noting that the clang syntax is now behind llvm_asm!(), and the new rust-specific syntax is behind asm!()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ironic part is that I contributed to that RFC...

doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
Copy link

@phil-levis phil-levis left a comment

Choose a reason for hiding this comment

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

Overall, this document is excellent. I think almost anyone who reads it will find some parts of it too terse -- those are the parts you don't quite understand. So in that way the document is great, because it says exactly how it is, but doesn't really explain it -- you can find the explanation elsewhere, but have the precise and concise statement to work from and understand. I will definitely give it to every student who starts working on Tock.

doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
### Closures {#closures}

Closures (sometimes called "lambda expressions" in other languages) are function literals that capture some portion of their environment, which can be passed into other functions to customize behavior.
The classic C equivalent is something like `pthread_create`, which takes a function pointer `void* (*)(void*)` and a `void*`, the latter being "captured state" sent across the thread boundary.

Choose a reason for hiding this comment

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

I don't think this is a good analogy -- it's not like pthread_create at all. I think it's safe to say that there is no classic C equivalent, but there are in many other languages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the point I want to get across is that &dyn Fn actually behaves that way at the ABI layer. You pass in a function pointer (well, there's an extra vtable indirection...) with some state that's behind a pointer. This is totally a pattern I've seen before; I think qsort() is similar.

I really want an analogy here with a familiar C function, to make it clear that dyn closures are not just a function pointer, but also a pointer to some context.

Choose a reason for hiding this comment

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

Maybe rather than say "the classic C equivalent" say "the closest analogy in C" and explain how it's different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote some prose. Tell me what you think!

Choose a reason for hiding this comment

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

Definitely clearer. My one remaining suggestion relates to this text:

"In a similar way, Rust need extra state to execute, except arg becomes part of the start_routine value. As we'll see, Rust has a number of different ABIs for closures, some of which closely resemble what pthread_create does; in some cases, the function pointer can even be inlined."

I'd suggest that you mention that unlike the void* in pthread_create, where this context must be manually constructed, the Rust language automatically constructs it through static analysis. I think this stuff is obvious to anyone who's used them before but if a pure C person is reading it this will not be clear.

I'm touching on this in part because closures are so critical in Tock (e.g., grants, MapCell, etc.).

Finally, I think "Rust need" should be "Rust closures need"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@mcy mcy force-pushed the rs4c branch 2 times, most recently from 226d442 to e52d7b6 Compare June 19, 2020 17:09
Copy link

@brghena brghena left a comment

Choose a reason for hiding this comment

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

Some weird stuff is going on with the footnotes that you might be concerned about. (Particularly an off-by-one error.)

Comment on lines +2058 to +2105
[^19]: This syntax: `(my_struct_t) { .a = 0, .b = 1 }`.

[^20]: Rust allows trailing commas basically everywhere.
Copy link

Choose a reason for hiding this comment

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

Something is screwed up around footnote 19/20. Either could successfully be applied where [^19] is in the text, but afterwards all the remaining footnotes are off by one. For example [^20] in the text should really be [^21].

Copy link

Choose a reason for hiding this comment

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

Still pretty sure your footnotes are off-by-one starting at [^20]. I don't think this is just a github visualization issue.

Also, the contents of the `RUSTFLAGS` environment variable are passed to `rustc`, as a mechanism for injecting flags.

The Rust standard library, like libc, is smaller in embedded environments.
The standard library consists of three crates: `core`[^10], `alloc`, and `std`.
Copy link

Choose a reason for hiding this comment

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

This doesn't visualize correctly in the github markdown. The [ and ] disappear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Markdown file isn't intended for GH; it's going to show up on the OpenTitan docs website. You can preview it with util/build_docs.py.

Comment on lines +2034 to +2081

[^10]: [https://doc.rust-lang.org/core/index.html](https://doc.rust-lang.org/core/index.html)

This comment was marked as resolved.

Comment on lines +2109 to +2156
[^40]: [https://doc.rust-lang.org/std/primitive.pointer.html#method.copy_to](https://doc.rust-lang.org/std/primitive.pointer.html#method.copy_to)

[^41]: [https://doc.rust-lang.org/std/primitive.pointer.html#method.copy_to_nonoverlapping](https://doc.rust-lang.org/std/primitive.pointer.html#method.copy_to_nonoverlapping)

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Copy link

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

This is a fantastic document, and I echo the others wishing that it was available when I was starting out! I made it ~2/3 through on this pass

doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved

Rust often refers to references as _borrows_: a reference can borrow a value from its owner for a limited time (the lifetime), but must return it before the owner gives the value up to someone else.
It is also possible for a reference to be a borrow of a borrow, or a _reborrow_: it is always possible to create a reference with a shorter lifetime but with the same value as another one.
Reborrowing is usually performed implicitly by the compiler, usually around call sites, but can be performed explicitly by writing `&*x`.

Choose a reason for hiding this comment

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

Maybe add an example where reborrowing is necessary? I just went hunting for places where we use &* in Tock, and the first one I tried compiled fine with the reborrow removed..., this might be something that's unintentionally overused at the moment?

Choose a reason for hiding this comment

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

Further update: tock/tock#1973

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've definitely come across examples of this... but I'm having trouble thinking of any on the spot. I think calling out the syntax isn't a bad idea, since people may see it and be confused (since it's meaningless in C and probably a bad idea in C++).

doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
@mcy mcy force-pushed the rs4c branch 2 times, most recently from 1e249b5 to e5602f5 Compare June 22, 2020 20:00
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lenary lenary left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcy
Copy link
Contributor Author

mcy commented Jun 24, 2020

WIll merge once I get gkelly's LGTM.

Copy link
Contributor

@gkelly gkelly left a comment

Choose a reason for hiding this comment

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

Some minor spelling things, but I think looks great. Thanks @mcy!

doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
doc/ug/rust_for_c.md Outdated Show resolved Hide resolved
This document is intended to be an introduction to Rust tailored for
engineers with an embedded C background, in particular for those ramping
up on Tock.

Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>
@mcy mcy merged commit 214bc09 into lowRISC:master Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants