-
Couldn't load subscription status.
- Fork 1
Rtsan/blocking 2 llvm pass #35
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.
Overall this is good, a few small nits but this is very similar to how I would have it.
| return PA; | ||
| } | ||
|
|
||
| static void transformRealtimeUnsafeFunction(Function &F) { |
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.
Nit (take it or leave it):
Transform is a little vague, could be more descriptive as to the actual transformation taking place
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, agreed
| static PreservedAnalyses rtsanPreservedAnalyses() { | ||
| PreservedAnalyses PA; | ||
| PA.preserveSet<CFGAnalyses>(); | ||
| return PA; |
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 get rid of this function and leave these explicit. I think the loop changing we will do will not preserve CFG, so I think it is OK to just do this explicitly in our main block
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.
Hmm yeah - part of my refactoring in the main pass function was intended to have a sequence of:
if (condition) {
return transformXYZ(...);
}
else if (condition) {
return transformABC(...);
}... to express the intent that these transformations are mutually exclusive and return early. But now I'm thinking about it, it doesn't really need to be that way - and it's a bit early days to start those sorts of refactors. So yeah, I think I'll revert this as you suggest
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.
On second thoughts, we still need it in two places. I've updated the function name to be more explicit, though - you'll see this in my latest PR. When we no longer need it as part of the sanitize_realtime pass, we can inline it then. I think it's better to keep it in this case, don't you?
|
|
||
| FunctionType *FuncType = | ||
| FunctionType::get(Type::getVoidTy(F.getContext()), | ||
| {PointerType::getUnqual(F.getContext())}, false); |
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.
we should call reviewer eyes to this code when we post this so we make sure it is correct (not saying it isn't just that I have no clue about this code)
|
|
||
| ; RealtimeSanitizer pass should insert __rtsan_expect_not_realtime at function entrypoint | ||
| ; CHECK-LABEL: @blocking_function() | ||
| ; CHECK-NEXT: call{{.*}}@__rtsan_expect_not_realtime({{ptr .*}}) |
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.
nice one!
No description provided.