Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC][sanitizer][asan] Promote stack_id into ThreadContextBase #111917

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Oct 10, 2024

parent_id and stack_id represent location
where the thread was created, so it's reasonable
to keep them togeter.

For now, only Asan and MemProf use stack_id,
but it will be halpfull to print thread origin from
other sanitizers as well.

For #111948

@llvmbot llvmbot added compiler-rt compiler-rt:asan Address sanitizer PGO Profile Guided Optimizations compiler-rt:sanitizer labels Oct 10, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 10, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

@llvm/pr-subscribers-pgo

Author: Vitaly Buka (vitalybuka)

Changes

parent_id and stack_id represent location
where the thread was created, so it's reasonable
to keep them togeter.

For now, only Asan and MemProf use stack_id,
but it will be halpfull to print thread origin from
other sanitizers as well.

For TODO


Full diff: https://github.com/llvm/llvm-project/pull/111917.diff

8 Files Affected:

  • (modified) compiler-rt/lib/asan/asan_fuchsia.cpp (+1-2)
  • (modified) compiler-rt/lib/asan/asan_thread.cpp (+3-6)
  • (modified) compiler-rt/lib/asan/asan_thread.h (+1-7)
  • (modified) compiler-rt/lib/memprof/memprof_thread.cpp (+3-6)
  • (modified) compiler-rt/lib/memprof/memprof_thread.h (+1-7)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp (+9-4)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.h (+7-2)
  • (modified) compiler-rt/lib/sanitizer_common/tests/sanitizer_thread_registry_test.cpp (+16-7)
diff --git a/compiler-rt/lib/asan/asan_fuchsia.cpp b/compiler-rt/lib/asan/asan_fuchsia.cpp
index dbc4342e83388c..96c41e9d42ba6a 100644
--- a/compiler-rt/lib/asan/asan_fuchsia.cpp
+++ b/compiler-rt/lib/asan/asan_fuchsia.cpp
@@ -121,8 +121,7 @@ static AsanThread *CreateAsanThread(StackTrace *stack, u32 parent_tid,
   // In lieu of AsanThread::Create.
   AsanThread *thread = (AsanThread *)MmapOrDie(AsanThreadMmapSize(), __func__);
 
-  AsanThreadContext::CreateThreadContextArgs args = {thread, stack};
-  u32 tid = asanThreadRegistry().CreateThread(0, detached, parent_tid, &args);
+  u32 tid = asanThreadRegistry().CreateThread(0, detached, parent_tid, thread);
   asanThreadRegistry().SetThreadName(tid, name);
 
   return thread;
diff --git a/compiler-rt/lib/asan/asan_thread.cpp b/compiler-rt/lib/asan/asan_thread.cpp
index c1a804b9fcccd3..0779daa107682b 100644
--- a/compiler-rt/lib/asan/asan_thread.cpp
+++ b/compiler-rt/lib/asan/asan_thread.cpp
@@ -28,10 +28,7 @@ namespace __asan {
 // AsanThreadContext implementation.
 
 void AsanThreadContext::OnCreated(void *arg) {
-  CreateThreadContextArgs *args = static_cast<CreateThreadContextArgs *>(arg);
-  if (args->stack)
-    stack_id = StackDepotPut(*args->stack);
-  thread = args->thread;
+  thread = static_cast<AsanThread *>(arg);
   thread->set_context(this);
 }
 
@@ -106,8 +103,8 @@ AsanThread *AsanThread::Create(const void *start_data, uptr data_size,
     CHECK_LE(data_size, availible_size);
     internal_memcpy(thread->start_data_, start_data, data_size);
   }
-  AsanThreadContext::CreateThreadContextArgs args = {thread, stack};
-  asanThreadRegistry().CreateThread(0, detached, parent_tid, &args);
+  asanThreadRegistry().CreateThread(0, detached, parent_tid,
+                                    stack ? StackDepotPut(*stack) : 0, thread);
 
   return thread;
 }
diff --git a/compiler-rt/lib/asan/asan_thread.h b/compiler-rt/lib/asan/asan_thread.h
index 62f1b5337fe4bf..f7c3a451aa9d30 100644
--- a/compiler-rt/lib/asan/asan_thread.h
+++ b/compiler-rt/lib/asan/asan_thread.h
@@ -37,20 +37,14 @@ class AsanThreadContext final : public ThreadContextBase {
  public:
   explicit AsanThreadContext(int tid)
       : ThreadContextBase(tid), announced(false),
-        destructor_iterations(GetPthreadDestructorIterations()), stack_id(0),
+        destructor_iterations(GetPthreadDestructorIterations()),
         thread(nullptr) {}
   bool announced;
   u8 destructor_iterations;
-  u32 stack_id;
   AsanThread *thread;
 
   void OnCreated(void *arg) override;
   void OnFinished() override;
-
-  struct CreateThreadContextArgs {
-    AsanThread *thread;
-    StackTrace *stack;
-  };
 };
 
 // AsanThreadContext objects are never freed, so we need many of them.
diff --git a/compiler-rt/lib/memprof/memprof_thread.cpp b/compiler-rt/lib/memprof/memprof_thread.cpp
index 50072bb91ee74c..4b9665ffc3fcea 100644
--- a/compiler-rt/lib/memprof/memprof_thread.cpp
+++ b/compiler-rt/lib/memprof/memprof_thread.cpp
@@ -25,10 +25,7 @@ namespace __memprof {
 // MemprofThreadContext implementation.
 
 void MemprofThreadContext::OnCreated(void *arg) {
-  CreateThreadContextArgs *args = static_cast<CreateThreadContextArgs *>(arg);
-  if (args->stack)
-    stack_id = StackDepotPut(*args->stack);
-  thread = args->thread;
+  thread = static_cast<MemprofThread *>(arg);
   thread->set_context(this);
 }
 
@@ -79,8 +76,8 @@ MemprofThread *MemprofThread::Create(thread_callback_t start_routine, void *arg,
   MemprofThread *thread = (MemprofThread *)MmapOrDie(size, __func__);
   thread->start_routine_ = start_routine;
   thread->arg_ = arg;
-  MemprofThreadContext::CreateThreadContextArgs args = {thread, stack};
-  memprofThreadRegistry().CreateThread(0, detached, parent_tid, &args);
+  memprofThreadRegistry().CreateThread(
+      0, detached, parent_tid, stack ? StackDepotPut(*stack) : 0, thread);
 
   return thread;
 }
diff --git a/compiler-rt/lib/memprof/memprof_thread.h b/compiler-rt/lib/memprof/memprof_thread.h
index 4c9313fcb369eb..fb90dbf328a430 100644
--- a/compiler-rt/lib/memprof/memprof_thread.h
+++ b/compiler-rt/lib/memprof/memprof_thread.h
@@ -34,20 +34,14 @@ class MemprofThread;
 struct MemprofThreadContext final : public ThreadContextBase {
   explicit MemprofThreadContext(int tid)
       : ThreadContextBase(tid), announced(false),
-        destructor_iterations(GetPthreadDestructorIterations()), stack_id(0),
+        destructor_iterations(GetPthreadDestructorIterations()),
         thread(nullptr) {}
   bool announced;
   u8 destructor_iterations;
-  u32 stack_id;
   MemprofThread *thread;
 
   void OnCreated(void *arg) override;
   void OnFinished() override;
-
-  struct CreateThreadContextArgs {
-    MemprofThread *thread;
-    StackTrace *stack;
-  };
 };
 
 // MemprofThreadContext objects are never freed, so we need many of them.
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
index df04822b28851c..cdc24f4a8869c7 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.cpp
@@ -27,6 +27,7 @@ ThreadContextBase::ThreadContextBase(u32 tid)
       detached(false),
       thread_type(ThreadType::Regular),
       parent_tid(0),
+      stack_id(0),
       next(0) {
   name[0] = '\0';
   atomic_store(&thread_destroyed, 0, memory_order_release);
@@ -88,14 +89,17 @@ void ThreadContextBase::SetStarted(tid_t _os_id, ThreadType _thread_type,
 }
 
 void ThreadContextBase::SetCreated(uptr _user_id, u64 _unique_id,
-                                   bool _detached, u32 _parent_tid, void *arg) {
+                                   bool _detached, u32 _parent_tid,
+                                   u32 _stack_tid, void *arg) {
   status = ThreadStatusCreated;
   user_id = _user_id;
   unique_id = _unique_id;
   detached = _detached;
   // Parent tid makes no sense for the main thread.
-  if (tid != kMainTid)
+  if (tid != kMainTid) {
     parent_tid = _parent_tid;
+    stack_id = _stack_tid;
+  }
   OnCreated(arg);
 }
 
@@ -143,7 +147,7 @@ uptr ThreadRegistry::GetMaxAliveThreads() {
 }
 
 u32 ThreadRegistry::CreateThread(uptr user_id, bool detached, u32 parent_tid,
-                                 void *arg) {
+                                 u32 stack_tid, void *arg) {
   ThreadRegistryLock l(this);
   u32 tid = kInvalidTid;
   ThreadContextBase *tctx = QuarantinePop();
@@ -181,7 +185,8 @@ u32 ThreadRegistry::CreateThread(uptr user_id, bool detached, u32 parent_tid,
     // positives later (e.g. if we join a wrong thread).
     CHECK(live_.try_emplace(user_id, tid).second);
   }
-  tctx->SetCreated(user_id, total_threads_++, detached, parent_tid, arg);
+  tctx->SetCreated(user_id, total_threads_++, detached, parent_tid, stack_tid,
+                   arg);
   return tid;
 }
 
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.h b/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.h
index 2c7e5c276fa1c7..f42859698ff98e 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_thread_registry.h
@@ -52,6 +52,7 @@ class ThreadContextBase {
   ThreadType thread_type;
 
   u32 parent_tid;
+  u32 stack_id;
   ThreadContextBase *next;  // For storing thread contexts in a list.
 
   atomic_uint32_t thread_destroyed; // To address race of Joined vs Finished
@@ -63,7 +64,7 @@ class ThreadContextBase {
   void SetFinished();
   void SetStarted(tid_t _os_id, ThreadType _thread_type, void *arg);
   void SetCreated(uptr _user_id, u64 _unique_id, bool _detached,
-                  u32 _parent_tid, void *arg);
+                  u32 _parent_tid, u32 _stack_tid, void *arg);
   void Reset();
 
   void SetDestroyed();
@@ -106,7 +107,11 @@ class SANITIZER_MUTEX ThreadRegistry {
 
   u32 NumThreadsLocked() const { return threads_.size(); }
 
-  u32 CreateThread(uptr user_id, bool detached, u32 parent_tid, void *arg);
+  u32 CreateThread(uptr user_id, bool detached, u32 parent_tid, u32 stack_tid,
+                   void *arg);
+  u32 CreateThread(uptr user_id, bool detached, u32 parent_tid, void *arg) {
+    return CreateThread(user_id, detached, parent_tid, 0, arg);
+  }
 
   typedef void (*ThreadCallback)(ThreadContextBase *tctx, void *arg);
   // Invokes callback with a specified arg for each thread context.
diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_thread_registry_test.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_thread_registry_test.cpp
index 8c4b4ba5c58f7d..c3cac707f0a0b3 100644
--- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_thread_registry_test.cpp
+++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_thread_registry_test.cpp
@@ -64,11 +64,12 @@ static void MarkUidAsPresent(ThreadContextBase *tctx, void *arg) {
 
 static void TestRegistry(ThreadRegistry *registry, bool has_quarantine) {
   // Create and start a main thread.
-  EXPECT_EQ(0U, registry->CreateThread(get_uid(0), true, -1, 0));
+  EXPECT_EQ(0U, registry->CreateThread(get_uid(0), true, -1, 0, nullptr));
   registry->StartThread(0, 0, ThreadType::Regular, 0);
   // Create a bunch of threads.
   for (u32 i = 1; i <= 10; i++) {
-    EXPECT_EQ(i, registry->CreateThread(get_uid(i), is_detached(i), 0, 0));
+    EXPECT_EQ(i, registry->CreateThread(get_uid(i), is_detached(i), 100 + i,
+                                        200 + i, nullptr));
   }
   CheckThreadQuantity(registry, 11, 1, 11);
   // Start some of them.
@@ -88,19 +89,27 @@ static void TestRegistry(ThreadRegistry *registry, bool has_quarantine) {
   std::vector<u32> new_tids;
   for (u32 i = 11; i <= 15; i++) {
     new_tids.push_back(
-        registry->CreateThread(get_uid(i), is_detached(i), 0, 0));
+        registry->CreateThread(get_uid(i), is_detached(i), 0, 0, nullptr));
   }
   ASSERT_LE(kRegistryQuarantine, 5U);
-  u32 exp_total = 16 - (has_quarantine ? 5 - kRegistryQuarantine  : 0);
+  u32 exp_total = 16 - (has_quarantine ? 5 - kRegistryQuarantine : 0);
   CheckThreadQuantity(registry, exp_total, 6, 11);
   // Test SetThreadName and FindThread.
   registry->SetThreadName(6, "six");
   registry->SetThreadName(7, "seven");
-  EXPECT_EQ(7U, registry->FindThread(HasName, (void*)"seven"));
+  EXPECT_EQ(7U, registry->FindThread(HasName, (void *)"seven"));
   EXPECT_EQ(kInvalidTid, registry->FindThread(HasName, (void *)"none"));
-  EXPECT_EQ(0U, registry->FindThread(HasUid, (void*)get_uid(0)));
-  EXPECT_EQ(10U, registry->FindThread(HasUid, (void*)get_uid(10)));
+  EXPECT_EQ(0U, registry->FindThread(HasUid, (void *)get_uid(0)));
+  EXPECT_EQ(10U, registry->FindThread(HasUid, (void *)get_uid(10)));
   EXPECT_EQ(kInvalidTid, registry->FindThread(HasUid, (void *)0x1234));
+  EXPECT_EQ(7U,
+            registry->FindThread([](ThreadContextBase *tctx,
+                                    void *) { return tctx->parent_tid == 107; },
+                                 nullptr));
+  EXPECT_EQ(8U,
+            registry->FindThread([](ThreadContextBase *tctx,
+                                    void *) { return tctx->stack_id == 208; },
+                                 nullptr));
   // Detach and finish and join remaining threads.
   for (u32 i = 6; i <= 10; i++) {
     registry->DetachThread(i, 0);

Copy link

github-actions bot commented Oct 10, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

wzssyqa and others added 2 commits October 10, 2024 17:54
Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
@vitalybuka vitalybuka requested a review from fmayer October 11, 2024 00:54
@vitalybuka vitalybuka changed the base branch from users/vitalybuka/spr/main.nfcsanitizerasan-promote-stack_id-into-threadcontextbase to main October 11, 2024 00:55
Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
Created using spr 1.3.4
Created using spr 1.3.4
Created using spr 1.3.4
@vitalybuka vitalybuka merged commit abe148a into main Oct 11, 2024
7 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/nfcsanitizerasan-promote-stack_id-into-threadcontextbase branch October 11, 2024 21:47
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
…111917)

`parent_id` and `stack_id` represent location
where the thread was created, so it's reasonable
to keep them togeter.

For now, only Asan and MemProf use `stack_id`,
but it will be halpfull to print thread origin from
other sanitizers as well.

For llvm#111948
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
…111917)

`parent_id` and `stack_id` represent location
where the thread was created, so it's reasonable
to keep them togeter.

For now, only Asan and MemProf use `stack_id`,
but it will be halpfull to print thread origin from
other sanitizers as well.

For llvm#111948
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler-rt:asan Address sanitizer compiler-rt:sanitizer compiler-rt PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants