Update suspended threads info to be compatible with darwin

[fjricci]

Summary

On Darwin, we need to track thread and tid as separate values.
This patch modifies suspended thread info to be more general, to
allow for this behavior.

[kubamracek]

lib/sanitizer_common/sanitizer_stoptheworld_mac.cc

55–63

Does this return the same value as pthread_threadid_np? Can we use it instead? Or even better, use GetTid from sanitizer_mac.cc? Calling thread_info here sounds very expensive.

Why not store the thread_id's in the array directly? If we really need to know both thread_t and thread_id, then we should have a struct that contains both and store the struct in the array.

[fjricci]

lib/sanitizer_common/sanitizer_stoptheworld_mac.cc

55–63

I think it's reasonable to use a struct instead. There is a fair amount of existing code that requires the thread_id, but the code for accessing register info will require the thread_t, so there's definitely a need for both, especially if it's expensive to convert.

[kubamracek]

lib/sanitizer_common/sanitizer_stoptheworld_mac.cc

55–63

I don't think it's expensive to convert, but we should make sure to only do the conversion once per thread.

[fjricci]

lib/sanitizer_common/sanitizer_stoptheworld_mac.cc

55–63

Does that go for pthread_threadid_np as well? I don't think there's a good way to cache the values and ensure it's only called once, other than using a struct to store both.

[fjricci]

lib/sanitizer_common/sanitizer_stoptheworld_mac.cc

55–63

Looks like the pthread call takes a pthread_t and not a thread_t anyway. I'll try using the struct with a single call to thread_info.

[kubamracek]

lib/sanitizer_common/sanitizer_stoptheworld_mac.cc

55–63

Right. I'm suggesting to use the struct to store both.

[fjricci]

Use a struct to store both thread and tid

I'm not sure how great this patch is, I don't love that we store
the tid twice on Linux.

[fjricci]

If it's helpful for the purposes of design/review, I can also upload the 3 followup patches that actually use this functionality. I've been trying to avoid overwhelming with too many patches at once.

[kubamracek]

lib/sanitizer_common/sanitizer_stoptheworld.h

39–40

Can we use actual types here? The typedefs are just confusing.

How about this: The contents of this struct will be ifdef'd and use actual types (pid_t on Linux, thread_t+uptr64_t on Darwin). The SuspendedThreadsList API below will only deal with the whole struct, i.e. GetThread returns the whole struct, GetThreadID will be removed, Contains will search for matches of the whole struct (or select one field to be the "primary id", which we'll match for).

[fjricci]

Refactor to address comments

[kubamracek]

Besides the issue with "uptr tid", LGTM.

lib/sanitizer_common/sanitizer_stoptheworld.h

28

uptr is not correct here; macOS has 64-bit thread IDs even on i386

[kcc]

lib/sanitizer_common/sanitizer_stoptheworld.h

20

please avoid any #if/#ifdef in this file (and in most others).
If you need something platform-specific, put it into a platform-specific file

[fjricci]

Hmm. In order to do this without using any #ifdef's, I think we'll need to move the struct into two new header files - sanitizer_stoptheworld_linux.h and sanitizer_stoptheworld_mac.h. Other than that, I think we should be fine removing them, if that sounds like an okay solution. I'm not sure that any existing plaform-specific headers really work for this.

[kcc]

lib/sanitizer_common/sanitizer_stoptheworld.h

21

And more generally, please do not include *any* system headers in any sanitizer file, unless it already includes other system headers.

[fjricci]

Don't pollute with system headers or use platform-specific ifdefs

[fjricci]

Ping - this blocks several more Darwin LSAN patches I have waiting to send for review.

[fjricci]

As an update to the concern raised by @kcc regarding platform-specific differences, the primary reason why we store two values (tid + thread_t) on Darwin and only one (tid) on linux is performance. We could remove platform-specific headers as well as the ifdef's if we only stored the tid on darwin as well, but that would require conversion between tid and thread_t in several cases (reading register state, etc). This is a relatively expensive conversion (requires a syscall), so we prefer to do the conversion only once and store the result, even though this does introduce some differences between the platforms.

I think that the current implementation, which uses platform-specific headers (but no system headers), and no ifdefs is a reasonable compromise. With this change: D31774, it could actually become even more straightforward, although platform-specific headers would still be required to store the thread_t. However, if we'd prefer to store only the tid on Darwin as well, and accept the cost of the conversion to thread_t each time it's required, I can upload a patch to do that.

[kcc]

Please add Aleksey to all lsan code reviews.

[alekseyshl]

Should we commit and merge with D31774 first?

lib/sanitizer_common/sanitizer_stoptheworld.h

33

const SuspendedThreadInfo& GetThread

38

I think we should stick to Contains(SuspendedThreadID thread_id), it's semantics is to check whether the thread info for the given thread id is already appended to the list, not that the list contains the same info. Maybe even rename it to ContainsTid.

[fjricci]

In D31474#724919, @alekseyshl wrote:

Should we commit and merge with D31774 first?

You can probably feel free to add yourself as a reviewer to that diff, I assume it may not have been merged since we weren't sure whether it's a solution we want for this patch.

[fjricci]

I'll rebase onto D31774 and add it as a dependency.

[fjricci]

Rebase onto D31774

[fjricci]

This breaks compilation of asan for windows unless we define a dummy struct there as well

[fjricci]

the tid struct member is required, even on windows

[alekseyshl]

Sorry for the push back, but the sanitizer_stoptheworld_win.h makes things really ugly. Now it feels like the entire SuspendedThreadsList implementation need to go into *_linux and *_mac specific files, leaving only the interface in sanitizer_stoptheworld.h:

class SuspendedThreadsList {
public:

virtual uptr GetThreadCount() const = 0;
virtual SuspendedThreadID GetThreadID(uptr index) const = 0;
virtual uptr RegisterCount() = 0;
virtual PtraceRegistersStatus GetRegistersAndSP(uptr index, uptr *buffer, uptr *sp) const = 0;

};

The actual implementation SuspendedThreadsListLinux/Mac is free to store whatever it needs. Does it look cleaner?

[fjricci]

Yeah, I was growing pretty dissatisfied with the current state as well - I like the abstract class idea. However, it will mean we need a windows implementation. The stoptheworld header is included by lsan_common, which is included by asan, even when lsan is disabled. I could write a dummy implementation for windows, with CHECK(0 && "not implemented") in all the functions, if that seems acceptable.

[fjricci]

Split out platform-specific implementations of suspended threads list

Turns out windows isn't needed for compilation.

[fjricci]

(This does still require D31774)

[fjricci]

Don't clobber the blame too much

[alekseyshl]

Yes, CHECK(0 && "not implemented") makes much more sense to the reader than dummy tid struct, it means exactly what it looks like, the functionality is not implemented.

[fjricci]

In D31474#728236, @alekseyshl wrote:

Yes, CHECK(0 && "not implemented") makes much more sense to the reader than dummy tid struct, it means exactly what it looks like, the functionality is not implemented.

It turns out that even the CHECK() isn't necessary for compilation on windows (I checked that asan+windows builds with this patch). Presumably if someone attempts to actually use stoptheworld on Windows, they'll hit a build error from the abstract class being unimplemented. I think this is exactly what we'd want (and even better than a CHECK).

[fjricci]

Add in some functionality that we'll need later, and clean up append function

[alekseyshl]

Looks much better, thank you!

lib/sanitizer_common/sanitizer_stoptheworld.h

39

Please rename it to ThreadCount.

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

518

Remove this comment.

521

Add empty line to separate functions.

lib/sanitizer_common/sanitizer_stoptheworld_mac.cc

56

Remove this comment.

57

uptr SuspendedThreadsListMac::thread_count() const {

return threads_.size();

}

and add an empty line between functions.

[fjricci]

Address comments

[fjricci]

Looks like we can't use __cxa_pure_virtual in the sanitizers - replace with UNIMPLEMENTED

[fjricci]

Since it isn't quite a trivial change, want to verify it still looks okay.

[alekseyshl]

LGTM

lib/lsan/lsan_common.cc

 void ForEachExtraStackRangeCb(uptr begin, uptr end, void* arg) {
   Frontier *frontier = reinterpret_cast<Frontier *>(arg);
   ScanRangeForPointers(begin, end, frontier, "FAKE STACK", kReachable);
 }
 
 // Scans thread data (stacks and TLS) for heap pointers.
 static void ProcessThreads(SuspendedThreadsList const &suspended_threads,
                            Frontier *frontier) {
-  InternalScopedBuffer<uptr> registers(SuspendedThreadsList::RegisterCount());
+  InternalScopedBuffer<uptr> registers(suspended_threads.RegisterCount());
   uptr registers_begin = reinterpret_cast<uptr>(registers.data());
   uptr registers_end = registers_begin + registers.size();
   for (uptr i = 0; i < suspended_threads.thread_count(); i++) {
     tid_t os_id = static_cast<tid_t>(suspended_threads.GetThreadID(i));
     LOG_THREADS("Processing thread %d.\n", os_id);
     uptr stack_begin, stack_end, tls_begin, tls_end, cache_begin, cache_end;
     DTLS *dtls;
     bool thread_found = GetThreadRangesLocked(os_id, &stack_begin, &stack_end,

lib/sanitizer_common/sanitizer_stoptheworld.h

 //
 //===----------------------------------------------------------------------===//
 #ifndef SANITIZER_STOPTHEWORLD_H
 #define SANITIZER_STOPTHEWORLD_H
 
 #include "sanitizer_internal_defs.h"
 #include "sanitizer_common.h"
 
 namespace __sanitizer {
 
 enum PtraceRegistersStatus {
   REGISTERS_UNAVAILABLE_FATAL = -1,
   REGISTERS_UNAVAILABLE = 0,
   REGISTERS_AVAILABLE = 1
 };
 
 // Holds the list of suspended threads and provides an interface to dump their
 // register contexts.
 class SuspendedThreadsList {
  public:
-  SuspendedThreadsList()
-    : thread_ids_(1024) {}
-  tid_t GetThreadID(uptr index) const {
-    CHECK_LT(index, thread_ids_.size());
-    return thread_ids_[index];
-  }
-  PtraceRegistersStatus GetRegistersAndSP(uptr index, uptr *buffer,
-                                          uptr *sp) const;
+  SuspendedThreadsList() = default;
+  virtual tid_t GetThreadID(uptr index) const = 0;
+
+  virtual PtraceRegistersStatus GetRegistersAndSP(uptr index, uptr *buffer,
+                                                  uptr *sp) const = 0;
   // The buffer in GetRegistersAndSP should be at least this big.
-  static uptr RegisterCount();
-  uptr thread_count() const { return thread_ids_.size(); }
-  bool Contains(tid_t thread_id) const {
-    for (uptr i = 0; i < thread_ids_.size(); i++) {
-      if (thread_ids_[i] == thread_id)
-        return true;
-    }
-    return false;
-  }
-  void Append(tid_t thread_id) {
-    thread_ids_.push_back(thread_id);
-  }
+  virtual uptr RegisterCount() const = 0;
+  virtual uptr thread_count() const = 0;
+  virtual bool ContainsTid(tid_t thread_id) const = 0;
 
  private:
-  InternalMmapVector<tid_t> thread_ids_;
-
   // Prohibit copy and assign.
   SuspendedThreadsList(const SuspendedThreadsList&);
   void operator=(const SuspendedThreadsList&);
 };
 
 typedef void (*StopTheWorldCallback)(
     const SuspendedThreadsList &suspended_threads_list,
     void *argument);

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

 // because clone() does not set up a thread-local storage for it. Any
 // thread-local variables used by libc will be shared between the tracer task
 // and the thread which spawned it.
 
 namespace __sanitizer {
 
 COMPILER_CHECK(sizeof(tid_t) == sizeof(pid_t));
 
+class SuspendedThreadsListLinux : public SuspendedThreadsList {
+ public:
+  SuspendedThreadsListLinux() : thread_ids_(1024) {}
+
+  tid_t GetThreadID(uptr index) const;
+  uptr thread_count() const;
+  bool ContainsTid(tid_t thread_id) const;
+  void Append(tid_t tid);
+
+  PtraceRegistersStatus GetRegistersAndSP(uptr index, uptr *buffer,
+                                          uptr *sp) const;
+  uptr RegisterCount() const;
+
+ private:
+  InternalMmapVector<tid_t> thread_ids_;
+};
+
 // Structure for passing arguments into the tracer thread.
 struct TracerThreadArgument {
   StopTheWorldCallback callback;
   void *callback_argument;
   // The tracer thread waits on this mutex while the parent finishes its
   // preparations.
   BlockingMutex mutex;
   // Tracer thread signals its completion by setting done.
   atomic_uintptr_t done;
   uptr parent_pid;
 };
 
 // This class handles thread suspending/unsuspending in the tracer thread.
 class ThreadSuspender {
  public:
   explicit ThreadSuspender(pid_t pid, TracerThreadArgument *arg)
     : arg(arg)
     , pid_(pid) {
       CHECK_GE(pid, 0);
     }
   bool SuspendAllThreads();
   void ResumeAllThreads();
   void KillAllThreads();
-  SuspendedThreadsList &suspended_threads_list() {
+  SuspendedThreadsListLinux &suspended_threads_list() {
     return suspended_threads_list_;
   }
   TracerThreadArgument *arg;
  private:
-  SuspendedThreadsList suspended_threads_list_;
+  SuspendedThreadsListLinux suspended_threads_list_;
   pid_t pid_;
   bool SuspendThread(tid_t thread_id);
 };
 
 bool ThreadSuspender::SuspendThread(tid_t tid) {
   // Are we already attached to this thread?
   // Currently this check takes linear time, however the number of threads is
   // usually small.
-  if (suspended_threads_list_.Contains(tid))
-    return false;
+  if (suspended_threads_list_.ContainsTid(tid)) return false;
   int pterrno;
   if (internal_iserror(internal_ptrace(PTRACE_ATTACH, tid, nullptr, nullptr),
                        &pterrno)) {
     // Either the thread is dead, or something prevented us from attaching.
     // Log this event and move on.
     VReport(1, "Could not attach to thread %d (errno %d).\n", tid, pterrno);
     return false;
   } else {

 typedef _user_regs_struct regs_struct;
 #define REG_SP gprs[15]
 #define ARCH_IOVEC_FOR_GETREGSET
 
 #else
 #error "Unsupported architecture"
 #endif // SANITIZER_ANDROID && defined(__arm__)
 
-PtraceRegistersStatus SuspendedThreadsList::GetRegistersAndSP(uptr index,
-                                                              uptr *buffer,
-                                                              uptr *sp) const {
+tid_t SuspendedThreadsListLinux::GetThreadID(uptr index) const {
+  CHECK_LT(index, thread_ids_.size());
+  return thread_ids_[index];
+}
+
+// The buffer in GetRegistersAndSP should be at least this big.
+uptr SuspendedThreadsListLinux::thread_count() const {
+  return thread_ids_.size();
+}
+bool SuspendedThreadsListLinux::ContainsTid(tid_t thread_id) const {
+  for (uptr i = 0; i < thread_ids_.size(); i++) {
+    if (thread_ids_[i] == thread_id) return true;
+  }
+  return false;
+}
+
+void SuspendedThreadsListLinux::Append(tid_t tid) {
+  thread_ids_.push_back(tid);
+}
+
+PtraceRegistersStatus SuspendedThreadsListLinux::GetRegistersAndSP(
+    uptr index, uptr *buffer, uptr *sp) const {
   pid_t tid = GetThreadID(index);
   regs_struct regs;
   int pterrno;
 #ifdef ARCH_IOVEC_FOR_GETREGSET
   struct iovec regset_io;
   regset_io.iov_base = &regs;
   regset_io.iov_len = sizeof(regs_struct);
   bool isErr = internal_iserror(internal_ptrace(PTRACE_GETREGSET, tid,
return pterrno == ESRCH ? REGISTERS_UNAVAILABLE_FATAL
                             : REGISTERS_UNAVAILABLE;
   }
 
   *sp = regs.REG_SP;
   internal_memcpy(buffer, &regs, sizeof(regs));
   return REGISTERS_AVAILABLE;
 }
 
-uptr SuspendedThreadsList::RegisterCount() {
+uptr SuspendedThreadsListLinux::RegisterCount() const {
   return sizeof(regs_struct) / sizeof(uptr);
 }
 } // namespace __sanitizer
 
 #endif  // SANITIZER_LINUX && (defined(__x86_64__) || defined(__mips__)
         // || defined(__aarch64__) || defined(__powerpc64__)
         // || defined(__s390__) || defined(__i386__) || defined(__arm__)

lib/sanitizer_common/sanitizer_stoptheworld_mac.cc

 //
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_platform.h"
 
 #if SANITIZER_MAC && (defined(__x86_64__) || defined(__aarch64__) || \
                       defined(__mips64) || defined(__i386))
 
+#include <mach/mach.h>
+
 #include "sanitizer_stoptheworld.h"
 
 namespace __sanitizer {
+typedef struct {
+  tid_t tid;
+  thread_t thread;
+} SuspendedThreadInfo;
+
+class SuspendedThreadsListMac : public SuspendedThreadsList {
+ public:
+  SuspendedThreadsListMac() : threads_(1024) {}
+
+  tid_t GetThreadID(uptr index) const;
+  thread_t GetThread(uptr index) const;
+  uptr thread_count() const;
+  bool ContainsTid(tid_t thread_id) const;
+  void Append(thread_t thread);
+
+  PtraceRegistersStatus GetRegistersAndSP(uptr index, uptr *buffer,
+                                          uptr *sp) const;
+  uptr RegisterCount() const;
+
+ private:
+  InternalMmapVector<SuspendedThreadInfo> threads_;
+};
+
 void StopTheWorld(StopTheWorldCallback callback, void *argument) {
   CHECK(0 && "unimplemented");
 }
 
-PtraceRegistersStatus SuspendedThreadsList::GetRegistersAndSP(uptr index,
-                                                              uptr *buffer,
-                                                              uptr *sp) const {
+tid_t SuspendedThreadsListMac::GetThreadID(uptr index) const {
+  CHECK_LT(index, threads_.size());
+  return threads_[index].tid;
+}
+
+thread_t SuspendedThreadsListMac::GetThread(uptr index) const {
+  CHECK_LT(index, threads_.size());
+  return threads_[index].thread;
+}
+
+// The buffer in GetRegistersAndSP should be at least this big.
+uptr SuspendedThreadsListMac::thread_count() const { return threads_.size(); }
+bool SuspendedThreadsListMac::ContainsTid(tid_t thread_id) const {
+  for (uptr i = 0; i < threads_.size(); i++) {
+    if (threads_[i].tid == thread_id) return true;
+  }
+  return false;
+}
+
+void SuspendedThreadsListMac::Append(thread_t thread) {
+  thread_identifier_info_data_t info;
+  mach_msg_type_number_t info_count = THREAD_IDENTIFIER_INFO_COUNT;
+  kern_return_t err = thread_info(thread, THREAD_IDENTIFIER_INFO,
+                                  (thread_info_t)&info, &info_count);
+  if (err != KERN_SUCCESS) {
+    VReport(1, "Error - unable to get thread ident for a thread\n");
+    return;
+  }
+  threads_.push_back({info.thread_id, thread});
+}
+
+PtraceRegistersStatus SuspendedThreadsListMac::GetRegistersAndSP(
+    uptr index, uptr *buffer, uptr *sp) const {
   CHECK(0 && "unimplemented");
   return REGISTERS_UNAVAILABLE_FATAL;
 }
 
-uptr SuspendedThreadsList::RegisterCount() {
+uptr SuspendedThreadsListMac::RegisterCount() const {
   CHECK(0 && "unimplemented");
   return 0;
 }
 } // namespace __sanitizer
 
 #endif  // SANITIZER_MAC && (defined(__x86_64__) || defined(__aarch64__)) ||
         //                   defined(__mips64) || defined(__i386))