Page MenuHomePhabricator

[TSan] Support `ignore_noninstrumented_modules` on Linux
Needs ReviewPublic

Authored by yln on May 8 2019, 5:45 PM.

Details

Summary

We implemented the ignore_noninstrumented_modules flag for Darwin [1]
to be able to use TSan on Darwin, where the "re-compile the world"
approach is difficult to follow in practice.

Now we are in the same situation when adding TSan support for Swift on
Linux. The libdispatch library is a source of false positives and
compiling it with TSan (although it is compiled from source on Linux)
proves to be tricky and insufficient. The better path seems to be to
make ignore_noninstrumented_modules work on Linux.

The missing piece with this functionality on Linux is the detection
whether or not a loaded library is instrumented (which is suprisingly
difficult). In this patch we use the ELF segment metadata and symbol
table to check whether any sanitizer symbols are present. [2,3] were
useful resources in the development of this patch (especially,
getSymbolCountFromGnuHash).

Also enable flag for libdispatch tests on Linux to make them pass. The
default value for this flag on Linux remains OFF.

[1]: https://reviews.llvm.org/D28264
[2]: https://chromium.googlesource.com/crashpad/crashpad/+/1f1657d573c789aa36b6022440e34d9ec30d894c%5E%21/
[3]: https://flapenguin.me/2017/05/10/elf-lookup-dt-gnu-hash/

Event Timeline

yln created this revision.May 8 2019, 5:45 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 8 2019, 5:45 PM
Herald added subscribers: llvm-commits, Restricted Project, kubamracek. · View Herald Transcript
yln marked an inline comment as done.May 8 2019, 5:47 PM
yln added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cc
661

ignore_noninstrumented_modules is only relevant for TSan, but this code is in sanitizer_common. Is this acceptable? How can we make it better?

yln edited the summary of this revision. (Show Details)May 8 2019, 5:51 PM
yln added reviewers: dvyukov, kubamracek, dcoughlin, samsonov.

Nice, thanks. I don’t understand the ELF code so someone else should review it, but from my view this LGTM.

dvyukov edited reviewers, added: vitalybuka, eugenis; removed: samsonov.May 8 2019, 10:21 PM

Evgenii, maybe you can think of some simpler magical way to understand if a module is instrumented or not?

yln updated this revision to Diff 198912.May 9 2019, 2:33 PM
yln edited the summary of this revision. (Show Details)

Chang symbol name check to require an exact match with the TSan
initializer called from the module ctor. This should avoid
accidental matches, e.g., __tsan_testonly_barrier_wait in tests.
My assumption is that this symbol is present in all modules
instrumented with TSan.

Add non-platform specific test. (I kept the existing test for
Darwin because it tests a specific system API.)

yln updated this revision to Diff 199487.May 14 2019, 11:34 AM

Fix linker error in test.

yln edited the summary of this revision. (Show Details)May 20 2019, 1:22 PM

I am still a bit worried by the amount of complex code we have for this. I wonder if there is a simpler way to do this. Have you considered any other alternatives? Could we emit some symbol in the library and then use dlsym on it? Or maybe emit some section and then iterate over section names only?

yln added a comment.May 21 2019, 10:00 AM

I am still a bit worried by the amount of complex code we have for this. I wonder if there is a simpler way to do this. Have you considered any other alternatives?

I agree with you on the complexity here. Alternatives: it's the only thing I was able to come up with without the help of the compiler.

Could we emit some symbol in the library and then use dlsym on it? Or maybe emit some section and then iterate over section names only?

That sounds certainly possible (and less complex). Initially, I wasn't sure if we want to try to avoid requiring compiler, but I can't think of disadvantages that we don't already have (compiler/runtime compatibility).

I will try the emit symbol/dlopen approach you suggested and open a new revision for it. Is it okay if this is TSan-specific?

There is also dlinfo which returns some info about a shared object. I don't know if it may help or not, just pointing out. We need just any mark that the compiler can leave in the shared object and runtime then can detect as easily as possible.

What do you mean by tsan-specific?

If you mean that it will work only for tsan now, I guess it is fine. Other sanitizers can add it when they need it.