Page MenuHomePhabricator

[tsan] Add TSan debugger APIs
ClosedPublic

Authored by kubamracek on Jan 14 2016, 8:26 AM.

Details

Summary

Currently, TSan only reports everything in a formatted textual form. The idea behind this patch is to provide a consistent API that can be used to query information contained in a TSan-produced report. User can use these APIs either in a debugger (via a script or directly), or they can use it directly from the process (e.g. in the __tsan_on_report callback). ASan already has a similar API, see http://reviews.llvm.org/D4466.

Some details about the patch:

  • The API is in C (and not C++), because we want it simple and stable.
  • The callback __tsan_on_report is called just after the report is printed to the console.
  • Added a test case which uses the callback __tsan_on_report to query and verify various data from the report.

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 44888.Jan 14 2016, 8:26 AM
kubamracek retitled this revision from to [tsan] Add TSan debugger APIs.
kubamracek updated this object.
kubamracek added reviewers: dvyukov, samsonov, glider, kcc.
kubamracek added subscribers: llvm-commits, zaks.anna.
dvyukov edited edge metadata.Jan 15 2016, 1:02 AM

What do you want to achieve?
__tsan::OnReport already exposes ReportDesc. And it is enough to do the checks.
Long time ago we had lots of tests that did exactly this:

assert(tid == 0);
assert(addr == &my_global);
assert(size == 8);
assert(write == 1);
assert(atomic == 0);
assert(trace[0] > (void *)&main);
assert(trace[0] < (void *)&__tsan_on_report);
assert(trace[1] == 0);

They proved to be very painful to write and to maintain. And in the end they don't actually verify what uses sees (e..g all checks can pass but we print nothing at all). I've dropped them in favor of output_tests. I've spent half an hour trying to find them, but unfortunately it seems that we've dropped our history and masked it with file copies without using svn cp.

What do you want to achieve?
__tsan::OnReport already exposes ReportDesc. And it is enough to do the checks.

Exposing ReportDesc is hard, because its definition requires a lot of header files, the structure is complex, and it's not stable. I want the user to be able to use this API in a binary distribution of Clang, where we don't have TSan internal headers. I want to provide API so the user can do something like:

__tsan_on_report(void *report) {
  void *addr = ...; // retrieve the address on which a race was detected
  my_logging(..., addr); // do whatever I want with the report data
}
int main() {
  ... the program I'm testing with TSan ...
}

Long time ago we had lots of tests that did exactly this:

assert(tid == 0);
assert(addr == &my_global);
assert(size == 8);
assert(write == 1);
assert(atomic == 0);
assert(trace[0] > (void *)&main);
assert(trace[0] < (void *)&__tsan_on_report);
assert(trace[1] == 0);

They proved to be very painful to write and to maintain. And in the end they don't actually verify what uses sees (e..g all checks can pass but we print nothing at all). I've dropped them in favor of output_tests. I've spent half an hour trying to find them, but unfortunately it seems that we've dropped our history and masked it with file copies without using svn cp.

Are you against asserts or against the API? If the first, I can print all the stuff in the test and verify it with // CHECK:.

I want to provide API so the user can do something like:

Why? What's the value and end goal of this?

I want to provide API so the user can do something like:

Why? What's the value and end goal of this?

Debugger integration and programmatic access to structured data (to avoid parsing of the text output) for various purposes (report post-processing, tracking, aggregation, ...).

Debugger integration and programmatic access to structured data (to avoid parsing of the text output) for various purposes (report post-processing, tracking, aggregation, ...).

Debugger integration and programmatic access to structured data (to avoid parsing of the text output) for various purposes (report post-processing, tracking, aggregation, ...).

Who and how is going to use this?
Without knowing the answer, I don't know whether we export the right info in the right form. E.g. I don't see how mutex count is useful for report post-processing, tracking, aggregation.

> Debugger integration and programmatic access to structured data (to avoid parsing of the text output) for various purposes (report post-processing, tracking, aggregation, ...).
Who and how is going to use this?

I'd like to use this in the same way AddressSanitizer reports are integrated into the debugger (LLDB), see http://reviews.llvm.org/D4466. The idea is to provide all the information in a TSan report via a structured API, so that LLDB can query it. The end goal might be to simply present this information in a GUI debugger (that uses LLDB) and/or to show all TSan-produced warnings in some user-friendly way - let's say a table with filtering options. I don't want to limit the options here, if someone wants to generate XML reports of TSan warnings, this should be possible. That's why I'm proposing a very generic API that provides almost all the report information.

kubamracek updated this revision to Diff 46279.Jan 28 2016, 8:24 AM
kubamracek edited edge metadata.

Changing the lit test to use // CHECK: instead of asserts.

So this is for lldb.
I though that you want to write tsan tests in such style using __tsan_on_report callback.
lldb is fine.

lib/tsan/rtl/tsan_debugging.cc
58

Do we need this function?
User already gets report pointer as argument to __tsan_on_report. So this function looks unnecessary and subject to calls at a wrong time, and provoking dangling cur_thread()->current_report (as I can see below).
I would remove it.

64

Argument set for this function looks somewhat strange.
I would either move sleep_trace to a separate function (sleep trace is very auxiliary). Or make this function returns as much assorted data as possible (e.g. also return number of stacks, number of locations, etc).

lib/tsan/rtl/tsan_rtl_report.cc
51

I afraid this will break some of out internal setups which define TSAN_EXTERNAL_HOOKS and define OnReport function. They don't define __tsan_on_report, so they will break on next llvm update.

Define it as:

SANITIZER_WEAK_DEFAULT_IMPL
void __tsan_on_report(const ReportDesc *rep) {

(void)rep;

}

outside of #ifdef/else/endif TSAN_EXTERNAL_HOOKS.

522

Forget to reset thr->current_report here.

test/tsan/debugging.cc
45

I guess you wanted to print result of __tsan_get_current_report() here.

kubamracek updated this revision to Diff 50281.Mar 10 2016, 8:21 AM

Addressing review comments.

Do we need this function? (__tsan_get_current_report)

It's very convenient to use it from LLDB. Even when we break inside __tsan_on_report, the function argument is not easily accessible (it might be optimized out).

dvyukov accepted this revision.Mar 10 2016, 8:36 AM
dvyukov edited edge metadata.

LGTM with nits

lib/tsan/rtl/tsan_debugging.cc
58

Please add a comment that this is meant to be manually called from a debugger.

lib/tsan/rtl/tsan_rtl_report.cc
500

I think it makes sense to do:

CHECK_EQ(thr->current_report, nullptr);

here. That would catch the bug below and any future similar bugs.

This revision is now accepted and ready to land.Mar 10 2016, 8:36 AM
Closed by commit rL263126: [tsan] Add TSan debugger APIs (authored by kuba.brecka). · Explain WhyMar 10 2016, 9:05 AM
This revision was automatically updated to reflect the committed changes.