Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: Implement RealtimeSanitizer (RTSan) support, add
#[realtime(nonblocking)]
,#[realtime(blocking)]
attributes #3766base: master
Are you sure you want to change the base?
RFC: Implement RealtimeSanitizer (RTSan) support, add
#[realtime(nonblocking)]
,#[realtime(blocking)]
attributes #3766Changes from 1 commit
adc27b3
11a2711
f844ab8
4348f86
8568f5c
8043b62
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Attribute naming thread
Existing comments: #3766 (comment) , #3766 (comment) , #3766 (comment)
I'll toss my hat in for
realtime(constrained)
to mark a "please validate this function" root, andrealtime(blocking)
to mark a function as forbidden underneath a validation root. Further naming work is needed to have separate validation classes for "blocking event loop" (target 20Hz 50ms) versus "blocking the frame timer" (target 120Hz 8ms - you cannot call Windows Thread sleep) versus "hard realtime" (no allocations)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.
Reading further, I see
rtsan_scoped_disabler!
. I already had text typed to proposerealtime(unconstrained)
for that usecase, but deleted it after I realized that wasn't the two primary attributes.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 like
realtime(constrained)
andrealtime(blocking)
. I would be OK with that being the two.Any thoughts other co-authors @steckes @davidtrevelyan?
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 can get on board with this style, for sure! If we're happy with
realtime(blocking)
, would we also be happy withrealtime(nonblocking)
? I suggest this in an effort to maintain the blocking/nonblocking symmetry and consistency with clang, while at the same time being explicit about it being realtime blocking/nonblocking (and thereby reducing confusion with other definitions of blocking and nonblocking). What do you think?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 also like to keep it closer to the original documentation, so it is less confusing.
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 also support
realtime(nonblocking/blocking)
. This may be the best of both worlds as it keeps the documentation green, and doesn't collide with any async work. Any thoughts @riking @clarfonthey @programmerjake?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.
seems good to me!
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 have updated the doc to use
realtime(nonblocking/blocking)
because of the support of all original authors and no objections (thusfar). Of course this can be changed again if more feedback comes in :)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.
@steckes @programmerjake starting a thread to discuss changing or not changing this macro name (so it's not in the main thread)
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.
@steckes and @programmerjake just poking this conversation here on the name of this proposed macro. Interested to see if you have any new thoughts.
After thinking about it some more, I think I support the original proposal of
rtsan_scoped_disabler!
. There are a couple reasons why I think this is right:rtsan_scoped_disabler!
also follows the rule of keeping as much documentation and education out there green, because it matches the clang name. It will match all of the LLVM docs, any tech talks and documentation in other languages.no_sanitize_realtime
would overload the attribute of the same name, so I'm not sure it's quite right. Users may be confused why this is sometimes a macro and sometimes an attribute.assume_realtime_unchecked!
is great because it's general, it may be preferable to do the opposite and be as specific as possible until such an extension is actually needed.rtsan_scoped_disabler!
is unambiguous what tool it is referring to. If some time in the future there is some additional real-time checking tool, we can add some "umbrella macro"assume_realtime_unchecked!
. Because we have no conception of what that tool could be, or how it may work, I think there is a case to make this macro as specific to rtsan as possible. I think this follows a kind of atomic design principle.Open to any feedback or discussion as always.
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'm not familiar with the implementation of RealtimeSanitizer, but would it be possible to implement this kind of thing /
sanitizer/rtsan_interface.h
in a crate instead? I recall that something like that is possible with Callgrind.If that's the case, you could perhaps downscope this to just "add RealtimeSanitizer and
std
support", and let users figure it out for themselves to do something else oncfg!(sanitize = "realtime")
if they have some API themselves that they want to mark as realtime.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'm not sure I understand, could you elaborate more?
As for internal details, let me give you some more info to help understand the inner workings of rtsan.
The main component is the sanitizer runtime library. This is a simple dynamic library, similar to the runtime components of tsan, asan etc. Like any shared library, if you have the header and the compiled library you can include it in your binary.
So, it would be possible to include the header, and link against the shared library. In fact, we made a standalone proof of concept crate to work out many of the kinks of rtsan in rust and write this proposal, that can be seen here:
https://github.com/realtime-sanitizer/rtsan-standalone-rs
With an article from @steckes on his blog on how he implemented and lessons he learned (not necessary to read for this RFC, but may provide additional info): https://steck.tech/posts/rtsan-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.
So, assuming that
std
can get special treatment and thus we don't need to worry about that for the design of the feature / the public API, the main part is thatrustc
is going to link in this sharedlibclang_rt.rtsan*
library when you enable-Zsantizer=realtime
(or-Csantizer=realtime
), right?If that is done, could the
rtsan-standalone-rs
crate be updated such that e.g.realtime_enter
would be defined as the following?Along with
rtsan-standalone-sys
being updated to not do anything inbuild.rs
(becauserustc
already does it for it)?(Remember that the
sanitize = "..."
cfg is already provided for the other sanitizers).If so, then I don't see the reason why these attributes has to be exposed by
rustc
as a language feature?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.
My argument here is not so much against a built-in
#[blocking]
etc., it's just that, if we can avoid having this in the language, and restrict it to just adding arustc
flag, then there's a much higher probability that the feature will be accepted (actually, then I don't even think you'd need an RFC, at most an MCP. But I'm not a compiler team member, so I can't say that for sure).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.
Yes, you're correct in your understanding here!
I see what you're saying, I think! I agree this would make it easier to accept/deny this RFC, the most controversial part is definitely the addition of the attributes (and changes to
assert
,panic
etc).However, as an end user, I think it would be strange for the compiler to expose the flag, link the library and do nothing until I downloaded and used another third party crate (
rtsan-standalone-rs
). I think a good proposal from the end users perspective would allow them to do everything with only rustc, not relying on a third party crate.I think the addition of the attributes into the standard would also encourage libraries to document their expectations - far in the future you could imagine just looking at the signature of functions and reasoning about if they are real-time safe because they have the attribute.
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.
Again, I'm not the one that's gonna make a decision here, but personally, I would like to evaluate the merits of such attributes separately from the actual sanitizer being added.
That's fair, and I agree that having this in the language/std could have ecosystem effects, but then I think the RFC should be written around that; or at least have a section that says "why does this belong in the language/std".
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.
In our initial zulip thread, one of the compiler team members said we did not need an RFC for adding the sanitizer, only the attributes (see the thread). However, to piece together the RFC we thought it made most sense to provide all of the context in which the attributes will be used.
Definitely we expect most of the conversation here to be around the attributes!
I'll wait a bit to see what some others say about this. Thanks for the feedback so far, it's appreciated.
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.
Note that it would be possible to make
panic!
and the following backtrace process not allocate, but obviously it currently does.