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

Imported document about undefined behavior and safe api in LLD #127

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

pellico
Copy link

@pellico pellico commented Dec 9, 2024

No description provided.

@JoelMarcey JoelMarcey requested a review from PLeVasseur December 9, 2024 20:21
@JoelMarcey JoelMarcey added the coding guidelines Related to work in the Coding Guidelines Subcommittee label Dec 9, 2024
Copy link
Collaborator

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Hi @pellico -- I gave this an initial read today, found a few suggestions. I'd like to read through it again and review.

I'd suggest posting a link to this PR into the Zulip to see if we can have further review by others as well 🙂

pellico and others added 3 commits December 11, 2024 09:04
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Pete LeVasseur <[email protected]>
…43/pellico/safety-critical-rust-consortium into rust_safety_low_level_driver
Copy link
Contributor

@vapdrs vapdrs left a comment

Choose a reason for hiding this comment

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

Interesting read. I left some comments, mostly grammar related.

pellico and others added 9 commits December 18, 2024 16:23
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Douglas Deslauriers <[email protected]>
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Douglas Deslauriers <[email protected]>
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Douglas Deslauriers <[email protected]>
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Douglas Deslauriers <[email protected]>
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Douglas Deslauriers <[email protected]>
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Douglas Deslauriers <[email protected]>
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Douglas Deslauriers <[email protected]>
@PLeVasseur
Copy link
Collaborator

@pellico -- I know this has been here for a bit. I'm hoping that you coordinating with the Embedded WG to discuss the concept further (perhaps having some sort of cross-over meeting / discussion in a small group?) and the work @adfernandes will work on for the "what is unsafety" pamphlet can better flesh out where to draw the line.

As I've mentioned, it's possible for us in the safety-critical space to draw the line in that gray zone a bit closer to the side of "safety".

@pellico
Copy link
Author

pellico commented Jan 31, 2025

@pellico -- I know this has been here for a bit. I'm hoping that you coordinating with the Embedded WG to discuss the concept further (perhaps having some sort of cross-over meeting / discussion in a small group?) and the work @adfernandes will work on for the "what is unsafety" pamphlet can better flesh out where to draw the line.

As I've mentioned, it's possible for us in the safety-critical space to draw the line in that gray zone a bit closer to the side of "safety".

I am perfectly fine with this solution. However what I described is applicable also to external libraries. I don't see any difference between calling an external function or start an operation of external peripheral.
So I would like to see how the definition of undefined behavior will evolve in the context of external libraries and then see the definition is applicable to embedded system as well.

Copy link
Contributor

@vapdrs vapdrs left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Collaborator

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Hey @pellico -- main thing I'd like to emphasize is that this may or may not be a Rust Project concern, it may be a svd2rust crate or Tools team concern or even simply within the domain of safety-critical if they don't feel it's within their realm of responsibility. I'd like to have this portion rewritten if possible.

I've given some minor edit suggestions here and there otherwise.

involves hardware:

- *Executing code compiled with platform features that the current
platform does not support (see target\_feature), except if the
platform explicitly documents this to be safe.*

Before to discuss safe/unsafe in Low Level Drivers let`s see the meaning
Before discussing safe/unsafe in Low Level Drivers let`s see the meaning
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Before discussing safe/unsafe in Low Level Drivers let`s see the meaning
Before discussing safe/unsafe in Low Level Drivers let's see the meaning

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll highlight this one here as I think my ask wasn't clear. It seems you're using tilde (`) instead of apostrophe (') in spots in the document. I was trying to note those spots. Could you switch the tildes for apostrophes where needed?


In this document I tried to show why the measures required to have safe
LLD are not clear. The reason derives from the definition of “undefined
behavior”. This is a call for Rust community and Rust specification
Copy link
Collaborator

Choose a reason for hiding this comment

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

From my point of view, we should be careful to delineate those things which the Rust Project and most of the Rust community would say lead to "undefined behavior" from a particular (and important!) segment which cares about safety-critical and writing low-level drivers.

I think that this involves some advocacy on our part as the Safety-Critical Rust Consortium Coding Guidelines Subcommittee.

As we have chatted about a few times, this seems to be a bit of a "scoping" or "system" issue where as you show here it depends on how you view the scope of your system and how it can do things which lead to "undefined behavior".

I'd recommend rewriting this section here to emphasize perhaps this "community" nature, since as you highlight the svd2rust crate here (which is maintained by the Tools team) is a first major collaborator in this mission you're embarking on.

Copy link
Author

@pellico pellico Feb 14, 2025

Choose a reason for hiding this comment

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

@PLeVasseur This was an old document that I though could be useful as food for thought and I have no problem to change.

The main point that I tried to highlight is that there is an inconsistency in the community regarding the definition of undefined behaviors and when use or not use unsafe keyword.

This inconsistency in the scope of safety this inconsistency can lead to different interpretation and therefore safety risk.

I would propose to rewrite the document in following way:

  • Highlight the inconsistency/unclarity of unsafe usage and undefined behavior definition across Rust community
  • Remove any reference to svd2rust
  • Remove all reference to HW peripheral (I will mention only interface to C Low Level Driver) and consider a more generic external world (FFI)
  • Remove the section about solution proposal and call for a community solution, better definition in Rust upcoming specification or just a guidance in coding guidelines.
  • Highlight the risk of this ambiguity in the context of safety application.

Is this addressing your recommendation ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you've got the start of a useful document here to drive discussion, for sure.

The aspect that I think you address pretty well and could be due more emphasis is what we often look at in a safety-critical context, which is of system scoping, i.e. where the safety issues could crop up.

While in a vacuum calling a certain API that's FFI or directly accessing memory or manipulating hardware may not cause a safety concern from the point of view, it's within the greater context of what a system is trying to accomplish that something may result in undefined behavior. Your diagrams and explanations I think go to that point.

The main point that I tried to highlight is that there is an inconsistency in the community regarding the definition of undefined behaviors and when use or not use unsafe keyword.

My main challenge with the document as-is is in this framing. I think that within the context of the Rust programming language the understanding of undefined behavior is ~reasonably well-defined.

However, when we take a function call that's doing something which in and of itself doesn't cause undefined behavior, but due to the ordering of the function called with respect to another function, that's where this system scoping framing can be useful.

e.g.

  • Calling a foo() function: defined
  • Calling foo(), then bar() functions: defined
  • Calling bar(), then foo() functions: undefined behavior

Therefore, when doing a low-level driver that should accomplish foo() + bar() we will consider the system scope as surrounding both of them to ensure they're called in the correct order.

I think it's admirable to say that low-level software that's still fairly close to manipulating hardware should not be unsafe! I'd like to make sure it's being motivated and communicated in this way.

@PLeVasseur PLeVasseur added the documentation Improvements or additions to documentation label Feb 13, 2025
pellico and others added 3 commits February 14, 2025 09:08
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Pete LeVasseur <[email protected]>
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Pete LeVasseur <[email protected]>
…guidelines/rust-embedded-lld-safe-definition.md

Co-authored-by: Pete LeVasseur <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coding guidelines Related to work in the Coding Guidelines Subcommittee documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants