[sanitizer] Simplify ThreadLister interface

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

925 ↗

(On Diff #145435)

why not while(true) ?

[vitalybuka]

Simplify loop

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

925 ↗

(On Diff #145435)

Just at artifact of attemt to detect short read by number of iterations

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

 #endif
 #endif  // SANITIZER_LINUX
 
 // This class reads thread IDs from /proc/<pid>/task using only syscalls.
 class ThreadLister {
  public:
   explicit ThreadLister(int pid);
   ~ThreadLister();
-  // GetNextTID returns -1 if the list of threads is exhausted, or if there has
-  // been an error.
-  int GetNextTID();
-  void Reset();
-  bool error();
+  bool ListThreads(InternalMmapVector<int> *threads);
 
  private:
-  bool GetDirectoryEntries();
-
   int pid_;
-  int descriptor_;
+  int descriptor_ = -1;
   InternalMmapVector<char> buffer_;
-  bool error_;
-  struct linux_dirent* entry_;
-  int bytes_read_;
 };
 
 // Exposed for testing.
 uptr ThreadDescriptorSize();
 uptr ThreadSelf();
 uptr ThreadSelfOffset();
 
 // Matches a library's file name against a base name (stripping path and version

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

bool internal_sigismember(__sanitizer_sigset_t *set, int signum) {
   const uptr idx = signum / (sizeof(k_set->sig[0]) * 8);
   const uptr bit = signum % (sizeof(k_set->sig[0]) * 8);
   return k_set->sig[idx] & (1 << bit);
 }
 #endif  // SANITIZER_LINUX
 #endif // !SANITIZER_SOLARIS
 
 // ThreadLister implementation.
-ThreadLister::ThreadLister(int pid)
-  : pid_(pid),
-    descriptor_(-1),
-    buffer_(4096),
-    error_(true),
-    entry_((struct linux_dirent *)buffer_.data()),
-    bytes_read_(0) {
+ThreadLister::ThreadLister(int pid) : pid_(pid), buffer_(4096) {
+  buffer_.resize(buffer_.capacity());
   char task_directory_path[80];
   internal_snprintf(task_directory_path, sizeof(task_directory_path),
                     "/proc/%d/task/", pid);
-  uptr openrv = internal_open(task_directory_path, O_RDONLY | O_DIRECTORY);
-  if (internal_iserror(openrv)) {
-    error_ = true;
+  descriptor_ = internal_open(task_directory_path, O_RDONLY | O_DIRECTORY);
+  if (internal_iserror(descriptor_)) {
     Report("Can't open /proc/%d/task for reading.\n", pid);
-  } else {
-    error_ = false;
-    descriptor_ = openrv;
   }
 }
 
-int ThreadLister::GetNextTID() {
-  int tid = -1;
-  do {
-    if (error_)
-      return -1;
-    if ((char *)entry_ >= &buffer_[bytes_read_] && !GetDirectoryEntries())
-      return -1;
-    if (entry_->d_ino != 0 && entry_->d_name[0] >= '0' &&
-        entry_->d_name[0] <= '9') {
+bool ThreadLister::ListThreads(InternalMmapVector<int> *threads) {
+  if (!descriptor_) return false;
+  internal_lseek(descriptor_, 0, SEEK_SET);
+  threads->clear();
+
+  while (uptr read = internal_getdents(descriptor_,
+                                       (struct linux_dirent *)buffer_.data(),
+                                       buffer_.size())) {
+    if (internal_iserror(read)) {
+      Report("Can't read directory entries from /proc/%d/task.\n", pid_);
+      return false;
+    }
+
+    for (uptr begin = (uptr)buffer_.data(), end = begin + read; begin < end;) {
+      struct linux_dirent *entry = (struct linux_dirent *)begin;
+      begin += entry->d_reclen;
+      if (entry->d_ino && *entry->d_name >= '0' && *entry->d_name <= '9') {
         // Found a valid tid.
-      tid = (int)internal_atoll(entry_->d_name);
+        threads->push_back(internal_atoll(entry->d_name));
       }
-    entry_ = (struct linux_dirent *)(((char *)entry_) + entry_->d_reclen);
-  } while (tid < 0);
-  return tid;
     }
-
-void ThreadLister::Reset() {
-  if (error_ || descriptor_ < 0)
-    return;
-  internal_lseek(descriptor_, 0, SEEK_SET);
+  }
+  return true;
 }
 
 ThreadLister::~ThreadLister() {
   if (descriptor_ >= 0)
     internal_close(descriptor_);
 }
 
-bool ThreadLister::error() { return error_; }
-
-bool ThreadLister::GetDirectoryEntries() {
-  CHECK_GE(descriptor_, 0);
-  CHECK_NE(error_, true);
-  bytes_read_ = internal_getdents(
-      descriptor_, (struct linux_dirent *)buffer_.data(), buffer_.size());
-  if (internal_iserror(bytes_read_)) {
-    Report("Can't read directory entries from /proc/%d/task.\n", pid_);
-    error_ = true;
-    return false;
-  } else if (bytes_read_ == 0) {
-    return false;
-  }
-  entry_ = (struct linux_dirent *)buffer_.data();
-  return true;
-}
-
 #if SANITIZER_WORDSIZE == 32
 // Take care of unusable kernel area in top gigabyte.
 static uptr GetKernelAreaSize() {
 #if SANITIZER_LINUX && !SANITIZER_X32
   const uptr gbyte = 1UL << 30;
 
   // Firstly check if there are writable segments
   // mapped to top gigabyte (e.g. stack).

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

for (uptr i = 0; i < suspended_threads_list_.ThreadCount(); i++)
     internal_ptrace(PTRACE_KILL, suspended_threads_list_.GetThreadID(i),
                     nullptr, nullptr);
 }
 
 bool ThreadSuspender::SuspendAllThreads() {
   ThreadLister thread_lister(pid_);
   bool added_threads;
   bool first_iteration = true;
+  InternalMmapVector<int> threads;
+  threads.reserve(128);
   do {
     // Run through the directory entries once.
     added_threads = false;
-    pid_t tid = thread_lister.GetNextTID();
-    while (tid >= 0) {
+    if (!thread_lister.ListThreads(&threads)) {
+      ResumeAllThreads();
+      return false;
+    }
+    for (int tid : threads)
       if (SuspendThread(tid))
         added_threads = true;
-      tid = thread_lister.GetNextTID();
-    }
-    if (thread_lister.error() || (first_iteration && !added_threads)) {
+    if (first_iteration && !added_threads) {
       // Detach threads and fail.
       ResumeAllThreads();
       return false;
     }
-    thread_lister.Reset();
     first_iteration = false;
   } while (added_threads);
   return true;
 }
 
 // Pointer to the ThreadSuspender instance for use in signal handler.
 static ThreadSuspender *thread_suspender_instance = nullptr;
 

compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_linux_test.cc

while (thread_arg.reported_tid == -1)
     pthread_cond_wait(&thread_arg.tid_reported_cond,
                       &thread_arg.tid_reported_mutex);
   pthread_mutex_unlock(&thread_arg.tid_reported_mutex);
   *tid = thread_arg.reported_tid;
 }
 
 static std::vector<pid_t> ReadTidsToVector(ThreadLister *thread_lister) {
   std::vector<pid_t> listed_tids;
-  pid_t tid;
-  while ((tid = thread_lister->GetNextTID()) >= 0)
-    listed_tids.push_back(tid);
-  EXPECT_FALSE(thread_lister->error());
-  return listed_tids;
+  InternalMmapVector<int> threads(128);
+  EXPECT_TRUE(thread_lister->ListThreads(&threads));
+  return std::vector<pid_t>(threads.begin(), threads.end());
 }
 
 static bool Includes(std::vector<pid_t> first, std::vector<pid_t> second) {
   std::sort(first.begin(), first.end());
   std::sort(second.begin(), second.end());
   return std::includes(first.begin(), first.end(),
                        second.begin(), second.end());
 }
 
 static bool HasElement(std::vector<pid_t> vector, pid_t element) {
   return std::find(vector.begin(), vector.end(), element) != vector.end();
 }
 
 // ThreadLister's output should include the current thread's TID and the TID of
 // every thread we spawned.
 TEST_F(ThreadListerTest, ThreadListerSeesAllSpawnedThreads) {
   pid_t self_tid = GetTid();
   ThreadLister thread_lister(getpid());
   std::vector<pid_t> listed_tids = ReadTidsToVector(&thread_lister);
   ASSERT_TRUE(HasElement(listed_tids, self_tid));
   ASSERT_TRUE(Includes(listed_tids, tids_));
 }
 
-// Calling Reset() should not cause ThreadLister to forget any threads it's
-// supposed to know about.
-TEST_F(ThreadListerTest, ResetDoesNotForgetThreads) {
+TEST_F(ThreadListerTest, DoNotForgetThreads) {
   ThreadLister thread_lister(getpid());
 
-  // Run the loop body twice, because Reset() might behave differently if called
-  // on a freshly created object.
+  // Run the loop body twice, because ThreadLister might behave differently if
+  // called on a freshly created object.
   for (uptr i = 0; i < 2; i++) {
-    thread_lister.Reset();
     std::vector<pid_t> listed_tids = ReadTidsToVector(&thread_lister);
     ASSERT_TRUE(Includes(listed_tids, tids_));
   }
 }
 
 // If new threads have spawned during ThreadLister object's lifetime, calling
-// Reset() should cause ThreadLister to recognize their existence.
-TEST_F(ThreadListerTest, ResetMakesNewThreadsKnown) {
+// relisting should cause ThreadLister to recognize their existence.
+TEST_F(ThreadListerTest, NewThreads) {
   ThreadLister thread_lister(getpid());
   std::vector<pid_t> threads_before_extra = ReadTidsToVector(&thread_lister);
 
   pthread_t extra_pthread_id;
   pid_t extra_tid;
   SpawnTidReporter(&extra_pthread_id, &extra_tid);
   // Register the new thread so it gets terminated in TearDown().
   pthread_ids_.push_back(extra_pthread_id);
 
   // It would be very bizarre if the new TID had been listed before we even
   // spawned that thread, but it would also cause a false success in this test,
   // so better check for that.
   ASSERT_FALSE(HasElement(threads_before_extra, extra_tid));
 
-  thread_lister.Reset();
-
   std::vector<pid_t> threads_after_extra = ReadTidsToVector(&thread_lister);
   ASSERT_TRUE(HasElement(threads_after_extra, extra_tid));
 }
 
 TEST(SanitizerCommon, SetEnvTest) {
   const char kEnvName[] = "ENV_FOO";
   SetEnv(kEnvName, "value");
   EXPECT_STREQ("value", getenv(kEnvName));