Page MenuHomePhabricator

[sanitizer] On OS X, verify that interceptors work and abort if not, take 2
ClosedPublic

Authored by kubamracek on Mar 16 2016, 8:30 AM.

Details

Summary

This is a new version of http://reviews.llvm.org/D18121, which I had to revert due to test failures. It's not possible to reopen a closed revision here, so I'm creating a new one.

The issue is that TSan unit tests run with a statically linked runtime, where interceptors don't work, but that's fine, we don't need them. With the patch, this is now detected and the unit tests are aborted. To avoid this, we have DisableReexec(), but due to static initializers order, it's not possible to call this function early enough (we'd have to make more invasive changes). So I'm proposing a simpler fix, which simply adds a "disable_reexec" flag that is set in unit tests.

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek updated this revision to Diff 50827.Mar 16 2016, 8:30 AM
kubamracek retitled this revision from to [sanitizer] On OS X, verify that interceptors work and abort if not, take 2.
kubamracek updated this object.
kubamracek added reviewers: dvyukov, samsonov, glider, kcc.
dvyukov edited edge metadata.Mar 16 2016, 8:54 AM

The issue is that TSan unit tests run with a statically linked runtime, where interceptors don't work, but that's fine, we don't need them.

What is the difference between that unit test and normal programs? Normal programs also link tsan runtime statically (or is not the case on Mac?). Can we instead make the test more like a normal program so that it does not need any special handling? Adding runtime flags for the sake of a single weird test does not look good to me.

The issue is that TSan unit tests run with a statically linked runtime, where interceptors don't work, but that's fine, we don't need them.

What is the difference between that unit test and normal programs? Normal programs also link tsan runtime statically (or is not the case on Mac?). Can we instead make the test more like a normal program so that it does not need any special handling? Adding runtime flags for the sake of a single weird test does not look good to me.

Normal programs and lit tests always link TSan dynamically on OS X. Unit tests need a static library because they need to access non-exported symbols.

samsonov edited edge metadata.Mar 16 2016, 12:09 PM

This is really unfortunate. I also don't like adding the runtime flag for the sake of TSan unit test. Why can we call DisableReexec() from ASan unit tests, but not from TSan ones? The former also use static runtime on OS X?

This is really unfortunate. I also don't like adding the runtime flag for the sake of TSan unit test. Why can we call DisableReexec() from ASan unit tests, but not from TSan ones? The former also use static runtime on OS X?

The difference is in the interceptors of new/delete C++ operators. In ASan, we have INTERCEPTOR(void *, _Znwm, size_t size), which is just a regular interceptor, and in the static library, it doesn't work (just as all other interceptors). But in TSan, we declare a custom operator with void *operator new(__sanitizer::uptr size), which works even in the static library. The TEST() macro from GTest expands to a static initializer that calls new, which gets intercepted and TSan will initialize at this point. We'd need to call DisableReexec() before any other initializers.

One solution would be to put the static initializer that calls DisableReexec() to a file that's first in the CMake source file list. That's quite fragile, I don't think we want to depend on the order of file lists in CMake.

Converting the new/delete operators to be the same as ASan uncovers some test failures and needs more changes/fixes. One thing that breaks is demangling in stack trace, as things like wrap__Znwm will appear (instead of operator new), because the wrap_ prefix prevents demangling. This bug currently exist in ASan as well, but no testcase covers it (on OS X). But TSan expects this to work in a few lit tests already.

One solution would be to put the static initializer that calls DisableReexec() to a file that's first in the CMake source file list. That's quite fragile, I don't think we want to depend on the order of file lists in CMake.

Maybe add a weak function in runtime that says whether we need to reexec and override it in the test (since it links runtime statically, it should work). I would prefer such function to an additional public flag. Should be less code as well.

... that is, make weak bool DisableReexec() { return false; } and override it with function returning true in unit tests.

kubamracek updated this revision to Diff 50862.Mar 16 2016, 1:35 PM
kubamracek edited edge metadata.

Updating patch to use a weak function.

dvyukov accepted this revision.Mar 16 2016, 1:39 PM
dvyukov edited edge metadata.

LGTM but wait for Alexey

This revision is now accepted and ready to land.Mar 16 2016, 1:39 PM
samsonov accepted this revision.Mar 16 2016, 2:46 PM
samsonov edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.