[compiler-rt] Realtime Sanitizer: Introduce Realtime Sanitizer (RTSan) backend #92460
Conversation
|
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -1382,6 +1382,10 @@ collectSanitizerRuntimes(const ToolChain &TC, const ArgList &Args, | |||
| StaticRuntimes.push_back("asan_cxx"); | |||
| } | |||
|
|
|||
| if (!SanArgs.needsSharedRt() && SanArgs.needsRadsanRt()) { | |||
| StaticRuntimes.push_back("radsan"); | |||
| } | |||
There was a problem hiding this comment.
One question we have here: What is the SharedRuntimes vs StaticRuntimes? Why would we need to support both? This is one change we "eyeballed" based on the other sanitizers.
There was a problem hiding this comment.
I have some notes at https://maskray.me/blog/2023-01-08-all-about-sanitizer-interceptors#static-runtime
Nit: we omit braces in this single-line statement case https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
There was a problem hiding this comment.
Ok, if I can summarize the important bits in my own words:
Different platforms have different defaults they use for the sanitizers, some (Darwin, for example) use shared, others (clang on Linux) use static.
A user can specify and override the default with the flags -[shared|static]-libsan.
To support the most possible configurations, do you think we should add back in the ability to link the shared library in this file? In Darwin.cpp we already show the ability to link the shared runtime, so it seemingly would be as simple as adding in a:
if (SanArgs.needsRadsanRt())
SharedRuntimes.push_back("radsan");
To the block above. Any advice?
| @@ -32,6 +32,9 @@ set(ALL_ASAN_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${RISCV64} | |||
| ${LOONGARCH64}) | |||
| set(ALL_ASAN_ABI_SUPPORTED_ARCH ${X86_64} ${ARM64} ${ARM64_32}) | |||
| set(ALL_DFSAN_SUPPORTED_ARCH ${X86_64} ${MIPS64} ${ARM64} ${LOONGARCH64}) | |||
| set(ALL_RADSAN_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${RISCV64} | |||
| ${MIPS32} ${MIPS64} ${PPC64} ${S390X} ${SPARC} ${SPARCV9} ${HEXAGON} | |||
| ${LOONGARCH64}) | |||
There was a problem hiding this comment.
This was copied from ASAN, is this acceptable? We don't have all architectures to test, although we think that our interceptors are simple enough to work on any machine (no platform specific assembly in our code, for example)
|
Pinging possibly interested parties for review. @yln @vitalybuka @Sirraide @AaronBallman @dougsonos @davidtrevelyan Please feel free to add more, we don't know who all may be interested. Thanks in advance |
cjappl
changed the title
|
|
||
| namespace detail { | ||
|
|
||
| static pthread_key_t Key; |
There was a problem hiding this comment.
Avoid VariableName in new compiler-rt code.
Use the recommended variable_name like asan/hwasan/lsan. Some new compiler-rt subprojects used Variable as cargo culting llvm/ and clang/, which is a pity (you can also read https://llvm.org/docs/Proposals/VariableNames.html for more history)
There was a problem hiding this comment.
Great thanks!! Before I go through and edit these, snake_case is preferred for variables in all contexts?
Really appreciate the review. Pushing up most of your requested changes now.
There was a problem hiding this comment.
Yes, variable_name and FunctionName.. Sorry if this change may take a while.
asan/hwasan/lsan are good examples to follow.
There was a problem hiding this comment.
No apology necessary! Happy to do this right the first time.
Does this also apply to lambdas?
auto Func = [&vec]() { vec.push_back(0.4f); };
Should this become:
auto func = [&vec]() { vec.push_back(0.4f); };
Or remain "function case"?
There was a problem hiding this comment.
I pushed the change for all non-lambda variable names, happy to change the lambdas too based on your feedback.
|
https://discourse.llvm.org/t/rfc-nolock-and-noalloc-attributes/76837/75 contains a discussion about the subproject name. |
|
Weekly ping of reviewers @yln @vitalybuka @Sirraide @AaronBallman @zygoloid @compnerd @petrhosek @isanbard Looking for weighing in on any of the code as it stands now, or the naming question posed by MaskRay here, so far there seems to be support for rtsan over radsan :) |
+1 for rtsan |
cjappl
changed the title
|
If you want another round, make sure to click |
|
Pinging reviewers, after we had more conversations on the overall structure and usefulness of RTSan, and it was approved. @zygoloid @vitalybuka @MaskRay Details of the aforementioned discussion are around here in the discourse thread: |
|
Bumped the branch to resolve the conflict in config-ix.cmake |
|
Pinging reviewers @vitalybuka @MaskRay @zygoloid All the comments on this PR have been addressed, looking for more feedback, or approval/merge if we are getting close! Thanks in advance :) |
clangDriver changes are usually the last. The expectation is that if
We typically split clangDriver and compiler-rt changes. Runtime compiler-rt tests often expose minor issues that only fail on certain platforms. Keeping it separate prevents us from potentially reverting the compiler-rt code, leading to unnecessary churn. |
Yeah, disabling the testsuite should be safe. The tests can be enabled after clangCodeGen and clangDriver patches landed. |
- remove empty initializers - better namespace compliance with LLVM standards - Fix lit.cfg.py to remove crufty elements not needed - Change name of InternalFree, add explainer comment
|
Pinging reviewers @vitalybuka @zygoloid State of the review: 1 approval from MaskRay, all open comments have been addressed. Looking for more feedback, or more approval or a merge if this is looking good. Thanks much! :) |
| @@ -67,3 +67,7 @@ D: ThreadSanitizer | |||
| N: Bill Wendling | |||
| E: isanbard@gmail.com | |||
| D: Profile runtime library | |||
|
|
|||
There was a problem hiding this comment.
BTW @alexander-shaposhnikov NSAN is missing from here.
| // See https://llvm.org/LICENSE.txt for license information. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// |
There was a problem hiding this comment.
In my experience LIT tests in are easier to maintain in LLVM environment
TEST(TestRtsan, are internals test, so probably fine to keep as GTEST
However TEST(TestRtsanInterceptors are likely better as regular LIT tests
There was a problem hiding this comment.
Actually even TestRtsan mostly can be done as LIT
BTW. Why I don't like GTEST in LLVM?
LIT is easier to reproduce and experiment and debug than with a particular test case of.
Also I don't trust incremental builds with GTEST, some times I am not sure that my changes are used in tests.
A couple of time CMakes bugs caused compiler-rt GTEST not executed on bots and it was unnoticed for a long time. LIT is majority, so unlikely it could happen with them.
There was a problem hiding this comment.
Seems reasonable to convert these to lit! I propose leaving this for this commit, and we convert them in the near future. Specifically this next commit will introduce the -fsanitize=realtime flag, and come with a bunch of the lit tests that we have written, so it will be easier to convert them once that infrastructure has landed.
One clarifying question, for when that work is done:
Should we have a lit test for every intereceptor? The reason we did it in gtest unit tests is that adding a new interceptor's test is a few lines, as opposed to a brand new file with the lit boilerplate. This makes it easy to make sure that every interceptor does what we think it does. We worried about the "cost" of the new file for every interceptor. In our demo branch, as we have it laid out, the lit tests test on the full end-to-end system level, while these tests actually hit every interceptor.
There was a problem hiding this comment.
Should we have a lit test for every intereceptor?
I don't have strong preference. For other sanitizer default interceptor behavior is to crash, so we usually create a test per interceptor.
The reason we did it in gtest unit tests is that adding a new interceptor's test is a few lines, as opposed to a brand new file with the lit boilerplate. This makes it easy to make sure that every interceptor does what we think it does. We worried about the "cost" of the new file for every interceptor. In our demo branch, as we have it laid out, the lit tests test on the full end-to-end system level, while these tests actually hit every interceptor.
Yes, boilerplate of LIT is not nice. So there is a trade off. I guess you can wait and see.
|
Also, just as a heads up, neither of the authors have commit access as we are new to LLVM. If this is OK to go in we will need help with that step. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/1332 Here is the relevant piece of the build log for the reference: |
|
I can reproduce and will debug in an hour. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/974 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/2414 Here is the relevant piece of the build log for the reference: |
Thanks for the help @vitalybuka , I am looking into this as well, but very grateful for more experienced eyes on the matter too. I'll keep you posted here if I find anything |
|
There are 3 errors I see:
Working to see if I can fix the llvm_gtest issue right now. I would love advice on Fuchsia and disabling rtsan for windows. |
|
This patch I have a strong hunch fixes issue 1 in my comment above. This patch compiles and runs on my linux container and my mac, proving it doesn't affect compilation on those two systems. This also better matches the other sanitizers, llvm_gtest is a dependency in llvm, anything in compiler-rt using gtest just uses the sources directly as seen in: |
|
This also breaks building the compiler-rt for older android versions, because pthread_spinlock_t is only defined when |
Thanks for reporting this @glandium ! I am in the process of disabling this for android as we speak. Let me know if you see anything else fishy |
Follow on to #92460 (reporting old android builds failing) and #98264 (powerpc failing to discover gtests). Getting back to stability by getting back down to a very basic set of supported arches and OS's. In the future if there is demand we can build it back up. Especially when I understand how to test these systems, or have people who want to do the work on them. --------- Co-authored-by: David Trevelyan <david.trevelyan@gmail.com>
…) backend (llvm#92460) Introducing the main runtime of realtime sanitizer. For more information, please see the [discourse thread](https://discourse.llvm.org/t/rfc-nolock-and-noalloc-attributes/76837). We have also put together a [reviewer support document](https://github.com/realtime-sanitizer/radsan/blob/doc/review-support/doc/review.md) to show what our intention is. This review introduces the sanitizer backend. This includes: * CMake build files (largely adapted from asan). * Main RTSan architecture (the external API, thread local context, stack). * Interceptors. * Many unit tests. Please see the [reviewer support document](https://github.com/realtime-sanitizer/radsan/blob/doc/review-support/doc/review.md) for what our next steps are. We are moving in lockstep with this PR llvm#84983 for the codegen coming up next. Note to reviewers: If you see support documentation mention "RADSan", this was the "old acronym" for the realtime sanitizer, they refer to the same thing. If you see it let us know and we can correct it (especially in the llvm codebase) --------- Co-authored-by: David Trevelyan <david.trevelyan@gmail.com>
Follow up to llvm#92460 DEPS llvm_gtest is not used by compiler-rt, compiler-rt compiles them with COMPILER_RT_GOOGLETEST_SOURCES. This reverts commit e217f98.
Follow on to llvm#92460 (reporting old android builds failing) and llvm#98264 (powerpc failing to discover gtests). Getting back to stability by getting back down to a very basic set of supported arches and OS's. In the future if there is demand we can build it back up. Especially when I understand how to test these systems, or have people who want to do the work on them. --------- Co-authored-by: David Trevelyan <david.trevelyan@gmail.com>
Introducing the main runtime of realtime sanitizer. For more information, please see the discourse thread. We have also put together a reviewer support document to show what our intention is.
This review introduces the sanitizer backend. This includes:
Please see the reviewer support document for what our next steps are. We are moving in lockstep with this PR #84983 for the codegen coming up next.
Note to reviewers: If you see support documentation mention "RADSan", this was the "old acronym" for the realtime sanitizer, they refer to the same thing. If you see it let us know and we can correct it (especially in the llvm codebase)