[PATCH] D53171: [tsan] Bring Dispatch support to Linux

[Dan Liew via Phabricator via llvm-commits]

delcypher requested changes to this revision.
delcypher added a comment.
This revision now requires changes to proceed.

This looks very promising. However I have some minor nits and some concerns regarding the use of system header files.



================
Comment at: lib/tsan/rtl/tsan_libdispatch.cc:1
 //===-- tsan_libdispatch_mac.cc -------------------------------------------===//
 //
----------------
This comment is out-of-date. The file name is different now.


================
Comment at: lib/tsan/rtl/tsan_libdispatch.cc:12
 //
 // Mac-specific libdispatch (GCD) support.
 //===----------------------------------------------------------------------===//
----------------
This comment is out-of-date.  This isn't mac specific anymore.


================
Comment at: lib/tsan/rtl/tsan_libdispatch.cc:23
 
 #include <Block.h>
 #include <pthread.h>
----------------
Should the `Block.h` include be here? I'm pretty sure that's not a standard header file on Linux.


================
Comment at: lib/tsan/rtl/tsan_libdispatch.cc:25
 #include <pthread.h>
+#include <stdint.h>
+#include <sys/types.h>
----------------
Perhaps you could have a comment here explaining that these declarations are to avoid depending the libdispatch header files? If you think these stubs might be needed elsewhere we could also move them into their own header file.


================
Comment at: lib/tsan/rtl/tsan_libdispatch.cc:25
 #include <pthread.h>
+#include <stdint.h>
+#include <sys/types.h>
----------------
delcypher wrote:
> Perhaps you could have a comment here explaining that these declarations are to avoid depending the libdispatch header files? If you think these stubs might be needed elsewhere we could also move them into their own header file.
I'm not sure if including system headers such as `pthread.h`, `stdint.h` and `sys/types.h` is a good idea.

In `sanitizer_interals_defs.h` there is this comment.

```
// For portability reasons we do not include stddef.h, stdint.h or any other
// system header, but we do need some basic types that are not defined
// in a portable way by the language itself.
```

this suggests to me that we probably should avoid including these headers files and instead use the equivalent definitions in the sanitizer's own internal header files.


================
Comment at: lib/tsan/rtl/tsan_libdispatch.cc:211
 #define DISPATCH_INTERCEPT_B(name, barrier)                                  \
-  TSAN_INTERCEPTOR(void, name, dispatch_queue_t q, dispatch_block_t block) { \
+  DEFINE_REAL(void, name, dispatch_queue_t q, dispatch_block_t block);       \
+  DECLARE_WRAPPER(void, name, dispatch_queue_t q, dispatch_block_t block);   \
----------------
It's not clear to me why `TSAN_INTERCEPTOR` has been replaced with several other macros. If `TSAN_INTERCEPTOR` doesn't work could we provide another macro in this file just to avoid repeating ourselves along with a comment briefly saying why TSAN_INTERCEPTOR doesn't work here?


================
Comment at: lib/tsan/rtl/tsan_libdispatch.cc:787
 
-}  // namespace __tsan
+void InitializeLibdispatchInterceptors() {
+  INTERCEPT_FUNCTION(dispatch_async_f);
----------------
Who is responsible for calling this function? I was expecting to see some code in TSan's init that checks if this function is available and then calls it if it is. However I can't see anything like that in this patch.


https://reviews.llvm.org/D53171