[NFC][LSAN] Change SuspendedThreadsList interface

[vitalybuka]

Summary

Remove RegisterCount and let GetRegistersAndSP to resize buffer as needed.

[vitalybuka]

fix ProcessThreads

[vitalybuka]

RoundUpTo

[morehouse]

LGTM. Was this causing any issues, or is this just a refactor?

[vitalybuka]

In D87747#2277088, @morehouse wrote:

LGTM. Was this causing any issues, or is this just a refactor?

Just refactoring before making GetRegistersAndSP more complicated. RegisterCount with extra registers is inconvenient to calculate.

compiler-rt/lib/lsan/lsan_common.cpp

 // Fuchsia handles all threads together with its own callback.
 static void ProcessThreads(SuspendedThreadsList const &, Frontier *) {}
 
 #else
 
 // Scans thread data (stacks and TLS) for heap pointers.
 static void ProcessThreads(SuspendedThreadsList const &suspended_threads,
                            Frontier *frontier) {
-  InternalMmapVector<uptr> registers(suspended_threads.RegisterCount());
-  uptr registers_begin = reinterpret_cast<uptr>(registers.data());
-  uptr registers_end =
-      reinterpret_cast<uptr>(registers.data() + registers.size());
+  InternalMmapVector<uptr> registers;
   for (uptr i = 0; i < suspended_threads.ThreadCount(); 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,
                                               &tls_begin, &tls_end,
                                               &cache_begin, &cache_end, &dtls);
     if (!thread_found) {
       // If a thread can't be found in the thread registry, it's probably in the
       // process of destruction. Log this event and move on.
       LOG_THREADS("Thread %d not found in registry.\n", os_id);
       continue;
     }
     uptr sp;
     PtraceRegistersStatus have_registers =
-        suspended_threads.GetRegistersAndSP(i, registers.data(), &sp);
+        suspended_threads.GetRegistersAndSP(i, &registers, &sp);
     if (have_registers != REGISTERS_AVAILABLE) {
       Report("Unable to get registers from thread %d.\n", os_id);
       // If unable to get SP, consider the entire stack to be reachable unless
       // GetRegistersAndSP failed with ESRCH.
       if (have_registers == REGISTERS_UNAVAILABLE_FATAL) continue;
       sp = stack_begin;
     }
 
-    if (flags()->use_registers && have_registers)
+    if (flags()->use_registers && have_registers) {
+      uptr registers_begin = reinterpret_cast<uptr>(registers.data());
+      uptr registers_end =
+          reinterpret_cast<uptr>(registers.data() + registers.size());
       ScanRangeForPointers(registers_begin, registers_end, frontier,
                            "REGISTERS", kReachable);
+    }
 
     if (flags()->use_stacks) {
       LOG_THREADS("Stack at %p-%p (SP = %p).\n", stack_begin, stack_end, sp);
       if (sp < stack_begin || sp >= stack_end) {
         // SP is outside the recorded stack range (e.g. the thread is running a
         // signal handler on alternate stack, or swapcontext was used).
         // Again, consider the entire stack range to be reachable.
         LOG_THREADS("WARNING: stack pointer not in stack range.\n");

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld.h

 // Holds the list of suspended threads and provides an interface to dump their
 // register contexts.
 class SuspendedThreadsList {
  public:
   SuspendedThreadsList() = default;
 
   // Can't declare pure virtual functions in sanitizer runtimes:
   // __cxa_pure_virtual might be unavailable. Use UNIMPLEMENTED() instead.
-  virtual PtraceRegistersStatus GetRegistersAndSP(uptr index, uptr *buffer,
-                                                  uptr *sp) const {
+  virtual PtraceRegistersStatus GetRegistersAndSP(
+      uptr index, InternalMmapVector<uptr> *buffer, uptr *sp) const {
     UNIMPLEMENTED();
   }
 
-  // The buffer in GetRegistersAndSP should be at least this big.
-  virtual uptr RegisterCount() const { UNIMPLEMENTED(); }
   virtual uptr ThreadCount() const { UNIMPLEMENTED(); }
   virtual tid_t GetThreadID(uptr index) const { UNIMPLEMENTED(); }
 
  private:
   // Prohibit copy and assign.
   SuspendedThreadsList(const SuspendedThreadsList&);
   void operator=(const SuspendedThreadsList&);
 };

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp

  public:
   SuspendedThreadsListLinux() { thread_ids_.reserve(1024); }
 
   tid_t GetThreadID(uptr index) const override;
   uptr ThreadCount() const override;
   bool ContainsTid(tid_t thread_id) const;
   void Append(tid_t tid);
 
-  PtraceRegistersStatus GetRegistersAndSP(uptr index, uptr *buffer,
+  PtraceRegistersStatus GetRegistersAndSP(uptr index,
+                                          InternalMmapVector<uptr> *buffer,
                                           uptr *sp) const override;
-  uptr RegisterCount() const override;
 
  private:
   InternalMmapVector<tid_t> thread_ids_;
 };
 
 // Structure for passing arguments into the tracer thread.
 struct TracerThreadArgument {
   StopTheWorldCallback callback;
bool SuspendedThreadsListLinux::ContainsTid(tid_t thread_id) const {
   return false;
 }
 
 void SuspendedThreadsListLinux::Append(tid_t tid) {
   thread_ids_.push_back(tid);
 }
 
 PtraceRegistersStatus SuspendedThreadsListLinux::GetRegistersAndSP(
-    uptr index, uptr *buffer, uptr *sp) const {
+    uptr index, InternalMmapVector<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,
if (isErr) {
     // ESRCH means that the given thread is not suspended or already dead.
     // Therefore it's unsafe to inspect its data (e.g. walk through stack) and
     // we should notify caller about this.
     return pterrno == ESRCH ? REGISTERS_UNAVAILABLE_FATAL
                             : REGISTERS_UNAVAILABLE;
   }
 
   *sp = regs.REG_SP;
-  internal_memcpy(buffer, &regs, sizeof(regs));
+  buffer->resize(RoundUpTo(sizeof(regs), sizeof(uptr)) / sizeof(uptr));
+  internal_memcpy(buffer->data(), &regs, sizeof(regs));
   return REGISTERS_AVAILABLE;
 }
 
-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__)

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_mac.cpp

public:
   SuspendedThreadsListMac() : threads_(1024) {}
 
   tid_t GetThreadID(uptr index) const override;
   thread_t GetThread(uptr index) const;
   uptr ThreadCount() const override;
   bool ContainsThread(thread_t thread) const;
   void Append(thread_t thread);
 
-  PtraceRegistersStatus GetRegistersAndSP(uptr index, uptr *buffer,
+  PtraceRegistersStatus GetRegistersAndSP(uptr index,
+                                          InternalMmapVector<uptr> *buffer,
                                           uptr *sp) const override;
-  uptr RegisterCount() const override;
 
  private:
   InternalMmapVector<SuspendedThreadInfo> threads_;
 };
 
 struct RunThreadArgs {
   StopTheWorldCallback callback;
   void *argument;
void SuspendedThreadsListMac::Append(thread_t thread) {
   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 {
+    uptr index, InternalMmapVector<uptr> *buffer, uptr *sp) const {
   thread_t thread = GetThread(index);
   regs_struct regs;
   int err;
   mach_msg_type_number_t reg_count = MACHINE_THREAD_STATE_COUNT;
   err = thread_get_state(thread, MACHINE_THREAD_STATE, (thread_state_t)&regs,
                          &reg_count);
   if (err != KERN_SUCCESS) {
     VReport(1, "Error - unable to get registers for a thread\n");
     // KERN_INVALID_ARGUMENT indicates that either the flavor is invalid,
     // or the thread does not exist. The other possible error case,
     // MIG_ARRAY_TOO_LARGE, means that the state is too large, but it's
     // still safe to proceed.
     return err == KERN_INVALID_ARGUMENT ? REGISTERS_UNAVAILABLE_FATAL
                                         : REGISTERS_UNAVAILABLE;
   }
 
-  internal_memcpy(buffer, &regs, sizeof(regs));
+  buffer->resize(RoundUpTo(sizeof(regs), sizeof(uptr)) / sizeof(uptr));
+  internal_memcpy(buffer->data(), &regs, sizeof(regs));
 #if defined(__aarch64__) && defined(arm_thread_state64_get_sp)
   *sp = arm_thread_state64_get_sp(regs);
 #else
   *sp = regs.SP_REG;
 #endif
 
   // On x86_64 and aarch64, we must account for the stack redzone, which is 128
   // bytes.
   if (SANITIZER_WORDSIZE == 64) *sp -= 128;
 
   return REGISTERS_AVAILABLE;
 }
 
-uptr SuspendedThreadsListMac::RegisterCount() const {
-  return MACHINE_THREAD_STATE_COUNT;
-}
 } // namespace __sanitizer
 
 #endif  // SANITIZER_MAC && (defined(__x86_64__) || defined(__aarch64__)) ||
         //                   defined(__i386))

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_netbsd_libcdep.cpp

  public:
   SuspendedThreadsListNetBSD() { thread_ids_.reserve(1024); }
 
   tid_t GetThreadID(uptr index) const;
   uptr ThreadCount() const;
   bool ContainsTid(tid_t thread_id) const;
   void Append(tid_t tid);
 
-  PtraceRegistersStatus GetRegistersAndSP(uptr index, uptr *buffer,
+  PtraceRegistersStatus GetRegistersAndSP(uptr index,
+                                          InternalMmapVector<uptr> *buffer,
                                           uptr *sp) const;
-  uptr RegisterCount() const;
 
  private:
   InternalMmapVector<tid_t> thread_ids_;
 };
 
 struct TracerThreadArgument {
   StopTheWorldCallback callback;
   void *callback_argument;
bool SuspendedThreadsListNetBSD::ContainsTid(tid_t thread_id) const {
   return false;
 }
 
 void SuspendedThreadsListNetBSD::Append(tid_t tid) {
   thread_ids_.push_back(tid);
 }
 
 PtraceRegistersStatus SuspendedThreadsListNetBSD::GetRegistersAndSP(
-    uptr index, uptr *buffer, uptr *sp) const {
+    uptr index, InternalMmapVector<uptr> *buffer, uptr *sp) const {
   lwpid_t tid = GetThreadID(index);
   pid_t ppid = internal_getppid();
   struct reg regs;
   int pterrno;
   bool isErr =
       internal_iserror(internal_ptrace(PT_GETREGS, ppid, &regs, tid), &pterrno);
   if (isErr) {
     VReport(1,
             "Could not get registers from process %d thread %d (errno %d).\n",
             ppid, tid, pterrno);
     return pterrno == ESRCH ? REGISTERS_UNAVAILABLE_FATAL
                             : REGISTERS_UNAVAILABLE;
   }
 
   *sp = PTRACE_REG_SP(&regs);
-  internal_memcpy(buffer, &regs, sizeof(regs));
+  buffer->resize(RoundUpTo(sizeof(regs), sizeof(uptr)) / sizeof(uptr));
+  internal_memcpy(buffer->data(), &regs, sizeof(regs));
 
   return REGISTERS_AVAILABLE;
 }
 
-uptr SuspendedThreadsListNetBSD::RegisterCount() const {
-  return sizeof(struct reg) / sizeof(uptr);
-}
 }  // namespace __sanitizer
 
 #endif