dvyukov (Dmitry Vyukov)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 6 2012, 2:31 AM (228 w, 5 d)

Recent Activity

Today

dvyukov added a comment to D31630: [tsan] Detect races on modifying accesses in Swift code.

ThreadSanitizer: race on Swift array
ThreadSanitizer: race on Swift map

Unfortunately, we won't be able to tell the type of the variable (at least not now). I've updated the patch to say "race on Swift variable", but that's not really exact and it's not describing the problem well. Swift is getting exact threading rules that are very different from C/C++ and I'm trying to focus on making the reports readable and understandable by Swift users.

Is there a way we could still special-case the Swift reports? I understand the desire to keep the API generic, but I think we need to make sure the users understand the report. E.g. "Swift access race" much better describes what the problem is, and users words that define the Swift threading model.

Tue, Apr 25, 5:35 AM · Restricted Project
dvyukov added a comment to D31630: [tsan] Detect races on modifying accesses in Swift code.

Also, could swift runtime register a tag at startup? Or performance overhead is a concern?

It's quite hard to make the runtime call the registration at process startup, because it would need to dynamically figure out whether we're running with TSan or not, plus we'd need to add an indirection to each access (to read the value of the tag). I'd really prefer to keep the value as a well-defined constant in TSan.

Tue, Apr 25, 5:23 AM · Restricted Project
dvyukov added inline comments to D32382: [tsan] Track external tags in thread traces.
Tue, Apr 25, 5:21 AM · Restricted Project

Sun, Apr 23

dvyukov added reviewers for D32408: [ASAN] Add interceptor for __longjmp_chk: alekseyshl, vitalybuka.
Sun, Apr 23, 11:11 PM
dvyukov accepted D32402: Add missing acquire_load to call_once overload..
Sun, Apr 23, 10:06 AM

Sat, Apr 22

dvyukov accepted D32384: [tsan] Include __tsan_external_* API from a header file instead of declaring them manually.
Sat, Apr 22, 2:09 AM · Restricted Project
dvyukov accepted D32383: [tsan] Remove the extra word "object" from description of external races.
Sat, Apr 22, 1:59 AM · Restricted Project

Fri, Apr 21

dvyukov accepted D32365: [sanitizer] Cache SizeClassForTransferBatch in the 32-bit local cache.
Fri, Apr 21, 12:12 PM
dvyukov accepted D32360: [tsan] Refactor __tsan_external_read/__tsan_external_write to avoid code duplication.
Fri, Apr 21, 10:41 AM · Restricted Project
dvyukov accepted D32359: [tsan] Track external API accesses as 1-byte accesses (instead of 8-byte).
Fri, Apr 21, 10:30 AM · Restricted Project
dvyukov added a comment to D32310: [scudo] Bypass Quarantine if its size is set to 0.

which will be checked in due time.

Fri, Apr 21, 10:27 AM
dvyukov accepted D32358: [tsan] Publish the TSan external API in tsan_interface.h.
Fri, Apr 21, 10:24 AM · Restricted Project
dvyukov added a comment to D31630: [tsan] Detect races on modifying accesses in Swift code.

Hi Kuba,

I've seen a bunch of changes from you and they all are marked in my inbox. The past 1.5 weeks I was travelling and next week I am OOO so won't be able to review them. Just wanted to let you know.

I am back. Slowly working on backlog.

Fri, Apr 21, 10:17 AM · Restricted Project
dvyukov added inline comments to D31689: [tsan] Summary should point to "responsible caller" for external races.
Fri, Apr 21, 7:11 AM · Restricted Project
dvyukov added a comment to D31630: [tsan] Detect races on modifying accesses in Swift code.

We have 3 potential users for the external API (Go, Java, races on fd, potentially more in future).

Fri, Apr 21, 6:48 AM · Restricted Project
dvyukov added a comment to D31630: [tsan] Detect races on modifying accesses in Swift code.

I don't like that we sprinkle swift-specific bits throughout the code when the external API was meant to be general enough to cover all such cases.
We have the special tag for swift, so it really seems to me that it should be the only swift-specific bit here and the rest must be handled by the external API on general basis. We have 3 potential users for the external API (Go, Java, races on fd, potentially more in future). So I would be really interested in making the external API general and flexible enough to support all of them, rather than later sprinkle Go/Java/fd/etc-specific bits throughout the code again later.

Fri, Apr 21, 6:42 AM · Restricted Project
dvyukov accepted D31553: [tsan] Ignore memory accesses for libignored modules for "external" races.
Fri, Apr 21, 6:05 AM · Restricted Project
dvyukov added inline comments to D31684: [sanitizer] Fix various issues reported by Clang Static Analyzer [NFC].
Fri, Apr 21, 5:21 AM · Restricted Project
dvyukov accepted D31734: [tsan] Add a test for "external" API that checks the dup suppression is based on the caller PC.
Fri, Apr 21, 4:45 AM · Restricted Project
dvyukov accepted D31449: [tsan] Don't report bugs from interceptors called from libignored modules.
Fri, Apr 21, 4:45 AM · Restricted Project
dvyukov added a comment to D31729: [tsan] Mark "responsible frames" in stack traces to show which frame contains the race.

I am not completely follow motivation for this. Is it meant for user? Or for any automated tools?
For automated tools we already have SUMMARY line. User must be proficient enough to understand what happens. And in the end this is not specific to libc, and the guilty frame is not necessary the one just above libc. Consider a race in std::vector, it's probably not std::vector code that is guilty. And this extends just to any library, consider you have race in libfoo, but it's actually your code that misuses libfoo. We don't have enough info to provide precise signal. What we provide here is a weak, potentially incorrect signal.

Fri, Apr 21, 4:33 AM · Restricted Project
dvyukov accepted D32310: [scudo] Bypass Quarantine if its size is set to 0.

If we actually plan to use such configuration, does it make sense to check header for corruption when we reallocate a block (in allocate)? That will give us at least some windows for UAF detection.

Fri, Apr 21, 1:46 AM

Thu, Apr 20

dvyukov accepted D32299: [scudo] Remove GetActuallyAllocatedSize calls from the fast path.
Thu, Apr 20, 10:35 AM
dvyukov accepted D32242: [scudo] Minor changes and refactoring.

LGTM with a nit

Thu, Apr 20, 6:16 AM

Wed, Apr 19

dvyukov added a comment to D31630: [tsan] Detect races on modifying accesses in Swift code.

Hi Kuba,

I've seen a bunch of changes from you and they all are marked in my inbox. The past 1.5 weeks I was travelling and next week I am OOO so won't be able to review them. Just wanted to let you know.

Wed, Apr 19, 1:03 PM · Restricted Project
dvyukov added a comment to D31947: [scudo] Android support groundwork.

@dvyukov would it be acceptable to get this patch out in its current form and use this as a base for the further improvement you suggested?
Namely: improve contention, potential use of the tsan TLS slot, rework thread/global initialization.

Wed, Apr 19, 11:32 AM

Tue, Apr 18

dvyukov added a comment to D31947: [scudo] Android support groundwork.

First of all, do you have any benchmarks? Single-threaded, multi-threaded with/without contention. How do profiles look like?

I have shared with Aleksey some Flame profiles, I can send that to you as well. This ended up with the couple of optimizations he recently did.

Tue, Apr 18, 8:44 AM
dvyukov added inline comments to D30583: [Asan, Tsan] Generalize means the sanitizers work with memory.
Tue, Apr 18, 5:35 AM · Restricted Project
dvyukov added a comment to D31395: [Asan] Disable inline instrumentation for MMU-less targets.

Ivan, do you have commit access? Or I need to land it?

Tue, Apr 18, 5:04 AM · Restricted Project
dvyukov accepted D31395: [Asan] Disable inline instrumentation for MMU-less targets.

I was on vacation/ooo, sorry for delays.

Tue, Apr 18, 5:03 AM · Restricted Project
dvyukov added a comment to D31947: [scudo] Android support groundwork.

First of all, do you have any benchmarks? Single-threaded, multi-threaded with/without contention. How do profiles look like?

Tue, Apr 18, 4:37 AM
dvyukov added a comment to D31947: [scudo] Android support groundwork.

Android doesn't support thread_local

Tue, Apr 18, 2:34 AM

Fri, Apr 7

dvyukov added a comment to D31630: [tsan] Detect races on modifying accesses in Swift code.

Hi Kuba,

Fri, Apr 7, 1:54 PM · Restricted Project

Sun, Apr 2

dvyukov added a comment to D31449: [tsan] Don't report bugs from interceptors called from libignored modules.
  1. s/ignore_reads_and_writes/ignore_reads_writes_and_reports/ does not look like a good idea to me, because these things are not necessary related. If we go this route we need separate flags.
  2. From the description it seems that your intention is to ignore all reports, but you are ignoring only deadlocks. There are many more types of reports.
Sun, Apr 2, 3:08 AM · Restricted Project
dvyukov added inline comments to D31449: [tsan] Don't report bugs from interceptors called from libignored modules.
Sun, Apr 2, 3:03 AM · Restricted Project
dvyukov added a comment to D31553: [tsan] Ignore memory accesses for libignored modules for "external" races.

Wait, the external API was meant specifically to be used from non-instrumented libraries. Quoting you in D28836:

It might not be feasible/easy to recompile the library with TSan instrumentation... The idea of this patch is to allow a non-instrumented library to call into TSan runtime.

With this change it all become pointless. We annotate non-instrumented libraries and then ignore that. What am I missing?

Sun, Apr 2, 1:57 AM · Restricted Project

Thu, Mar 30

dvyukov added a comment to D31449: [tsan] Don't report bugs from interceptors called from libignored modules.

When an interceptor comes from a non-instrumented library, we set thr->in_ignored_lib=true. This causes SCOPED_TSAN_INTERCEPTOR to call the real function an skip all tsan processing. Consequently mutex lock/unlock operations in non-instrumented libraries are not handled by tsan already. So it's not clear to me how you get deadlock reports there. What am I missing?

Thu, Mar 30, 5:49 AM · Restricted Project
dvyukov accepted D31475: [tsan] Add interceptor for xpc_connection_cancel to avoid false positives.
Thu, Mar 30, 5:24 AM · Restricted Project

Tue, Mar 28

dvyukov added a comment to D31354: [tsan] Assert to make sure we don't try to Acquire() or Release() a NULL pointer.

I see that we already exclude the first page from app memory on x86_64:

static const uptr kLoAppMemBeg   = 0x000000001000ull;

So I think a better check would be:

CHECK(IsAppMem(addr));

It will also check for any other bogus addresses, like random values, NULL+epsilon (&null_ptr->field) and also will not duplicate mapping logic.

Tue, Mar 28, 1:11 AM

Mon, Mar 27

dvyukov accepted D31376: [sanitizers] Avoid using -fomit-frame-pointer on Darwin.
Mon, Mar 27, 6:56 AM
dvyukov added a comment to D31395: [Asan] Disable inline instrumentation for MMU-less targets.

You can use -asan-instrumentation-with-call-threshold=0 for this without any changes to llvm.

Mon, Mar 27, 6:40 AM · Restricted Project

Sun, Mar 26

dvyukov committed rL298809: tsan: add new mutex annotations.
tsan: add new mutex annotations
Sun, Mar 26, 8:43 AM
dvyukov closed D31093: tsan: add new mutex annotations.

Submitted in 298809.

Sun, Mar 26, 8:40 AM
dvyukov added inline comments to D31093: tsan: add new mutex annotations.
Sun, Mar 26, 8:38 AM
dvyukov updated the diff for D31093: tsan: add new mutex annotations.
Sun, Mar 26, 8:37 AM

Mar 26 2017

dvyukov accepted D31355: [tsan] Only Acquire/Release GCD queues if they're not NULL.
Mar 26 2017, 2:09 AM
dvyukov added a comment to D31354: [tsan] Assert to make sure we don't try to Acquire() or Release() a NULL pointer.

What was the bug? How did it manifest?
I somewhat afraid of breaking some existing code. Currently we consider memory around 0 to be "user memory". And in fact it's possible to mmap the first page at least on some linuxes, not sure about other OSes. And there is some effort to port sanitizers to mmu-less environment (https://reviews.llvm.org/D30583). Another possible use if you have N virtual entities and use N as address passed to acquire/release annotations (not perfect, but currently valid).
If we are excluding NULL from user memory, then I think we need to change IsAppMem to return false for NULL. That will make behavior consistent across all functions. Acquire/Release already check IsAppMem for the passed address (at least in debug build). It will also make simpler to undo this decision in some particular contexts.

Mar 26 2017, 2:05 AM

Mar 24 2017

dvyukov added a comment to D31093: tsan: add new mutex annotations.

PTAL

Mar 24 2017, 4:10 AM
dvyukov updated the diff for D31093: tsan: add new mutex annotations.
Mar 24 2017, 4:09 AM
dvyukov updated the diff for D31093: tsan: add new mutex annotations.
Mar 24 2017, 4:07 AM

Mar 23 2017

dvyukov added inline comments to D31093: tsan: add new mutex annotations.
Mar 23 2017, 1:57 AM

Mar 22 2017

dvyukov committed rL298492: tsan: fix a typo.
tsan: fix a typo
Mar 22 2017, 2:43 AM
dvyukov updated the diff for D31093: tsan: add new mutex annotations.
Mar 22 2017, 2:41 AM
dvyukov added a comment to D31093: tsan: add new mutex annotations.

Tests now have full coverage of annotations.

Mar 22 2017, 2:41 AM
dvyukov updated the diff for D31093: tsan: add new mutex annotations.

added more tests

Mar 22 2017, 2:39 AM

Mar 21 2017

dvyukov added a comment to D31093: tsan: add new mutex annotations.

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.)

Mar 21 2017, 10:56 AM
dvyukov added inline comments to D31093: tsan: add new mutex annotations.
Mar 21 2017, 10:46 AM
dvyukov updated the diff for D31093: tsan: add new mutex annotations.

addressed review comments

Mar 21 2017, 10:46 AM
dvyukov committed rL298383: tsan: fix pie_no_aslr test.
tsan: fix pie_no_aslr test
Mar 21 2017, 8:50 AM
dvyukov committed rL298378: tsan: support __ATOMIC_HLE_ACQUIRE/RELEASE flags.
tsan: support __ATOMIC_HLE_ACQUIRE/RELEASE flags
Mar 21 2017, 7:41 AM
dvyukov committed rL298372: tsan: add test for pie/no aslr.
tsan: add test for pie/no aslr
Mar 21 2017, 6:56 AM

Mar 17 2017

dvyukov added a comment to D31093: tsan: add new mutex annotations.

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.

Mar 17 2017, 12:16 PM
dvyukov created D31093: tsan: add new mutex annotations.
Mar 17 2017, 12:14 PM

Mar 13 2017

dvyukov added a comment to D30583: [Asan, Tsan] Generalize means the sanitizers work with memory.

Few suggestions if you want to upstream this:

  • remove unchanged files, unnecessary ifdefs and todo's
  • start with just asan (tsan is more complex)
  • use the existing asan compiler instrumentation that inserts function calls, and do the rest in runtime (at least in the first version, this will greatly simplify the change)
Mar 13 2017, 11:49 AM · Restricted Project

Mar 8 2017

dvyukov added inline comments to D30583: [Asan, Tsan] Generalize means the sanitizers work with memory.
Mar 8 2017, 5:02 AM · Restricted Project

Feb 16 2017

dvyukov accepted D30023: [tsan] Provide external tags (object types) via debugging API.
Feb 16 2017, 1:11 AM · Restricted Project

Feb 9 2017

dvyukov accepted D29728: Remove strict tid checks from the mac implementation of BlockingMutex.

LGTM if the comment is moved

Feb 9 2017, 10:36 AM
dvyukov added a comment to D29728: Remove strict tid checks from the mac implementation of BlockingMutex.

Kuba, this is a prerequisite for lsan on mac.

Feb 9 2017, 10:17 AM
dvyukov added a comment to D29728: Remove strict tid checks from the mac implementation of BlockingMutex.

Do you have commit access? Or you want me to commit after the comment move?

Feb 9 2017, 10:16 AM
dvyukov added a comment to D29728: Remove strict tid checks from the mac implementation of BlockingMutex.

Since we spent so much time discussion this, I guess you need to leave some traces in comments for future generations. Document what's CheckLocked contract and why it is that way.

Feb 9 2017, 2:12 AM

Feb 8 2017

dvyukov added a comment to D29588: Use rw locks for sanitizer thread registry.

SGTM

Feb 8 2017, 10:58 AM
dvyukov added inline comments to D29594: Add tid validation checks to blocking mutex and rw mutex.
Feb 8 2017, 8:15 AM
dvyukov added a comment to D29588: Use rw locks for sanitizer thread registry.

The contract for CheckLocked is: CheckLocked does nothing if the current thread holds the mutex, otherwise behavior is undefined.
And, yes, we are currently abusing the sloppy check in BlockingMutex on linux in CheckForLeaks.

Feb 8 2017, 8:04 AM

Feb 7 2017

dvyukov added a comment to D29588: Use rw locks for sanitizer thread registry.

Correct. We know that the data won't change in this particular case, because we know the parent isn't changing the data, and we know that our parent is holding the lock. However, this isn't what CheckLocked() is checking. CheckLocked() checks that *any* thread holds the lock, it doesn't need to be our parent to pass the check.

Feb 7 2017, 1:36 PM
dvyukov added a comment to D29588: Use rw locks for sanitizer thread registry.

Sharing a mutex access across threads like this is not supported by blocking mutex

Why?

If a thread owns a mutex on the thread registry for writing (as with a blocking mutex), we can't really assume in another thread that the registry won't change. Since the thread accessing the data is actually in a separate process (the result of a clone() in StopTheWorld), I'm not sure that there's even a way to pass the blocking mutex around safely.

I've checked BlockingMutex comments and linux/mac/win implementation. I don't see any indication that it's not supported.
How is RWMutex different?

An RWMutex acquired for read-only use can be safely used for reads by multiple threads, as it guarantees the protected data won't change. A blocking mutex provides no guarantees that the protected state won't change (and in fact implies that it probably will).

Feb 7 2017, 12:38 PM
dvyukov added a comment to D29594: Add tid validation checks to blocking mutex and rw mutex.

This adds 5 calls and 3 syscalls per rw mutex lock/unlock. What's the performance effect of this?

Feb 7 2017, 2:17 AM
dvyukov added a comment to D29588: Use rw locks for sanitizer thread registry.

Spin mutex can behave very poorly on contention, and thread registry can be contended. I suspect blocking mutex is there on purpose.

Feb 7 2017, 2:16 AM
dvyukov added a comment to D29588: Use rw locks for sanitizer thread registry.

Sharing a mutex access across threads like this is not supported by blocking mutex

Feb 7 2017, 2:01 AM

Feb 1 2017

dvyukov accepted D28836: [tsan] Provide API for libraries for race detection on custom objects.
Feb 1 2017, 6:17 AM · Restricted Project
dvyukov added a comment to D28836: [tsan] Provide API for libraries for race detection on custom objects.

Here's the output of the testcase from the patch (the "TEST3" scenario, which is the only one interesting). The stacks have some C++'ish noise in them due to the use of std::thread.

Feb 1 2017, 2:54 AM · Restricted Project
dvyukov accepted D29103: [tsan] Properly describe GCD worker threads in reports.

Thanks!
Now this also improves asan.

Feb 1 2017, 1:28 AM · Restricted Project

Jan 27 2017

dvyukov added inline comments to D28836: [tsan] Provide API for libraries for race detection on custom objects.
Jan 27 2017, 1:05 AM · Restricted Project
dvyukov added a comment to D28836: [tsan] Provide API for libraries for race detection on custom objects.

But please post full reports for the tests.

What exactly would you like to see? The TSan output for the lit test that's part of the patch? Or the TSan output for a more realistic use case?

Jan 27 2017, 1:03 AM · Restricted Project

Jan 25 2017

dvyukov added inline comments to D29103: [tsan] Properly describe GCD worker threads in reports.
Jan 25 2017, 7:09 AM · Restricted Project
dvyukov accepted D29106: [tsan] Fix os_id of main thread.
Jan 25 2017, 2:28 AM · Restricted Project

Jan 24 2017

dvyukov added a comment to D28836: [tsan] Provide API for libraries for race detection on custom objects.

I do already have a buy in from 2 major libraries on our side and we're in the talks with a 3rd library. The library owners did preliminary performance tests and they are fine with inserting those callbacks. Sorry for being vague here :)

Jan 24 2017, 11:13 AM · Restricted Project
dvyukov added a comment to D28836: [tsan] Provide API for libraries for race detection on custom objects.

We can't ship those libraries TSanified (performance, security) nor can we maintain interceptors for them (too many APIs, changing APIs, ...). At this point, it's not clear which libraries will adopt this

Jan 24 2017, 10:23 AM · Restricted Project
dvyukov added inline comments to D28836: [tsan] Provide API for libraries for race detection on custom objects.
Jan 24 2017, 7:59 AM · Restricted Project
dvyukov added a comment to D28836: [tsan] Provide API for libraries for race detection on custom objects.

Are they meant to be called from instrumented code or from non-instrumented code?

Jan 24 2017, 7:35 AM · Restricted Project
dvyukov added a comment to D28836: [tsan] Provide API for libraries for race detection on custom objects.

Do you plan to this functionality? Where? How?

Jan 24 2017, 7:31 AM · Restricted Project
dvyukov added a comment to D28836: [tsan] Provide API for libraries for race detection on custom objects.

Did not notice this before somehow. Sorry.

Jan 24 2017, 7:26 AM · Restricted Project
dvyukov accepted D29041: [tsan] Enable ignore_noninstrumented_modules=1 on Darwin by default.
Jan 24 2017, 7:12 AM · Restricted Project

Jan 12 2017

dvyukov added a comment to D25509: tsan: intercept libatomic.

One way to make this work on Darwin is by using the __asm trick:

Jan 12 2017, 10:34 AM
dvyukov updated the diff for D25509: tsan: intercept libatomic.
Jan 12 2017, 10:21 AM
dvyukov accepted D28586: ASAN activate/deactive controls thread_local_quarantine_size_kb option..
Jan 12 2017, 5:07 AM

Jan 11 2017

dvyukov added a comment to D25509: tsan: intercept libatomic.

Any news here?

No news yet. I wanted to get a darwin machine somewhere to figure out a working solution, but did not figure out where to get a good darwin machine yet... and then was paged out by other stuff.

I didn't want to block this patch, I only noticed that it doesn't build on macOS.

Well, that's serious enough reason to not submit it as is :)

What about declaring a new type of interception and using it just for the libatomic wrappers (and leave the existing interceptors as they are)?

We can, of course, resort to this.
But generally I prefer to have a single solution that works in all cases, rather than one solution that works for A but not for B + another that works for B but not for A.

How bad are darwin breakages? I mean while testing linux build I had to do changes like:

  • int res = vswprintf(str, size, format, ap); + int res = WRAP(vswprintf)(str, size, format, ap);
Jan 11 2017, 4:07 AM
dvyukov added a comment to D25509: tsan: intercept libatomic.

Any news here?

Jan 11 2017, 4:06 AM

Jan 10 2017

dvyukov accepted D28264: [tsan] Implement a 'ignore_noninstrumented_modules' flag to better suppress false positive races.

Thanks!

Jan 10 2017, 6:25 AM · Restricted Project

Jan 7 2017

dvyukov closed D28443: Introducing a function to flush the shadow memory from third-party libraries.
Jan 7 2017, 3:38 AM
dvyukov added a comment to D28443: Introducing a function to flush the shadow memory from third-party libraries.

Submitted in r291346.
Thanks

Jan 7 2017, 3:38 AM