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
[llvm][rtsan] Add transform pass for sanitize_realtime_unsafe #109543
[llvm][rtsan] Add transform pass for sanitize_realtime_unsafe #109543
Changes from all commits
ddca9d5
144c637
8cc5872
e3302a4
1a7b7e3
47754d4
0e656bb
384d92f
bc0f8ed
b4db726
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.
Mostly out of interest, why do we de-mangle here and not in the reporting?
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.
That's a really great question. In all honesty I didn't originally think about the possibility of de-mangling during the report stage. After thinking about it further, I have an idea on which one will probably be less work overall, but I'd need advice on whether the distribution of work is appropriate.
Here's how I see the work involved:
halt_on_error
is off)My very naive intuition is that demangling in the pass is preferable, but very happy to move it to the reporting if I have my priorities a bit upside down. Thanks in advance for any suggestions here.
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 you are missing the downside of binary size, because demangled name takes up more space, for demangling in the pass.
I am not sure if demangling will cause any noticable overhead over unwinding and symbolizing the stack frame.
Taking a step back, why do we pass this name and not use the PC and symbolize that?
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.
Thanks for this question and prompting us to look into this in more detail. I had indeed not considered the issue of binary size, so went away and thought about it a bit harder with @cjappl. There's lots to think about here, especially in terms of how we expect the sanitizer to be used.
We anticipate that the sanitizer users will only be marking a handful of functions they are worried about with the
[[clang::blocking]]
attribute, which is what ultimately results insanitize_realtime_unsafe
being added in IR. We think this because any functions that do the normal bad real-time stuff like allocation will be picked up by the libc interceptors anyway. Adding the[[clang::blocking]]
attribute is more for user functions that could block entirely within user space, like a spin mutex lock or similar busy-wait scenario. Conservatively estimating that a user has 10 concerning[[blocking]]
functions, each with demangled symbol length 25 characters - I think that would be (at least ideally) of the order 250 bytes of extra binary size to store. The rtsan runtime library is (when stripped) currently about 750 kB already, so we think this is about a 0.03% size increment - and only for sanitized builds.We tried the PC approach in a branch and confirmed it works, but we were sad to lose the function name for printing diagnostics in release builds, and also to have to do the extra run-time work to unwind the stack. Being able to run the sanitizer with release builds is one of our design goals - we'd like to achieve as-near-normal performance as possible, so that users can meaningfully QA their real-time systems with RealtimeSanitizer enabled.
Given:
... our moderate preference is currently to keep the approach how we have it outlined in this PR. However, this is of course caveated with the appreciation that we're very new here and inexperienced with the codebase, so I'm very keen to take as much advice as possible on this. Many thanks for your help so far - keen to hear your thoughts!
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.
Confirming that this also meant that the whole stack trace symbolization was gone, but that was okay.
I don't understand. To symbolize a PC, we don't need to unwind the stack. From the runtime CLs I saw go buy, it seems like you unwind the stack on error regardless though
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 - thanks - that was indeed my intended meaning.
That's correct, my mistake. We can discount the runtime cost of unwinding the stack and just concentrate on the functionality/binary size trade-off. After some more thought, we also realised that the symbolised PC approach would make suppression of these errors difficult to implement, because we would no longer have a function name to match against a suppression list. For this reason, we're leaning more strongly towards the existing implmentation and taking the small hit on binary size. I noticed that you approved this PR last night, so I guess you're happy enough with it as it is - please let me know if you have any further concerns. Thanks again for the review 👍
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.
Use update_test_checks.py, unless there's some reason this is not possible here.
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.
Awesome, great tool. Thanks for showing me this script - I've updated this test in 0775459