[PATCH] D32382: [tsan] Track external tags in thread traces

Kuba (Brecka) Mracek via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 25 13:53:05 PDT 2017


kubamracek marked an inline comment as done.
kubamracek added inline comments.


================
Comment at: lib/tsan/rtl/tsan_rtl_report.cc:669
+    VarSizeStackTrace trace;
+    trace.Init(traces[i].trace, traces[i].size);
+    uptr tag = RetrieveTagFromStackTrace(&trace);
----------------
dvyukov wrote:
> I think it's better to separate stack and tag directly in ObtainCurrentStack and RestoreStack because:
> 1. We will not need to copy traces here (which is ugly and confusing at first glance).
> 2. No chance that the fake PCs pop up somewhere else because we forgot to remove them.
> 3. Racy stacks will be more difficult to break (currently a future change to this function can lead to a situation when we remember a stripped stack but check against non-stripped).
> 4. It will be easier to figure out report type without creating ScopedReport first and then fixing up the type with SetType.
Done.


================
Comment at: lib/tsan/rtl/tsan_rtl_report.cc:672
+    if (tag != kExternalTagNone) {
+      rep.SetType(ReportTypeExternalRace);
+    }
----------------
dvyukov wrote:
> I wonder if we need ReportTypeExternalRace at all.
> We could always use ReportTypeRace, but attach tag to the report instead of memory accesses. Then we don't need SetType and PrintReport will be able to specialize report header with e.g. "data race on curl handle", because then it will know that the whole report is about an external access.
> What do you think?
I'd still like to keep it.  There's at least the debugger support that needs to be able to distinguish an "external" race from a regular one.


https://reviews.llvm.org/D32382





More information about the llvm-commits mailing list