[sanitizer] Simplify ThreadLister interface

Reviewers: eugenis

Subscribers: kubamracek, llvm-commits

Differential Revision: https://reviews.llvm.org/D46516

llvm-svn: 331701

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
index ce8a18b..71ec762 100644 (file)
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
@@ -905,47 +905,40 @@ bool internal_sigismember(__sanitizer_sigset_t *set, int signum) {
 #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') {
-      // Found a valid tid.
-      tid = (int)internal_atoll(entry_->d_name);
+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;
     }
-    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);
+    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.
+        threads->push_back(internal_atoll(entry->d_name));
+      }
+    }
+  }
+  return true;
 }
 
 ThreadLister::~ThreadLister() {
@@ -953,24 +946,6 @@ ThreadLister::~ThreadLister() {
     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() {

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.h b/compiler-rt/lib/sanitizer_common/sanitizer_linux.h
index 5d38f5e..d4733f3 100644 (file)
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.h
@@ -76,21 +76,12 @@ 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.

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc b/compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc
index e911216..344356b 100644 (file)
--- a/compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc
@@ -211,21 +211,23 @@ 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;

diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_linux_test.cc b/compiler-rt/lib/sanitizer_common/tests/sanitizer_linux_test.cc
index 8a6afab..5efd173 100644 (file)
--- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_linux_test.cc
+++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_linux_test.cc
@@ -123,11 +123,9 @@ void ThreadListerTest::SpawnTidReporter(pthread_t *pthread_id,
 
 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) {
@@ -151,23 +149,20 @@ TEST_F(ThreadListerTest, ThreadListerSeesAllSpawnedThreads) {
   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);
 
@@ -182,8 +177,6 @@ TEST_F(ThreadListerTest, ResetMakesNewThreadsKnown) {
   // 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));
 }