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

Rust RFC #2

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

Rust RFC #2

wants to merge 5 commits into from

Conversation

cjappl
Copy link
Owner

@cjappl cjappl commented Dec 4, 2024

No description provided.

RUST_RFC.md Outdated

As stated in the previous section, many of the interceptors "just work". Often, the Rust runtime will call into the libc runtime to allocate memory (`malloc` et al), interface with the networking stack (`socket` et al) or do I/O (`read`, `write`, `open`, `close` et al).

One place where this is not the case is `Mutex::lock`. Locks are disallowed in real-time contexts but because `Mutex::lock` begins as a spin-lock, the RTsan runtime cannot automatically detect its usage.
Copy link

Choose a reason for hiding this comment

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

Let's just say because Mutex::lock does not call the pthread functions or double check that it in fact can be called spin lock.

RUST_RFC.md Outdated Show resolved Hide resolved
RUST_RFC.md Outdated
```

### nosanitize
Another approach we could take is similar to the ASan and TSan `nosanitize` attribute. We advocate for the scoped disabler macro, as it allows users to specify a more specific scope to disable the tool in. This means users will not have to extract unsafe code into helper functions to disable them at the function level.
Copy link

Choose a reason for hiding this comment

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

It is called no_sanitize and we should add it there as well because currently you can specify the precise sanitizer you want to disable: no_sanitize(address). Would be nice to add no_sanitize(realtime) there.

Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

also see the PR: rust-lang/rust#68164

Copy link
Owner Author

Choose a reason for hiding this comment

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

I put in a sentence in this next upcoming draft requesting feedback which path to take. The LLVM folks were pretty convinced NOT to add nosanitize, so we will have to see if Rust wants it if we can go back and add the proper attrs

RUST_RFC.md Outdated Show resolved Hide resolved
RUST_RFC.md Outdated
* **Are attributes inheritable on interfaces?**
* **do we make it a requirement of all lock types?**
* **What other details can we provide here??**
* **Are there other obvious ones we want to add this attribute to?**
Copy link

Choose a reason for hiding this comment

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

Definitely std::sync::RwLock::lock as it is similar to a Mutex only allowing multiple readers at the same time.
We should check with try_lock if it is hitting anything already when unlocking.

Copy link

@steckes steckes Dec 5, 2024

Choose a reason for hiding this comment

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

We could also think about the atomic functions that not do a direct atomic operation but trigger a CAS-loop. As far as I know CAS loops have unbounded execution time or am I wrong? I can send you the part where Jon is explaining when the Rust atomics have CAS-loops instead of a direct atomic operation.

Copy link

@steckes steckes Dec 5, 2024

Choose a reason for hiding this comment

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

My guess is, that most of the other stuff like file io, network stuff and so on will trigger something in libc.

Maybe a bit out of scope for now, but one unknown I have in my mind is async fn and .await, that should probably not be allowed, but I don't even know if it is possible to somehow call async functions from your process block, as you need an async runtime to do that. Maybe when adding async to a fn it could automatically be marked blocking, just in case someone somehow manages to do that.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok, I will add RwLock, good idea.

CAS loops are tricky - I'm not sure I'd advocate for adding those right now. I think you're right in them being unbounded but I think we should be 100% convinced before putting it in. I know at least using >1 producer or >1 consumer necessitates a loop and is technically unbounded. I am not sure about 1 producer and 1 consumer - is that always unbounded as well because one thread can "spam"?

Copy link
Owner Author

Choose a reason for hiding this comment

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

My guess is, that most of the other stuff like file io, network stuff and so on will trigger something in libc.

Yes this is my guess too - if you have a little time would you give this a try in the example project and report back? Things like writing to stdout, opening a file, connecting to a socket should all automatically fail.

Maybe a bit out of scope for now, but one unknown I have in my mind is async fn and .await, that should probably not be allowed

Good idea, I think this may be opening up a big can of worms so we may consider leaving it for a v2

RUST_RFC.md Outdated

1. RealtimeSanitizer can be enabled in unstable mode - like the other sanitizers
2. The introduction of `nonblocking` (marking a function as real-time constrained) and `blocking` (marking a function as inappropriate for use in a `nonblocking` context)
3. The addition of the `blocking` attribute to `Mutex::lock`
Copy link

@steckes steckes Dec 5, 2024

Choose a reason for hiding this comment

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

maybe say directly std::sync::Mutex::lock to be precise?

RUST_RFC.md Outdated Show resolved Hide resolved
@cjappl
Copy link
Owner Author

cjappl commented Dec 5, 2024

@steckes I just put up a new version attempting to address each of your comments, please give it another look when you get a chance. Thanks for the feedback so far

rtsan_scoped_disabler!(
let audio = vec![1.0; 256]; // report is suppressed
)
}
Copy link

Choose a reason for hiding this comment

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

this is a reminder for me, to check if this is possible. Currently it is

rtsan_scoped_disabler!({
   let audio = vec![1.0; 256];
});

with curly braces and closing semicolon.
But it does not really matter for the proposal.

RUST_RFC.md Outdated Show resolved Hide resolved
RUST_RFC.md Outdated

> In aerospace guidance systems if your software doesn't update on a regular tick your simulation of what is happening may diverge from reality. Unfortunately this may also mean your rocket converges with the ground.

Code in these environments must run in a deterministic amount of time. Allocations, locks, and other OS resource access are disallowed because they don't have bound upper execution time. Unbound execution time leads to blown deadlines, and blown deadlines leads to consequences.

Choose a reason for hiding this comment

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

Minor: I think the point in the final sentence in this paragraph has already been made, so could be removed here

RUST_RFC.md Outdated

As stated in the previous section, many of the interceptors "just work". Often, the Rust runtime will call into the libc runtime to allocate memory (`malloc` et al), interface with the networking stack (`socket` et al) or do I/O (`read`, `write`, `open`, `close` et al).

A couple places where this is not the case is `std::sync::Mutex::lock` and `std::sync::RwLock::lock`. Locks are disallowed in real-time contexts but because these methods do not call in to `pthread_mutex_lock` initially, the RTsan runtime cannot automatically detect its usage.

Choose a reason for hiding this comment

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

On "Locks are disallowed in real-time contexts"...

This is difficult to say definitively. Consider two use cases:

  1. the spin mutex is locked by both non-real-time and real-time threads, vs.
  2. the spin mutex is locked only by real-time threads, which never try to re-lock it immediately

In the case of 1., we can get the classic priority inversion if the real-time thread tries to acquire while the non-real-time thread has the lock. But in the case of 2., there are situations where all real-time threads can make forward progress (e.g. popping jobs off a thread pool queue).

We kind of get out of jail for free in the C++ spin mutex case, because adding the attribute [[clang::nonblocking]] is a user decision. But if we were to blanket apply it to std::sync::Mutex::lock, then we might get into trouble with conflicting user requirements. I'm not sure what to do here - let's discuss it further soon?

Copy link

@steckes steckes left a comment

Choose a reason for hiding this comment

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

Yes, looks good :)

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.

3 participants