This patch starts a series of changes dedicated to adding support for targets that do not have hardware memory managers.
D30583 contains the overview patch for this work.
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
You can use -asan-instrumentation-with-call-threshold=0 for this without any changes to llvm.
Comment Actions
If the only thing you'll want to do with kUseSoftwareMemoryManager is to always emit calls, then you should use what @dvyukov mentioned: -asan-instrumentation-with-call-threshold=0
If, eventually, you'll want this flag to mean additional things, then let's add a *run-time* option for it, when it's needed.
| CMakeLists.txt | ||
|---|---|---|
| 82 | Why is this a compile-time option for llvm? | |
Comment Actions
Indeed there's no need to introduce a special clang build. Thanks for pointing out.
Updated.
Comment Actions
Is this something orthogonal to the target triple? Can you know if it's an MMU-less target from the triple?
I'd prefer to switch on the target triple if possible (some component of the triple that would allow us to assume it's an MMU-less target).
| CMakeLists.txt | ||
|---|---|---|
| 95 | That option's name + what it does isn't what the string says. | |
| lib/asan/tests/CMakeLists.txt | ||
| 343 | # The software memory manager requires out-of-line instrumentation.
if(NOT SANITIZER_USE_SOFTWARE_MEMORY_MANAGER)
add_asan_tests_for_arch_and_kind(${arch} "-inline")
endif()
add_asan_tests_for_arch_and_kind(${arch} "-with-calls"
-mllvm -asan-instrumentation-with-call-threshold=0)And no need to have the argument passing above. At least for now this seems enough. | |
| test/asan/lit.cfg | ||
| 86 | Check out how we handle COMPILER_RT_DEBUG, and use pythonize_bool with SANITIZER_USE_SOFTWARE_MEMORY_MANAGER. | |
| lib/asan/tests/CMakeLists.txt | ||
|---|---|---|
| 343 | This way building the "-with-calls" tests will fail due to duplicate "-mllvm -asan-instrumentation-with-call-threshold=0" option. With SANITIZER_USE_SOFTWARE_MEMORY_MANAGER turned on, the option is part of the command string by default. | |
Comment Actions
It seems to be orthogonal to the architecture and vendor fields and we can probably deduce presence of MMU from the OS and environment fields of the triple. For development and testing purposes, however, we still would need a cmake-level option that makes it possible to build and run Asan tests with the software memory manager enabled on Linux and other platforms that do have MMU.
Comment Actions
I don't like adding a new configuration option, but the argument about testing the no-MMU code paths in an MMU environment makes sense (*assuming* those code paths make sense in that environment).
Do you plan on setting up a bot for this configuration? (Right after this patch it's not absolutely needed, but I'm guessing it will soon, when the other patches get here)
| lib/asan/tests/CMakeLists.txt | ||
|---|---|---|
| 343 | Not if you remove the chunk above adding that flag. That way you only need to change here. | |
| test/asan/lit.cfg | ||
| 87 | No need to use getattr if you're always setting it in lit.common.configured.in. Just if config. use_software_memory_manager: should do it, no? | |
Comment Actions
Yes, we are planning establishing a buildbot at some point later when there is more of the no-MMU specific code to maintain.
Comment Actions
I have no problems with this patch (assuming the rest of the patches will follow), but double-check that @kcc is ok with it (I haven't been paying too much attention to the discussion thread, sorry).
Comment Actions
I was on vacation/ooo, sorry for delays.
LGTM (assuming the new option is for testing on linux)
Comment Actions
Yes, I can commit it, thanks. I just thought that we probably should postpone committing it until we know we can do the rest in a form suitable for putting to mainline.
Comment Actions
Dmitry, with your permission I'm going to update and commit this patch as it seems to be useful regardless of which way we support sanitizers on MMU-less platforms.
One more thing: now that D31592 is landed, would you mind if we do the same for Tsan?
Thanks.
Comment Actions
Dmitry, with your permission I'm going to update and commit this patch as it seems to be useful regardless of which way we support sanitizers on MMU-less platforms.
If you plan to continue working on this, I think it's fine.
It's not end-user-visible, so we are not committing to anything (no pun intended).
Comment Actions
One more thing: now that D31592 is landed, would you mind if we do the same for Tsan?
In general getting rid of the reverse mapping is positive.
However, there is a potential problem with tsan. The memory accesses handling functions are super performance sensitive, and we tried very hard to get number of memory accesses in general and spill in particular to absolute minimum (you can see that insane lib/tsan/check_analyze.sh).
Now, there is a reason to pass shadow address to ReportRace -- this way we convert address to shadow in the very beginning of the function and then it's not needed anymore (if we need original address in ReportRace we instead convert shadow back), this frees 1 register for the function. If you pass address to ReportRace, it means that it needs to be preserved alive for the whole duration of the function, which can cause additional spills.
So if you can make it without affecting fast path performance, then I am all for it. Otherwise, my plan was to get rid of the reverse mapping only in the nommu mode under ifdefs.
Why is this a compile-time option for llvm?