[compiler-rt] r240539 - tsan: don't print external PCs in reports

[Dmitry Vyukov]

On Wed, Jun 24, 2015 at 8:21 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
>
> On Wed, Jun 24, 2015 at 6:04 AM, Dmitry Vyukov <dvyukov at google.com> wrote:
>>
>> Author: dvyukov
>> Date: Wed Jun 24 08:04:12 2015
>> New Revision: 240539
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=240539&view=rev
>> Log:
>> tsan: don't print external PCs in reports
>>
>> They are meaningless.
>>
>>
>> Added:
>>     compiler-rt/trunk/test/tsan/java_symbolization.cc
>> Modified:
>>     compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h
>>     compiler-rt/trunk/lib/sanitizer_common/sanitizer_stacktrace_printer.cc
>>     compiler-rt/trunk/lib/tsan/rtl/tsan_symbolize.cc
>>     compiler-rt/trunk/lib/tsan/rtl/tsan_symbolize.h
>>     compiler-rt/trunk/test/tsan/java.h
>>
>> Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h?rev=240539&r1=240538&r2=240539&view=diff
>>
>> ==============================================================================
>> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h (original)
>> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h Wed Jun 24
>> 08:04:12 2015
>> @@ -44,6 +44,10 @@ static const uptr kMaxNumberOfModules =
>>
>>  const uptr kMaxThreadStackSize = 1 << 30;  // 1Gb
>>
>> +// Denotes fake PC values that come from JIT/JAVA/etc.
>> +// For such PC values __tsan_symbolize_external() will be called.
>> +const uptr kExternalPCBit = 1ULL << 60;
>> +
>
>
> I think you should instead introduce a function IsExternalPC(), and hide
> this magic constant in .cc file as an implementation detail.

That will increase code size, introduce an additional indirection and
don't allow grepping for constant value. What are the benefits?


>>  extern const char *SanitizerToolName;  // Can be changed by the tool.
>>
>>  extern atomic_uint32_t current_verbosity;
>>
>> Modified:
>> compiler-rt/trunk/lib/sanitizer_common/sanitizer_stacktrace_printer.cc
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_stacktrace_printer.cc?rev=240539&r1=240538&r2=240539&view=diff
>>
>> ==============================================================================
>> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_stacktrace_printer.cc
>> (original)
>> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_stacktrace_printer.cc
>> Wed Jun 24 08:04:12 2015
>> @@ -99,7 +99,9 @@ void RenderFrame(InternalScopedString *b
>>        break;
>>      case 'M':
>>        // Module basename and offset, or PC.
>> -      if (info.module)
>> +      if (info.address & kExternalPCBit)
>> +        {} // There PCs are not meaningful.
>> +      else if (info.module)
>>          buffer->append("(%s+%p)", StripModuleName(info.module),
>>                         (void *)info.module_offset);
>>        else
>>
>> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_symbolize.cc
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_symbolize.cc?rev=240539&r1=240538&r2=240539&view=diff
>>
>> ==============================================================================
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_symbolize.cc (original)
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_symbolize.cc Wed Jun 24 08:04:12
>> 2015
>> @@ -44,7 +44,7 @@ extern "C" bool __tsan_symbolize_externa
>>                                 int *line, int *col)
>>                                 SANITIZER_WEAK_ATTRIBUTE;
>>
>> -bool __tsan_symbolize_external(uptr pc,
>> +bool WEAK __tsan_symbolize_external(uptr pc,
>>                                 char *func_buf, uptr func_siz,
>>                                 char *file_buf, uptr file_siz,
>>                                 int *line, int *col) {
>
>
> Can't you combine a declaration and definition here?
> extern "C" bool __tsan_symbolize_external(...) SANITIZER_WEAK_ATTRIBUTE {
>   return false;
> }


Done in r240633.

>>
>>
>> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_symbolize.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_symbolize.h?rev=240539&r1=240538&r2=240539&view=diff
>>
>> ==============================================================================
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_symbolize.h (original)
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_symbolize.h Wed Jun 24 08:04:12
>> 2015
>> @@ -18,10 +18,6 @@
>>
>>  namespace __tsan {
>>
>> -// Denotes fake PC values that come from JIT/JAVA/etc.
>> -// For such PC values __tsan_symbolize_external() will be called.
>> -const uptr kExternalPCBit = 1ULL << 60;
>> -
>>  void EnterSymbolizer();
>>  void ExitSymbolizer();
>>  SymbolizedStack *SymbolizeCode(uptr addr);
>>
>> Modified: compiler-rt/trunk/test/tsan/java.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/tsan/java.h?rev=240539&r1=240538&r2=240539&view=diff
>>
>> ==============================================================================
>> --- compiler-rt/trunk/test/tsan/java.h (original)
>> +++ compiler-rt/trunk/test/tsan/java.h Wed Jun 24 08:04:12 2015
>> @@ -22,3 +22,5 @@ int  __tsan_java_release_store(jptr addr
>>  void __tsan_read1_pc(jptr addr, jptr pc);
>>  void __tsan_write1_pc(jptr addr, jptr pc);
>>  }
>> +
>> +const jptr kExternalPCBit = 1ULL << 60;
>>
>> Added: compiler-rt/trunk/test/tsan/java_symbolization.cc
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/tsan/java_symbolization.cc?rev=240539&view=auto
>>
>> ==============================================================================
>> --- compiler-rt/trunk/test/tsan/java_symbolization.cc (added)
>> +++ compiler-rt/trunk/test/tsan/java_symbolization.cc Wed Jun 24 08:04:12
>> 2015
>> @@ -0,0 +1,44 @@
>> +// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
>> +#include "java.h"
>> +#include <memory.h>
>> +
>> +extern "C" bool __tsan_symbolize_external(jptr pc,
>> +                                          char *func_buf, jptr func_siz,
>> +                                          char *file_buf, jptr file_siz,
>> +                                          int *line, int *col) {
>> +  if (pc == (1234 | kExternalPCBit)) {
>> +    memcpy(func_buf, "MyFunc", sizeof("MyFunc"));
>> +    memcpy(file_buf, "MyFile.java", sizeof("MyFile.java"));
>> +    *line = 1234;
>> +    *col = 56;
>> +    return true;
>> +  }
>> +  return false;
>> +}
>> +
>> +void *Thread(void *p) {
>> +  barrier_wait(&barrier);
>> +  __tsan_write1_pc((jptr)p, 1234 | kExternalPCBit);
>> +  return 0;
>> +}
>> +
>> +int main() {
>> +  barrier_init(&barrier, 2);
>> +  int const kHeapSize = 1024 * 1024;
>> +  jptr jheap = (jptr)malloc(kHeapSize + 8) + 8;
>> +  __tsan_java_init(jheap, kHeapSize);
>> +  const int kBlockSize = 16;
>> +  __tsan_java_alloc(jheap, kBlockSize);
>> +  pthread_t th;
>> +  pthread_create(&th, 0, Thread, (void*)jheap);
>> +  __tsan_write1_pc((jptr)jheap, 1234 | kExternalPCBit);
>> +  barrier_wait(&barrier);
>> +  pthread_join(th, 0);
>> +  __tsan_java_free(jheap, kBlockSize);
>> +  fprintf(stderr, "DONE\n");
>> +  return __tsan_java_fini();
>> +}
>> +
>> +// CHECK: WARNING: ThreadSanitizer: data race
>> +// CHECK:     #0 MyFunc MyFile.java:1234:56
>> +// CHECK: DONE