[DeviceASAN][NFC] Code Restructure #2232
Conversation
github-actions
bot
added
loader
AllanZyne
changed the title
AllanZyne
changed the title
|
Hi @oneapi-src/unified-runtime-maintain, could you please prioritize the review of this PR? |
|
I've pushed a commit to fix this in LLVM part.
|
|
Hi @pbalcer, could you please review and merge this PR before other sanitizer related PRs? |
| @@ -93,10 +92,9 @@ void StackTrace::print() const { | |||
| BacktraceInfo BI = BacktraceSymbols[i]; | |||
|
|
|||
| // Skip runtime modules | |||
| if (!getContext()->interceptor->getOptions().Debug && | |||
There was a problem hiding this comment.
I think we should keep verbosity printing for debug mode, maybe by creating a base class of option that have the common options available for all sanitizers, and in this case, the "debug" switch. But that can be addressed later.
There was a problem hiding this comment.
I agree. Besides, I started to prefer to create a single option object for all sanitizers.
We can discuss this later.
Will do, is the PR final? I see that you are still making changes. |
Yes, it's final. I just pushed a commit to address a comment (copyright issue) by Yingcong. |
pbalcer
left a comment
pbalcer
left a comment
There was a problem hiding this comment.
lgtm, just a few nits.
Not insisting, but I think you should drop the prefixes on files if they are not necessary, so "common" instead of "sanitizer_common", "allocator.hpp" instead of "sanitizer_allocator.hpp" and so on.
|
|
||
| using namespace asan; | ||
|
|
||
| static AsanInterceptor *interceptor; |
There was a problem hiding this comment.
Globals mess up constructor/destructor ordering, but I can see you tied its lifetime to the layer context, which is good. But can't we just leave the AsanInterceptor inside of the layer object?
There was a problem hiding this comment.
But can't we just leave the AsanInterceptor inside of the layer object?
Yes, I agree that using global variable is not a good idea.
We are going to develop each sanitizer as independent as possible at the beginning, and when these features are stable, we will try to extract their common characteristics, and create some base classes then.
Now I want the outside world (layer context) don't know how each sanitizer is implemented, and just give it some simple interfaces.
|
I'll merge this once you get an approval on your intel/llvm PR (you are missing llvm-reviewers-runtime, I'll approve on behalf of unified-runtime-reviewers). |
For file naming style, I completely use the same way in compiler_rt (https://github.com/intel/llvm/tree/sycl/compiler-rt/lib/sanitizer_common) for convenience. |
Ok, makes sense, it's better to be consistent. Disregard that then. |
UR: oneapi-src/unified-runtime#2232 Code refactoring for implementing MemorySanitizer --------- Co-authored-by: Piotr Balcer <[email protected]>
4 participants
LLVM: intel/llvm#15843
code refactoring for implementing the msan layer