[Lldb-commits] [PATCH] D30024: [lldb] Add support for "external" reports in ThreadSanitizer LLDB plugin

[Phabricator via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL295342: [lldb] Add support for "external" reports in 
ThreadSanitizer LLDB plugin (authored by kuba.brecka).

Changed prior to commit:
  https://reviews.llvm.org/D30024?vs=88654&id=88751#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30024

Files:
  
lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp


Index: 
lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp
===
--- 
lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp
+++ 
lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp
@@ -85,6 +85,10 @@
  int *running, const char **name, int 
*parent_tid,
  void **trace, unsigned long trace_size);
 int __tsan_get_report_unique_tid(void *report, unsigned long idx, int 
*tid);
+  
+// TODO: dlsym won't work on Windows.
+void *dlsym(void* handle, const char* symbol);
+int (*ptr__tsan_get_report_loc_object_type)(void *report, unsigned long 
idx, const char **object_type);
 }
 
 const int REPORT_TRACE_SIZE = 128;
@@ -125,6 +129,7 @@
 int fd;
 int suppressable;
 void *trace[REPORT_TRACE_SIZE];
+const char *object_type;
 } locs[REPORT_ARRAY_SIZE];
 
 int mutex_count;
@@ -158,6 +163,8 @@
 const char *thread_sanitizer_retrieve_report_data_command = R"(
 data t = {0};
 
+ptr__tsan_get_report_loc_object_type = 
(typeof(ptr__tsan_get_report_loc_object_type))(void *)dlsym((void*)-2 
/*RTLD_DEFAULT*/, "__tsan_get_report_loc_object_type");
+
 t.report = __tsan_get_current_report();
 __tsan_get_report_data(t.report, &t.description, &t.report_count, 
&t.stack_count, &t.mop_count, &t.loc_count, &t.mutex_count, &t.thread_count, 
&t.unique_tid_count, t.sleep_trace, REPORT_TRACE_SIZE);
 
@@ -177,6 +184,8 @@
 for (int i = 0; i < t.loc_count; i++) {
 t.locs[i].idx = i;
 __tsan_get_report_loc(t.report, i, &t.locs[i].type, &t.locs[i].addr, 
&t.locs[i].start, &t.locs[i].size, &t.locs[i].tid, &t.locs[i].fd, 
&t.locs[i].suppressable, t.locs[i].trace, REPORT_TRACE_SIZE);
+if (ptr__tsan_get_report_loc_object_type)
+ptr__tsan_get_report_loc_object_type(t.report, i, 
&t.locs[i].object_type);
 }
 
 if (t.mutex_count > REPORT_ARRAY_SIZE) t.mutex_count = REPORT_ARRAY_SIZE;
@@ -409,6 +418,8 @@
  o->GetValueForExpressionPath(".suppressable")
  ->GetValueAsUnsigned(0));
 dict->AddItem("trace", StructuredData::ObjectSP(CreateStackTrace(o)));
+dict->AddStringItem("object_type",
+RetrieveString(o, process_sp, ".object_type"));
   });
   dict->AddItem("locs", StructuredData::ObjectSP(locs));
 
@@ -511,6 +522,8 @@
 return "Overwrite of errno in a signal handler";
   } else if (description == "lock-order-inversion") {
 return "Lock order inversion (potential deadlock)";
+  } else if (description == "external-race") {
+return "Race on a library object";
   }
 
   // for unknown report codes just show the code
@@ -634,6 +647,13 @@
->GetValueForKey("locs")
->GetAsArray()
->GetItemAtIndex(0);
+std::string object_type = loc->GetAsDictionary()
+  ->GetValueForKey("object_type")
+  ->GetAsString()
+  ->GetValue();
+if (!object_type.empty()) {
+  summary = "Race on " + object_type + " object";
+}
 addr_t addr = loc->GetAsDictionary()
   ->GetValueForKey("address")
   ->GetAsInteger()
@@ -726,8 +746,17 @@
   ->GetValueForKey("size")
   ->GetAsInteger()
   ->GetValue();
-  result =
-  Sprintf("Location is a %ld-byte heap object at 0x%llx", size, addr);
+  std::string object_type = loc->GetAsDictionary()
+->GetValueForKey("object_type")
+->GetAsString()
+->GetValue();
+  if (!object_type.empty()) {
+result = Sprintf("Location is a %ld-byte %s object at 0x%llx", size,
+ object_type.c_str(), addr);
+  } else {
+result =
+Sprintf("Location is a %ld-byte heap object at 0x%llx", size, 
addr);
+  }
 } else if (type == "stack") {
   int tid = loc->GetAsDictionary()
 ->GetValueForKey("thread_id")


Index: lldb/trunk/source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp
==

[Lldb-commits] [PATCH] D30024: [lldb] Add support for "external" reports in ThreadSanitizer LLDB plugin

[Kuba (Brecka) Mracek via Phabricator via lldb-commits]

kubamracek added inline comments.



Comment at: 
source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp:89
+  
+void *dlsym(void* handle, const char* symbol);
+int (*ptr__tsan_get_report_loc_object_type)(void *report, unsigned long 
idx, const char **object_type);

clayborg wrote:
> kubamracek wrote:
> > clayborg wrote:
> > > Doesn't "dlsym" work on all platforms? Windows? 
> > Right, it probably doesn't work on Windows.  I'll put in a FIXME/TODO (TSan 
> > currenly only works on POSIX anyway), okay?
> Now worries, you can also #ifdef around it if needed and put the code needed 
> for windows right in the expression text here. Does TSAN current work on 
> Windows? If so it seems like bug to not support this feature on windows?
TSan doesn't work on Windows.


https://reviews.llvm.org/D30024



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D30024: [lldb] Add support for "external" reports in ThreadSanitizer LLDB plugin

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: 
source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp:89
+  
+void *dlsym(void* handle, const char* symbol);
+int (*ptr__tsan_get_report_loc_object_type)(void *report, unsigned long 
idx, const char **object_type);

kubamracek wrote:
> clayborg wrote:
> > Doesn't "dlsym" work on all platforms? Windows? 
> Right, it probably doesn't work on Windows.  I'll put in a FIXME/TODO (TSan 
> currenly only works on POSIX anyway), okay?
Now worries, you can also #ifdef around it if needed and put the code needed 
for windows right in the expression text here. Does TSAN current work on 
Windows? If so it seems like bug to not support this feature on windows?


https://reviews.llvm.org/D30024



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D30024: [lldb] Add support for "external" reports in ThreadSanitizer LLDB plugin

[Kuba (Brecka) Mracek via Phabricator via lldb-commits]

kubamracek added inline comments.



Comment at: 
source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp:89
+  
+void *dlsym(void* handle, const char* symbol);
+int (*ptr__tsan_get_report_loc_object_type)(void *report, unsigned long 
idx, const char **object_type);

clayborg wrote:
> Doesn't "dlsym" work on all platforms? Windows? 
Right, it probably doesn't work on Windows.  I'll put in a FIXME/TODO (TSan 
currenly only works on POSIX anyway), okay?


https://reviews.llvm.org/D30024



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D30024: [lldb] Add support for "external" reports in ThreadSanitizer LLDB plugin

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Looks good.




Comment at: 
source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp:89
+  
+void *dlsym(void* handle, const char* symbol);
+int (*ptr__tsan_get_report_loc_object_type)(void *report, unsigned long 
idx, const char **object_type);

Doesn't "dlsym" work on all platforms? Windows? 


https://reviews.llvm.org/D30024



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D30024: [lldb] Add support for "external" reports in ThreadSanitizer LLDB plugin

[Kuba (Brecka) Mracek via Phabricator via lldb-commits]

kubamracek created this revision.

TSan now has the ability to report races on "external" object, i.e. any library 
class/object that has read-shared write-exclusive threading semantics.  The 
detection and reporting work almost out of the box, but TSan can now provide 
the type of the object (as a string).  This patch implements this into LLDB.

This is a new API in the TSan dylib, and we should still support cases the old 
dylib (that don't have the API), so the expression contains a call to dlsym to 
see if the new API is available or not.  Let me know if there's a better 
solution.


https://reviews.llvm.org/D30024

Files:
  
source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp


Index: 
source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp
===
--- 
source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp
+++ 
source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp
@@ -85,6 +85,9 @@
  int *running, const char **name, int 
*parent_tid,
  void **trace, unsigned long trace_size);
 int __tsan_get_report_unique_tid(void *report, unsigned long idx, int 
*tid);
+  
+void *dlsym(void* handle, const char* symbol);
+int (*ptr__tsan_get_report_loc_object_type)(void *report, unsigned long 
idx, const char **object_type);
 }
 
 const int REPORT_TRACE_SIZE = 128;
@@ -125,6 +128,7 @@
 int fd;
 int suppressable;
 void *trace[REPORT_TRACE_SIZE];
+const char *object_type;
 } locs[REPORT_ARRAY_SIZE];
 
 int mutex_count;
@@ -158,6 +162,8 @@
 const char *thread_sanitizer_retrieve_report_data_command = R"(
 data t = {0};
 
+ptr__tsan_get_report_loc_object_type = 
(typeof(ptr__tsan_get_report_loc_object_type))(void *)dlsym((void*)-2 
/*RTLD_DEFAULT*/, "__tsan_get_report_loc_object_type");
+
 t.report = __tsan_get_current_report();
 __tsan_get_report_data(t.report, &t.description, &t.report_count, 
&t.stack_count, &t.mop_count, &t.loc_count, &t.mutex_count, &t.thread_count, 
&t.unique_tid_count, t.sleep_trace, REPORT_TRACE_SIZE);
 
@@ -177,6 +183,8 @@
 for (int i = 0; i < t.loc_count; i++) {
 t.locs[i].idx = i;
 __tsan_get_report_loc(t.report, i, &t.locs[i].type, &t.locs[i].addr, 
&t.locs[i].start, &t.locs[i].size, &t.locs[i].tid, &t.locs[i].fd, 
&t.locs[i].suppressable, t.locs[i].trace, REPORT_TRACE_SIZE);
+if (ptr__tsan_get_report_loc_object_type)
+ptr__tsan_get_report_loc_object_type(t.report, i, 
&t.locs[i].object_type);
 }
 
 if (t.mutex_count > REPORT_ARRAY_SIZE) t.mutex_count = REPORT_ARRAY_SIZE;
@@ -409,6 +417,8 @@
  o->GetValueForExpressionPath(".suppressable")
  ->GetValueAsUnsigned(0));
 dict->AddItem("trace", StructuredData::ObjectSP(CreateStackTrace(o)));
+dict->AddStringItem("object_type",
+RetrieveString(o, process_sp, ".object_type"));
   });
   dict->AddItem("locs", StructuredData::ObjectSP(locs));
 
@@ -511,6 +521,8 @@
 return "Overwrite of errno in a signal handler";
   } else if (description == "lock-order-inversion") {
 return "Lock order inversion (potential deadlock)";
+  } else if (description == "external-race") {
+return "Race on a library object";
   }
 
   // for unknown report codes just show the code
@@ -634,6 +646,13 @@
->GetValueForKey("locs")
->GetAsArray()
->GetItemAtIndex(0);
+std::string object_type = loc->GetAsDictionary()
+  ->GetValueForKey("object_type")
+  ->GetAsString()
+  ->GetValue();
+if (!object_type.empty()) {
+  summary = "Race on " + object_type + " object";
+}
 addr_t addr = loc->GetAsDictionary()
   ->GetValueForKey("address")
   ->GetAsInteger()
@@ -726,8 +745,17 @@
   ->GetValueForKey("size")
   ->GetAsInteger()
   ->GetValue();
-  result =
-  Sprintf("Location is a %ld-byte heap object at 0x%llx", size, addr);
+  std::string object_type = loc->GetAsDictionary()
+->GetValueForKey("object_type")
+->GetAsString()
+->GetValue();
+  if (!object_type.empty()) {
+result = Sprintf("Location is a %ld-byte %s object at 0x%llx", size,
+ object_type.c_str(), addr);
+  } else {
+result =
+Sprintf("Location is a %ld-byte heap object at 0x%llx", size, 
addr);
+  }
 } else if (type == "stack") {
   int tid = loc->GetAsDictionary()