This is an archive of the discontinued LLVM Phabricator instance.

[lsan] Add interceptor for pthread_detach.
ClosedPublic

Authored by charco on Sep 23 2020, 2:47 PM.

Details

Summary

This commit adds an interceptor for the pthread_detach function,
calling into ThreadRegistry::DetachThread, allowing for thread contexts
to be reused.

Without this change, programs may fail when they create more than 8K
threads.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=47389

Diff Detail

Event Timeline

charco created this revision.Sep 23 2020, 2:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2020, 2:47 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
charco requested review of this revision.Sep 23 2020, 2:47 PM
charco updated this revision to Diff 293876.Sep 23 2020, 3:26 PM

Add test for spawning multiple threads.

LGTM but I'll leave approval to one of the maintainers.
Thanks for the fix!

lsan patch is LGTM but I am not sure how this test verity that it's actually reused

lsan patch is LGTM but I am not sure how this test verity that it's actually reused

LSan sets the thread registry's maximum number of threads to 8192 (static const uptr kMaxThreads = 1 << 13; in lsan_thread.cpp).

This test exercises various ways threads can be created and then reaped, and it ensures each can successfully run for 10,000 thread create/reap cycles. Without the associated LSan fix, some of the paths end up failing due to hitting LSan's thread limit.

@charco We should probably add a comment to kTestThreads explaining this.

(I think it's out of scope for this CL, but it would also be nice if, for testing, we could bump down to thread registry limits. E.g., ASan also leaks threads, but it sets a *much* higher thread limit. So running this test long enough to demonstrate ASan's leaks takes a non-trivial amount of time.)

charco updated this revision to Diff 294179.Sep 24 2020, 2:56 PM
  • Add tests for spawning multiple threads.
  • Add comment for max number of threads.
vitalybuka added inline comments.Sep 24 2020, 7:33 PM
compiler-rt/test/lsan/TestCases/many_threads.cpp
3–4

this way easier to copy full command line from test output when it fails :)

13

lsan_thread?

95

How long this test takes?

I tested and it's now the slowest lsan test.
Looks like this is enough and make the test faster.

// Test that threads are reused.
// RUN: %clangxx_lsan %s -o %t -lpthread && %run %t

#include <pthread.h>
#include <stdlib.h>

// Number of threads to create. This value is greater than kMaxThreads in
// lsan_thread.cpp so that we can test that thread contexts are not being
// reused.
static const size_t kTestThreads = 10000;

void *null_func(void *args) {
  return NULL;
}

void detach_exited(void) {
  for (size_t i = 0; i < kTestThreads; i++) {
    pthread_t thread;
    pthread_create(&thread, NULL, null_func, NULL);
    pthread_detach(thread);
  }
}

int main() {
  detach_exited();
  return 0;
}
compiler-rt/test/lsan/TestCases/many_threads.cpp
4

2>&1 is not not needed without FileCheck

vitalybuka accepted this revision.Sep 24 2020, 8:08 PM

LGTM with simplified test

This revision is now accepted and ready to land.Sep 24 2020, 8:08 PM
vitalybuka added inline comments.Sep 24 2020, 8:10 PM
compiler-rt/test/lsan/TestCases/many_threads.cpp
16

testing only is probably not good enough justification for the option.
so could you please remove it as well?

charco updated this revision to Diff 294424.Sep 25 2020, 2:18 PM
  • simplify test.
This revision was landed with ongoing or failed builds.Sep 25 2020, 2:22 PM
This revision was automatically updated to reflect the committed changes.