-
Notifications
You must be signed in to change notification settings - Fork 0
Add rtsan #1
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments in terms of wording.
Overall, I'm feeling a little uneasy about officially exposing these internal calls to some official release. I'm going to ping @davidtrevelyan so we can discuss more about it.
Another thought, on a different note: Is there any way that we can have one single construct that does the entry and exit at all entry points? In C++ this would be called RAII, similar to ScopedMutex, preventing a call from .lock()
from ever being unmatched from a .unlock()
.
I worry that something like ?
would bubble up some error and leave the sanitizer in a bad state where it's been entered, but not exited. Interested to hear your thoughts on this one, or maybe Stephan has already figured it out for the standalone?
"x86_64-apple-darwin" => darwin_libs("osx", &["asan", "lsan", "tsan"]), | ||
"x86_64-apple-darwin" => darwin_libs("osx", &["asan", "lsan", "tsan", "rtsan"]), | ||
"x86_64-unknown-fuchsia" => common_libs("fuchsia", "x86_64", &["asan"]), | ||
"x86_64-apple-ios" => darwin_libs("iossim", &["asan", "tsan"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be supported as well, we create an ios library when we build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is x86_64 ios, I don't even know how that is used. Regular arm ios i already have added. But just making sure, if it is supported i will of course add it.
"x86_64-unknown-linux-gnu" => common_libs( | ||
"linux", | ||
"x86_64", | ||
&["asan", "dfsan", "lsan", "msan", "safestack", "tsan", "rtsan"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these were in in alpha order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. "hwsan" in aarch linux gnu was at the end before. So not really ordered.
* [LeakSanitizer](#leaksanitizer) a run-time memory leak detector. | ||
* [MemorySanitizer](#memorysanitizer) a detector of uninitialized reads. | ||
* [ThreadSanitizer](#threadsanitizer) a fast data race detector. | ||
* [RealtimeSanitizer](#realtimesanitizer) a detector of blocking functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe go with "non-deterministic execution time calls in real-time contexts".
(as a side note in our docs and presentations we were trying to go with real-time over realtime. This is a matter of preference but it would be good to stick with what we decided back then)
pub fn __rtsan_realtime_enter(); | ||
/// Exits a realtime context. After calling this the program could still be in a realtime context, | ||
/// as they can be nested. | ||
pub fn __rtsan_realtime_exit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible addition around these first two "Each call to enter must be matched with exactly one call to exit. Anything else leads to undefined behavior in the sanitizer."
I believe we have a similar note in our official docs if you want to check out that wording.
pub fn __rtsan_enable(); | ||
/// Should be called before any other sanitizer calls are made. | ||
pub fn __rtsan_ensure_initialized(); | ||
/// Notify the sanitizer that a blocking function was called. This will trigger a violation report. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"To mark your function as not safe for real-time contexts, place this call as the first line in the function. If rtsan hits this call while in a real-time context it will abort in the same way that calling an illegal system call does"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to avoid mentioning "safe" or "unsafe". I think my version is still fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: whoops the new version wasn't showing up on my phone, will review on my computer later!!
/// Disables the sanitizer. | ||
pub fn __rtsan_disable(); | ||
/// Enables the sanitizer again. | ||
pub fn __rtsan_enable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"These two calls allow you to disable the sanitizer for a given scope. Useful for situations where you know some system call has deterministic execution time under the hood"
This will only ever go into rust nightly. I thought about adding a explicit comment that this API has the same stability promise as rust nightly (so basically none). Do you think that would be helpful?
Yeah in the standalone we use RAII. the issue is that i can't put anything in the rust stdlib. So i guess i could add a comment about that in the docs here. We could do a library (or add it to this https://github.com/rcvalle/rust-crate-sanitizers) that would expose this structure, but otherwise everyone who uses this would have to copy these ffi definitions and maybe the RAII struct definition. This of course isn't really ergonomic, and it won't be necessary once the attribute exists in rust. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs look good, I think we should get to the bottom of the API exposure before putting it against the upstream. I have pinged David
# RealtimeSanitizer | ||
RealtimeSanitizer detects blocking calls in realtime contexts. If enabled, | ||
it won't sanitize the whole program, it needs to be enabled explicitly. | ||
RealtimeSanitizer detects non-deterministic execution time calls in realtime contexts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"real-time contexts"
pub fn __rtsan_realtime_exit(); | ||
/// Disables the sanitizer. | ||
/// Disables the sanitizer. Must be matched to a later call to `__rtsan_enable()`. | ||
/// Useful for situations where you know the systemcall you do has a deterministic execution time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
system call
pub fn __rtsan_disable(); | ||
/// Enables the sanitizer again. | ||
/// Enables the sanitizer again. Must be matched to a preceding call to `__rtsan_disable()`. | ||
/// Useful when you know that a systemcall you do has a deterministic execution time in that situation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
system call
Isn't relevant anymore as we now use the new attribute. |
…=jieyouxu Rehome 30 `tests/ui/issues/` tests to other subdirectories under `tests/ui/` [#1 of Batch #2] Part of rust-lang#133895 Methodology: 1. Refer to the previously written `tests/ui/SUMMARY.md` 2. Find an appropriate category for the test, using the original issue thread and the test contents. 3. Add the issue URL at the bottom (not at the top, as that would mess up stderr line numbers) 4. Rename the tests to make their purpose clearer Inspired by the methodology that `@Kivooeo` was using. r? `@jieyouxu`
No description provided.