Fix LazyInitialization in tsan

[ZijunZhao]

Summary

In Android, further initialization is always necessary whether preinit_array can be used.
LazyInitialize is needed regardless of .preinit_array support on platforms where runtime is loaded as dynamic library, e.g. Android.

[MaskRay]

This presumably disables DT_PREINIT_ARRAY on Android, but bionic supports DT_PREINIT_ARRAY.
What does this fix?

[danalbert]

BIonic supports it, but the sanitizer cannot use it for Android because sanitizers are shared libraries on Android, and preinit array only works for excutables.

Worth a comment IMO.

[eugenis]

I think this is a fix for LazyInitialize in the tsan_rtl.h - the logic there assumes that when preinit_array is always used whenever it can be used, and no further initialization is necessary. This assumption does not hold under -shared-libsan. IMO this should be fixed in LazyInitialize, not here.

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_rtl.h

681–682

should we instead define SANITIZER_CAN_USE_PREINIT_ARRAY=0 for ANDROID ?

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_rtl.h

681–682

I misread the patch.
I agree with @eugenis, removing #if is LGTM

[ZijunZhao]

This comment was removed by ZijunZhao.

compiler-rt/lib/tsan/rtl/tsan_rtl.h

681–682

@pirama considers that removing the #if does come with a performance cost and we don't want to regress other platforms. I agree with him, so I keep it.

[pirama]

compiler-rt/lib/tsan/rtl/tsan_rtl.h

681–682

Yes, it was my suggestion to add this only for Android to avoid the branch, even if it's marked UNLIKELY... (Partly because we don't have access to the to measure the actual impact).

Fine to remove the #if if @vitalybuka and @eugenis recommend it.

[eugenis]

compiler-rt/lib/tsan/rtl/tsan_rtl.h

681–682

This is only called in the interceptors. Perf impact should be insignificant. @dvyukov WDYT?

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_rtl.h

681–682

Yes, performance impact should be small.
But I am not following the commit description. The preinit_array function calls the very same Initialize function, so what is "further initialization"?

[ZijunZhao]

compiler-rt/lib/tsan/rtl/tsan_rtl.h

681–682

ohhh before the change, if SANITIZER_CAN_USE_PREINIT_ARRAY

if (UNLIKELY(!is_initialized))

will be skipped so we remove the #if

[vitalybuka]

OWNERS

1 ↗

(On Diff #469306)

I guess you don't want to commit this file?

compiler-rt/lib/tsan/rtl/tsan_rtl.h

681–682

Probably the patch description should mention that
LazyInitialize is needed regardless of .preinit_array support on platforms where runtime is loaded as dynamic library, e.g. Android.

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_rtl.h

681–682

Oh, sorry, I mis-read the question.
The performance impact should be small, but I did not mean to remove the ifdef entirely for all platforms.
It still affects small mem/str* functions. It normal builds they read just few bytes, or maybe even completely inlined. But in tsan build they impose all interceptor overheads. It's not that is_initialized check is super expensive on its own, but these bits sum up. So this LazyInitialize was a part of removing these overhead bits bit-by-bit.
Please restore the ifdef for other platforms.

[ZijunZhao]

@dvyukov yes, I restore the ifder for other platforms and is_initialized check is a must for Android.

compiler-rt/lib/tsan/rtl/tsan_rtl.h

 }
 
 extern bool is_initialized;
 
 ALWAYS_INLINE
 void LazyInitialize(ThreadState *thr) {
   // If we can use .preinit_array, assume that __tsan_init
   // called from .preinit_array initializes runtime before
-  // any instrumented code.
-#if !SANITIZER_CAN_USE_PREINIT_ARRAY
+  // any instrumented code except ANDROID.
+#if (!SANITIZER_CAN_USE_PREINIT_ARRAY || defined(__ANDROID__))
   if (UNLIKELY(!is_initialized))
     Initialize(thr);
 #endif
 }
 
 void TraceResetForTesting();
 void TraceSwitchPart(ThreadState *thr);
 void TraceSwitchPartImpl(ThreadState *thr);