This is an archive of the discontinued LLVM Phabricator instance.

tsan: add new mutex annotations
ClosedPublic

Authored by dvyukov on Mar 17 2017, 12:14 PM.

Details

Summary

There are several problems with the current annotations (AnnotateRWLockCreate and friends):

  • they don't fully support deadlock detection (we need a hook _before_ mutex lock)
  • they don't support insertion of random artificial delays to perturb execution (again we need a hook _before_ mutex lock)
  • they don't support setting extended mutex attributes like read/write reentrancy (only "linker init" was bolted on)
  • they don't support setting mutex attributes if a mutex don't have a "constructor" (e.g. static, Java, Go mutexes)
  • they don't ignore synchronization inside of lock/unlock operations which leads to slowdown and false negatives

The new annotations solve of the above problems. See tsan_interface.h for the interface specification and comments.

Diff Detail

Event Timeline

dvyukov created this revision.Mar 17 2017, 12:14 PM

I still need to look at this with fresh head next week, but it is ready for first review pass.
Need to add more tests. It's not that I did not test, I've worked out interface and tested on private programs.

kubamracek edited edge metadata.Mar 17 2017, 1:18 PM

Sounds like a great improvement, especially the introduction of a public tsan_interface.h header file.

I didn't test the patch yet, but do I understand correctly that this now detects actual deadlocks and reports them via ReportDeadlock? Should we change the message in that case from "potential deadlock" to "deadlock detected"? Can we add a test for it? (Doesn't need to be part of this patch, though.)

lib/tsan/rtl/tsan_interceptors.cc
1117

"flags" instead?

2268

remove the last argument (0)

lib/tsan/rtl/tsan_interface_ann.cc
273

This is hard to read/understand (function is "PostLock" but uses "PreLock" flag)... Could we rename the flag to more precisely indicate what it does? E.g. something like "MutexFlagPreLockNotCalled" or "MutexFlagIncludePreLockSemantics". Is the only use of this flag allowed in MutexPostLock? Then what about "MutexFlagPerformBothPreAndPostLock". Or maybe this even deserves its own function?

464

flag -> flags

ditto several times below and in other files

lib/tsan/rtl/tsan_rtl.h
738–748

flag -> flags

lib/tsan/rtl/tsan_rtl_mutex.cc
317–318

Can we rename FlagSet to "IsFlagSet" to avoid confusion with "SetFlags"?

lib/tsan/rtl/tsan_stat.cc
156–157

Is this line supposed to be twice here?

lib/tsan/rtl/tsan_sync.h
26–28

Could we just #include tsan_interface.h and use the constants directly to avoid duplication?

dvyukov updated this revision to Diff 92512.Mar 21 2017, 10:46 AM
dvyukov marked 4 inline comments as done.

addressed review comments

lib/tsan/rtl/tsan_interceptors.cc
1117

Here and in other places 'flags' conflicts with the flags() function.

lib/tsan/rtl/tsan_interface_ann.cc
273

Renamed to MutexFlagDoPreLockOnPostLock

464

ditto

lib/tsan/rtl/tsan_sync.h
26–28

I've tried that and it leads to compilation failures as the interface header transitively includes some standard headers, and tsan runtime does not allow inclusion of standard headers.

Sounds like a great improvement, especially the introduction of a public tsan_interface.h header file.

I didn't test the patch yet, but do I understand correctly that this now detects actual deadlocks and reports them via ReportDeadlock? Should we change the message in that case from "potential deadlock" to "deadlock detected"? Can we add a test for it? (Doesn't need to be part of this patch, though.)

Yes, now we can also report actual deadlocks. But I would expect that majority of deadlocks that tsan will report does not actually happen (just cycles in mutex graph). And false positives are still possible, e.g. when a cycle between A<->B is protected by a third mutex C, so the deadlock can't actually happen. So I think we need to leave "potential deadlock".
There is a disabled test for actual deadlock in test/tsan/must_deadlock.cc. But unfortunately it still does not work. Deadlock detector v1 does not detect it at all. Deadlock detector v2 detects it, but provides some incorrect data in report so it CHECK-fails. The deadlock detection wasn't ever completely finished. It still needs more work. And this change does not completely fix it, it just an incremental step towards fully working deadlock detection.
But the part that I have pressing need for is the ignores in the new annotations. That's the main motivator for this change.

kcc edited edge metadata.

+Aleksey (but I'll look myself too)
At high level looks great!

dvyukov updated this revision to Diff 92608.Mar 22 2017, 2:39 AM

added more tests

Tests now have full coverage of annotations.

dvyukov updated this revision to Diff 92610.Mar 22 2017, 2:41 AM
alekseyshl added inline comments.Mar 22 2017, 2:39 PM
lib/tsan/rtl/tsan_interceptors.cc
1117

"flagz" maybe?

dvyukov added inline comments.Mar 23 2017, 1:57 AM
lib/tsan/rtl/tsan_interceptors.cc
1117

Is it significantly better than flag?

alekseyshl added inline comments.Mar 23 2017, 10:50 AM
lib/tsan/rtl/tsan_interceptors.cc
1117

Well, it expresses the semantics of all those vars and params much better (since flag does not express potential multiplicity at all).

lib/tsan/rtl/tsan_sync.h
26–28

Nit: maybe add the corresponding constants as comments so that code search can help with the connection?

MutexFlagLinkerInit = 1 << 0, // == __tsan_mutex_linker_init
...

93

It's a bit unclear why something called MutexCreationFlagMask is of such importance when we just UpdateFlags. I admit I know very little about the code and curious to know more. So, UpdateFlags is not a generic flag operation?

kubamracek added inline comments.Mar 23 2017, 1:44 PM
lib/tsan/rtl/tsan_interface_ann.cc
540–541

Why is __tsan_mutex_pre_divert calling ThreadIgnore(Sync)End? Shouldn't it be calling ThreadIgnore(Sync)Begin to mark the beginning of an ignored region? I guess I don't understand this feature.

lib/tsan/rtl/tsan_stat.cc
156

extra comma

dvyukov updated this revision to Diff 92917.Mar 24 2017, 4:07 AM
dvyukov marked an inline comment as done.
dvyukov updated this revision to Diff 92918.Mar 24 2017, 4:09 AM

PTAL

lib/tsan/rtl/tsan_interceptors.cc
1117

Done

lib/tsan/rtl/tsan_interface_ann.cc
540–541

I've added comment here and extended comment on this function in tsan_interface.h. Also see usage in the tests.
Is it clearer now?

lib/tsan/rtl/tsan_stat.cc
156

Done

lib/tsan/rtl/tsan_sync.h
26–28

Done

93

MutexFlag* flags contain both flags that characterize a mutex and flags that characterize an operation on a mutex. SyncVar::flags only holds flags that characterize the mutex. So we filter out flags that characterize a particular operations that calls UpdateFlags.

alekseyshl accepted this revision.Mar 24 2017, 2:17 PM
alekseyshl added inline comments.
lib/tsan/rtl/tsan_sync.h
93

Ah, I see, thank you! Please add the comment to MutexCreationFlagMask definition, that all flags that are not operation flags should be mentioned there.

This revision is now accepted and ready to land.Mar 24 2017, 2:17 PM
dvyukov updated this revision to Diff 93072.Mar 26 2017, 8:37 AM
dvyukov added inline comments.
lib/tsan/rtl/tsan_sync.h
93

Okay. Done.

dvyukov closed this revision.Mar 26 2017, 8:40 AM

Submitted in 298809.