[ppc64] Disable sibling-call-optimization when building tsan library by clang

[cycheng]

Summary

Use "-mllvm -disable-ppc-sco=true" for clang when building tsan library, because there is a distance (in terms of instruction count) assumption between StackTrace::GetCurrentPc and BufferedStackTrace::Unwind.

Tail-call to "PrintCurrentStackSlow" break this assumption on PPC64.

See: https://llvm.org/bugs/show_bug.cgi?id=27280#c1

[hfinkel]

I'd prefer that we not fix the problem this way. You mention in the bug report that we could increase the scan distance:

uptr BufferedStackTrace::LocatePcInTrace(uptr pc) {
#ifdef __powerpc__
  const int kPcThreshold = 800;
#else
  const int kPcThreshold = 320;
#endif

why not do that?

[cycheng]

Hi Hal,

Because I found that threshold is only workable for gcc, not for clang.

If I build compiler-rt with clang, distance between sanitizer_print_stack_trace and tsan::PrintCurrentStackSlow is 0x2df8 = 11768

So it looks like feasible work around is disable sco when building tsan library.

(Run-time data when building compiler-rt by clang: https://llvm.org/bugs/show_bug.cgi?id=27280#c2)

CY

[hfinkel]

In D19001#398177, @cycheng wrote:

Hi Hal,

Because I found that threshold is only workable for gcc, not for clang.

If I build compiler-rt with clang, distance between sanitizer_print_stack_trace and tsan::PrintCurrentStackSlow is 0x2df8 = 11768

So it looks like feasible work around is disable sco when building tsan library.

(Run-time data when building compiler-rt by clang: https://llvm.org/bugs/show_bug.cgi?id=27280#c2)

CY

As a general rule, I don't think it is acceptable to have a build-system dependency on an LLVM-level backend parameter. If this is something we need, then we need to add a Clang-level parameter for this. Alternatively, we might add an attribute applied to the function which must not be called as a tail call. On the other hand, before we consider that, is there a more-principled solution to actually fix this problem?

Looking at the code in BufferedStackTrace::LocatePcInTrace and MatchPc, the threshold used does not actually affect runtime, it is simply used to control matching variance. As a result, so long as it is not set large enough to pick up functions coming from other object files, it seems like setting a significantly-larger threshold would have have negative side-effect?

[cycheng]

Agree!

1. First, I would like to update my investigations:
   • Current mechanism of "BufferedStackTrace::LocatePcInTrace and MatchPc" is unable to handle tail-call to "__tsan::PrintCurrentStackSlow" case.
   • I used clang 3.9.0 on x86 to build tsan library, of course it tail-call to "__tsan::PrintCurrentStackSlow", and I found it passed print-stack-trace.cc test was because of lucky. see: https://llvm.org/bugs/show_bug.cgi?id=27280#c3
   • Possible solutions:
     1. Use "always_inline" attribute on "__tsan::PrintCurrentStackSlow"
     2. New mechanism? I'm still thinking @@
2. No, the threshold does not affect runtime, but it depend on compiler's codegen decision, I feel it is a magic number, use large number can cause poping incorrect stack count.

[hfinkel]

In D19001#400915, @cycheng wrote:

Agree!

1. First, I would like to update my investigations:
   • Current mechanism of "BufferedStackTrace::LocatePcInTrace and MatchPc" is unable to handle tail-call to "__tsan::PrintCurrentStackSlow" case.
   • I used clang 3.9.0 on x86 to build tsan library, of course it tail-call to "__tsan::PrintCurrentStackSlow", and I found it passed print-stack-trace.cc test was because of lucky. see: https://llvm.org/bugs/show_bug.cgi?id=27280#c3
   • Possible solutions:
     1. Use "always_inline" attribute on "__tsan::PrintCurrentStackSlow"
     2. New mechanism? I'm still thinking @@
2. No, the threshold does not affect runtime, but it depend on compiler's codegen decision, I feel it is a magic number, use large number can cause poping incorrect stack count.

If adding always_inline to __tsan::PrintCurrentStackSlow will fix the problem, that sounds like the best solution (as it will work across compilers). I suggest proposing a patch adding that attribute (and adding a comment explaining why the function must be always_inline).

[cycheng]

I submit a new patch http://reviews.llvm.org/D19148 to replace this one.
Thanks Hal!

lib/tsan/CMakeLists.txt

if(arch STREQUAL "x86_64")
         VERBATIM)
     elseif(arch STREQUAL "aarch64")
       set(TSAN_ASM_SOURCES rtl/tsan_rtl_aarch64.S)
       # Pass ASM file directly to the C++ compiler.
       set_source_files_properties(${TSAN_ASM_SOURCES} PROPERTIES
         LANGUAGE C)
    elseif(arch MATCHES "powerpc64|powerpc64le")
      set(TSAN_ASM_SOURCES rtl/tsan_rtl_ppc64.S)
+     # On PPC64, when using clang with sibling-call-optimization to build
+     # tsan library, sibling-call (tail-call) to PrintCurrentStackSlow breaks
+     # distance assumption between StackTrace::GetCurrentPc() and
+     # BufferedStackTrace::Unwind(), so we turn off sco to work around it.
+     # See https://llvm.org/bugs/show_bug.cgi?id=27280#c1
+     if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
+       set(TSAN_RTL_CFLAGS ${TSAN_RTL_CFLAGS} -mllvm -disable-ppc-sco=true)
+     endif ()
      # Pass ASM file directly to the C++ compiler.
      set_source_files_properties(${TSAN_ASM_SOURCES} PROPERTIES
        LANGUAGE C)
     else()
       set(TSAN_ASM_SOURCES)
     endif()
     add_compiler_rt_runtime(clang_rt.tsan
       STATIC