Don't assume PTHREAD_CREATE_JOINABLE is 0 on all systems

[fjricci]

Summary

Lsan was using PTHREAD_CREATE_JOINABLE/PTHREAD_CREATE_DETACHED
as truthy values, which works on Linux, where the values are 0 and 1,
but this fails on OS X, where the values are 1 and 2.

Set PTHREAD_CREATE_DETACHED to the correct value for a given system.

[alekseyshl]

lib/lsan/lsan_interceptors.cc

61 ↗

(On Diff #94668)

Why do we have to define them? why not use the proper definition in phtread.h?

283 ↗

(On Diff #94668)

int detached = PTHREAD_CREATE_JOINABLE; to be consistent with the rest of your change.

[fjricci]

lib/lsan/lsan_interceptors.cc

61 ↗

(On Diff #94668)

We don't want to include any system headers in this file.

283 ↗

(On Diff #94668)

This review established that we don't want to define PTHREAD_CREATE_JOINABLE (that value will get over-written by pthread_attr_getdetachstate anyway): D10606

[alekseyshl]

lib/lsan/lsan_interceptors.cc

61 ↗

(On Diff #94668)

Oh, we already have those defined in tsan... Let's move the definition to saniter_linux.h and sanitizer_mac.h, define them as enum (yes, I'd still vote for defining PTHREAD_CREATE_JOINABLE, I don't see the upside of not defining it) and reuse them in both tsan and lsan.

283 ↗

(On Diff #94668)

pthread_attr_getdetachstate returns int and can fail, we're ignoring the error and go with a default value. I admit, it does not change the result of detached == PTHREAD_CREATE_DETACHED check, but does not feel like a sound practice.

[fjricci]

Address comments

[alekseyshl]

lib/sanitizer_common/sanitizer_linux.h

59 ↗

(On Diff #94866)

I'd prefer them to be an anonymous enum:

enum {

PTHREAD_CREATE_JOINABLE = 0,
PTHREAD_CREATE_DETACHED = 1

}

Preprocessor is evil.

[fjricci]

So I think we may have to return to the original method of defining the value in the source files. sanitizer_mac.h is included in files that also include pthread.h, which causes a multiple definition error. We can't use an ifndef, because the pthreads header isn't necessarily included first. If we want to keep these in sanitizer_mac and sanitizer_linux, we'll need to rename them to something that won't conflict with the pthreads name. Thoughts?

[fjricci]

Update to avoid conflicts with definitions from pthreads header

It doesn't look like we have any existing uses of the const-type constants
in platform-specific headers currently though, so I'm not sure that this is
better than the original solution of defining inside the source file.

[kubamracek]

Can we use the original names instead of kCreateJoinable and kCreateDetached? The name "kCreateDetached" suggests that it's some arbitrary sanitizer-internal constant, but that's not true. It's a platform-defined value that we must follow.

[fjricci]

In D31883#724975, @kubamracek wrote:

Can we use the original names instead of kCreateJoinable and kCreateDetached? The name "kCreateDetached" suggests that it's some arbitrary sanitizer-internal constant, but that's not true. It's a platform-defined value that we must follow.

The problem with the original name is that we hit conflicts in files that include both 'sanitizer_linux.h' and 'pthread.h', due to a multiple definition.

[fjricci]

In D31883#724977, @fjricci wrote:

In D31883#724975, @kubamracek wrote:

Can we use the original names instead of kCreateJoinable and kCreateDetached? The name "kCreateDetached" suggests that it's some arbitrary sanitizer-internal constant, but that's not true. It's a platform-defined value that we must follow.

The problem with the original name is that we hit conflicts in files that include both 'sanitizer_linux.h' and 'pthread.h', due to a multiple definition.

Well, not technically a multiple definition, but after the preprocessor replaces PTHREAD_CREATE_JOINABLE, you end up with a statement like '#define 0 = 0` in whichever header is included second, which is an error.

[alekseyshl]

Hmm, what about this then, define bool IsThreadAttached wrapper function in sanitizer_common/sanitizer_{linux|mac}, the same way we do for GetTid(). Definitions from pthread.h are available there, feels like a cleaner solution.

[fjricci]

Add wrapper function for checking thread detached state

[fjricci]

Fix typo

[alekseyshl]

lib/lsan/lsan_interceptors.cc

289 ↗

(On Diff #95016)

What about !SANITIZER_POSIX case? Who's going to provide this function?

[fjricci]

lib/lsan/lsan_interceptors.cc

289 ↗

(On Diff #95016)

Lots of other things in this function will also break in the !SANITIZER_POSIX case - pthread_attr_getdetachstate, pthread_create, etc. I don't think this file is currently designed to work at all in non-posix cases.

[alekseyshl]

Other than tsan_interceptors.cc question, lgtm.

lib/lsan/lsan_interceptors.cc

289 ↗

(On Diff #95016)

Let's remove #if SANITIZER_POSIX around include then, it's misleading.

What about tsan_interceptors.cc? Is it also posix only?

[fjricci]

lib/lsan/lsan_interceptors.cc

289 ↗

(On Diff #95016)

Yeah, tsan is also posix only, and there is definitely quite a bit of posix-specific code in that file. Interestingly, they do use the SANITIZER_POSIX guard on the header inclusion in that file, perhaps for future-proofing?

[alekseyshl]

lib/lsan/lsan_interceptors.cc

289 ↗

(On Diff #95016)

I don't know why. It's trivial to add back and in case of future changes, the failure is more explicit when there's no ifdef around the include.

[fjricci]

lib/lsan/lsan_interceptors.cc

289 ↗

(On Diff #95016)

Ok, sounds good. I'll remove it from the tsan file as well then, for consistency.

compiler-rt/trunk/lib/lsan/lsan_interceptors.cc

 #include "sanitizer_common/sanitizer_allocator.h"
 #include "sanitizer_common/sanitizer_atomic.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_internal_defs.h"
 #include "sanitizer_common/sanitizer_linux.h"
 #include "sanitizer_common/sanitizer_platform_interceptors.h"
 #include "sanitizer_common/sanitizer_platform_limits_posix.h"
+#include "sanitizer_common/sanitizer_posix.h"
 #include "sanitizer_common/sanitizer_tls_get_addr.h"
 #include "lsan.h"
 #include "lsan_allocator.h"
 #include "lsan_common.h"
 #include "lsan_thread.h"
 
 #include <stddef.h>
 
int res;
     // Ignore all allocations made by pthread_create: thread stack/TLS may be
     // stored by pthread for future reuse even after thread destruction, and
     // the linked list it's stored in doesn't even hold valid pointers to the
     // objects, the latter are calculated by obscure pointer arithmetic.
     ScopedInterceptorDisabler disabler;
     res = REAL(pthread_create)(th, attr, __lsan_thread_start_func, &p);
   }
   if (res == 0) {
-    int tid = ThreadCreate(GetCurrentThread(), *(uptr *)th, detached);
+    int tid = ThreadCreate(GetCurrentThread(), *(uptr *)th,
+                           IsStateDetached(detached));
     CHECK_NE(tid, 0);
     atomic_store(&p.tid, tid, memory_order_release);
     while (atomic_load(&p.tid, memory_order_acquire) != 0)
       internal_sched_yield();
   }
   if (attr == &myattr)
     pthread_attr_destroy(&myattr);
   return res;

compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h

 // order for internal_sigaction() to bypass interceptors.
 int internal_sigaction(int signum, const void *act, void *oldact);
 void internal_sigfillset(__sanitizer_sigset_t *set);
 void internal_sigemptyset(__sanitizer_sigset_t *set);
 bool internal_sigismember(__sanitizer_sigset_t *set, int signum);
 
 uptr internal_execve(const char *filename, char *const argv[],
                      char *const envp[]);
+
+bool IsStateDetached(int state);
+
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_POSIX_H

compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix_libcdep.cc

int WaitForProcess(pid_t pid) {
   int local_errno;
   if (internal_iserror(waitpid_status, &local_errno)) {
     VReport(1, "Waiting on the process failed (errno %d).\n", local_errno);
     return -1;
   }
   return process_status;
 }
 
+bool IsStateDetached(int state) {
+  return state == PTHREAD_CREATE_DETACHED;
+}
+
 } // namespace __sanitizer
 
 #endif // SANITIZER_POSIX

compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc

 // sanitizer_common/sanitizer_common_interceptors.inc
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_common/sanitizer_atomic.h"
 #include "sanitizer_common/sanitizer_libc.h"
 #include "sanitizer_common/sanitizer_linux.h"
 #include "sanitizer_common/sanitizer_platform_limits_posix.h"
 #include "sanitizer_common/sanitizer_placement_new.h"
+#include "sanitizer_common/sanitizer_posix.h"
 #include "sanitizer_common/sanitizer_stacktrace.h"
 #include "sanitizer_common/sanitizer_tls_get_addr.h"
 #include "interception/interception.h"
 #include "tsan_interceptors.h"
 #include "tsan_interface.h"
 #include "tsan_platform.h"
 #include "tsan_suppressions.h"
 #include "tsan_rtl.h"
 #include "tsan_mman.h"
 #include "tsan_fd.h"
 
-#if SANITIZER_POSIX
-#include "sanitizer_common/sanitizer_posix.h"
-#endif
 
 using namespace __tsan;  // NOLINT
 
 #if SANITIZER_FREEBSD || SANITIZER_MAC
 #define __errno_location __error
 #define stdout __stdoutp
 #define stderr __stderrp
 #endif
 
 #if SANITIZER_ANDROID
 #define __errno_location __errno
 #define mallopt(a, b)
 #endif
 
-#if SANITIZER_LINUX || SANITIZER_FREEBSD
-#define PTHREAD_CREATE_DETACHED 1
-#elif SANITIZER_MAC
-#define PTHREAD_CREATE_DETACHED 2
-#endif
-
-
 #ifdef __mips__
 const int kSigCount = 129;
 #else
 const int kSigCount = 65;
 #endif
 
 struct my_siginfo_t {
   // The size is determined by looking at sizeof of real siginfo_t on linux.
TSAN_INTERCEPTOR(int, pthread_create,
   {
     // Otherwise we see false positives in pthread stack manipulation.
     ScopedIgnoreInterceptors ignore;
     ThreadIgnoreBegin(thr, pc);
     res = REAL(pthread_create)(th, attr, __tsan_thread_start_func, &p);
     ThreadIgnoreEnd(thr, pc);
   }
   if (res == 0) {
-    int tid = ThreadCreate(thr, pc, *(uptr*)th,
-                           detached == PTHREAD_CREATE_DETACHED);
+    int tid = ThreadCreate(thr, pc, *(uptr*)th, IsStateDetached(detached));
     CHECK_NE(tid, 0);
     // Synchronization on p.tid serves two purposes:
     // 1. ThreadCreate must finish before the new thread starts.
     //    Otherwise the new thread can call pthread_detach, but the pthread_t
     //    identifier is not yet registered in ThreadRegistry by ThreadCreate.
     // 2. ThreadStart must finish before this thread continues.
     //    Otherwise, this thread can call pthread_detach and reset thr->sync
     //    before the new thread got a chance to acquire from it in ThreadStart.