Details
- Reviewers
eugenis - Commits
- rG36daf074d997: [hwasan] also omit safe mem[cpy|mov|set].
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
This leads to a 20 % runtime improvement on PDFium, when stack instrumentation is disabled.
| llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp | ||
|---|---|---|
| 789 | Is this a separate bugfix? Am I right that this is not needed for regular load/store because the argument is required to be 100% traceable to a single alloca, but 2-args memintrinsics are safe if one arg is 100%, and the other is 100% not stack? That does not sound right. The comment on accessIsSafe does not even try to cover such cases. Also, I do not see any tests under Analysis/StackSafety for the mixed memintrinsic case. | |
| llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll | ||
| 132 | If lifetimes are irrelevant to a test case, you can just remove them altogether. | |
| llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp | ||
|---|---|---|
| 976 | Consider getDest / getSource for readability. | |
| llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp | ||
|---|---|---|
| 789 | Before this change, we only checked this for instructions that took a single pointer variable. There were two cases in accessIsSafe:
Now, for memcpy / memmove there is the extra case that we can have both cases for two arguments. Now the problem becomes if one argument is in range of the alloca, but the other one isn't reachable from any alloca, accessIsSafe will return true. What I did in this change is clarify the semantics (as they already were) of accessIsSafe in the comment, and defer responsibility to check whether *all* arguments are potentially stack to the caller. As such, we only hit the second case above. | |
| llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll | ||
| 132 | The stack safety does look at lifetimes, so to make it more representative I put them there. Either way is fine, WDYT? | |
| llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp | ||
|---|---|---|
| 789 | Yeah, I agree with that. I just want it clarified even more. For a sample of cases:
the comment does not give me confidence in what isAccessSafe answer would be. | |
| llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll | ||
| 132 | There are no lifetimes at -O0, for example. It is generally a good idea to focus each test case on a single concern - if you are testing hwasan handling of memmove() with one safe and one unsafe argument, lifetimes are irrelevant and only hurt test readability. It is good to have some tests with lifetimes, but they don't really need to be present *everywhere*. | |
| llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp | ||
|---|---|---|
| 789 | I clarified the comment and added some more tests. | |
| llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll | ||
| 132 | Sure, removed for legibility (though with sanitizers enabled, there are always lifetimes, and this is a sanitizer test). | |
| llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp | ||
|---|---|---|
| 789 | I still find the description confusing, especially around "potential stack accesses". How about this:
i.e. not "accesses that may go to stack are safe" but "safe when goes to stack". To be true to this description, the function should return true if the argument can not be traced back to an alloca. I think this can be implemented by removing "SafeAccesses" and looking straight at "AccessIsUnsafe". WDYT? | |
| 789 | Now since both branches do interesting stuff only when findAllocaForValue is non-null, we can hoist this predicate. if (findAllocaForValue(Ptr)) {
if (!InstrumentedStack) return true;
if ((SSI && SSI->accessIsSafe(*Inst)) return true;
} | |
I remember from the my C days that one of gotchas with memmove/memcpy is overlapping regions. Not sure if it is relevant the context of this change, just wanted to mention it.
| llvm/include/llvm/Analysis/StackSafetyAnalysis.h | ||
|---|---|---|
| 84 | ||
Thanks! That should be okay, we look at the two parameters separately; there is even a test that has memmove to itself (@test_in_range4).
LGTM
| llvm/lib/Analysis/StackSafetyAnalysis.cpp | ||
|---|---|---|
| 928 ↗ | (On Diff #373926) | This is kind of ugly, but ok for a debug print. We could also
Up to your what seems easiest. |
| llvm/lib/Analysis/StackSafetyAnalysis.cpp | ||
|---|---|---|
| 928 ↗ | (On Diff #373926) | Makes sense. Let's submit this and maybe do this in a follow up. |