Hi,
TSan's race detection algorithm can be applied generally to anything where modification disallows other simulatenous accesses. Data races on memory locations are the common case, but we also already use the same algorithm to track races on file descriptors. This patch extends race detection and proposes to provide an interface that would allow libraries/framework to track races on objects that are exposed via API. If a mutable object is accessible via some API, it's very common that the threading rules of the API follow TSan's standard race rules. Example:
ArchiverRef ArchiverCreateNew(); void ArchiverAddFile(ArchiverRef ref, char *filename, char *data, size_t len); void ArchiverRemoveAllFiles(ArchiverRef ref); long ArchiverGetNumberOfFiles(ArchiverRef ref); char * ArchiverGetFileData(ArchiverRef ref, char *filename, size_t *out_len);
A reasonable threading contract would be that "read-only" methods can be called simulatenously from multiple threads (for one ArchiverRef handle), while modifying methods must be exclusive.
One option to detect violations of this API contract is to TSanify the library itself, but this has some downsides:
- The detected race will be on some internal state variable, possibly several functions deep (but the real bug is that user's code is breaking the API contract).
- The bug might not be detected at all, because the two APIs just happen not to touch the same memory. It's still a misuse of the API, and something we want to report.
- It might not be feasible/easy to recompile the library with TSan instrumentation.
Another option is to add interceptors for the APIs of the library. However, interceptors introduce a dependency, which is undesirable for high-level libraries, and we want TSan to link against as few libraries as possible. Secondly, maintaining a large list of interceptors for various libraries is hard and error-prone.
The idea of this patch is to allow a non-instrumented library to call into TSan runtime, which would be weakly (dynamically) linked (if TSan is not loaded into the process, this won't do anything). The library would tell TSan about "readonly" and "modifying" accesses to an "object" (which simply translate to a read/write of the object's pointer) and provide the caller and tag (type of object):
static void *tag_Archiver = __tsan_register_tag("MyLibrary", "Archiver");
void ArchiverAddFile(ArchiverRef ref, ...) {
__tsan_external_write(ref, CALLER_PC, tag_Archiver);
...
}
long ArchiverGetNumberOfFiles(ArchiverRef ref) {
__tsan_external_read(ref, CALLER_PC, tag_Archiver);
...
}The caller and tag are used to generate a user-friendly report:
==================
WARNING: ThreadSanitizer: race on a library object
Mutating access of object Archiver from library MyLibrary at 0x7b04000000f0 by thread T6:
#0 ArchiverAddFile (archiver.dylib+...)
#1 UserCodeFunc1 (usercode+...)
Previous read-only access of object Archiver from library MyLibrary at 0x7b04000000f0 by thread T5:
#0 ArchiverAddFile (archiver.dylib+...)
#1 UserCodeFunc2 (usercode+...)
Location is Archiver object from library MyLibrary of size 16 at 0x7b04000000f0 allocated by main thread:
#0 malloc (...)
#1 ArchiverCreateNew (archiver.dylib+...)
#2 main (usercode+...)
...So, the proposal here is to add these functions to TSan's interface:
void *__tsan_external_register_tag(char *library_name, char *object_type); void __tsan_external_assign_tag(void *addr, void *tag); void __tsan_external_read(void *addr, void *caller_pc, void *tag); void __tsan_external_write(void *addr, void *caller_pc, void *tag);
The __tsan_external_register_tag function creates a new tag, __tsan_external_assign_tag marks a heap-allocated memory chunk with the specified tag (this is to identify the object in the report). The __tsan_external_read/__tsan_external_read calls are basically the same as __tsan_read8/__tsan_write8, but they identify that the call isn't from the instrumentation, but from a library.
Another common threading model in library API is that a object handle must only be used from a single thread "at any given time" (but may be passed to another thread). This can be enforced by simply treating all APIs as modifying methods.
This does not need to be in header.
p.s. consts are static by default