This is an archive of the discontinued LLVM Phabricator instance.

[dfsan] Add thread registration
ClosedPublic

Authored by stephan.yichao.zhao on Feb 3 2021, 11:52 AM.

Details

Summary
This is a part of https://reviews.llvm.org/D95835.

 This change is to address two problems
 1) When recording stacks in origin tracking, libunwind is not async signal safe. Inside signal callbacks, we need
 to use fast unwind. Fast unwind needs threads
 2) StackDepot used by origin tracking is not async signal safe, we set a flag per thread inside
 a signal callback to prevent from using it.

 The thread registration is similar to ASan and MSan.

 Related MSan changes are
 * https://github.com/llvm/llvm-project/commit/98f5ea0dbae664a2e5f9381a64f2913fe1add208
 * https://github.com/llvm/llvm-project/commit/f653cda2695ac7390fe5663f2c0895213938334d
 * https://github.com/llvm/llvm-project/commit/5a7c3643437c262137bd3dac7f6a0f5b9e8501be

 Some changes in the diff are used in the next diffs
 1) The test case pthread.c is not very interesting for now. It will be
   extended to test origin tracking later.
 2) DFsanThread::InSignalHandler will be used by origin tracking later.

Diff Detail

Event Timeline

stephan.yichao.zhao requested review of this revision.Feb 3 2021, 11:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2021, 11:52 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
stephan.yichao.zhao edited the summary of this revision. (Show Details)Feb 3 2021, 11:53 AM
stephan.yichao.zhao edited the summary of this revision. (Show Details)
stephan.yichao.zhao retitled this revision from [dfsan] Add thread registration to [dfsan] Add thread registration.

fixed broken builts

fixed clang-tidy errors

morehouse added inline comments.Feb 4 2021, 2:16 PM
compiler-rt/lib/dfsan/dfsan.cpp
434

Isn't the else block dead code? Getting past the first if block implies WillUseFastUnwind is true.

compiler-rt/lib/dfsan/dfsan_custom.cpp
486–488

Since it isn't used by anything else, let's inline dfsan_pthread_create here.

compiler-rt/lib/dfsan/dfsan_thread.cpp
27

nit

70

stack_switching_ is always false.

77

next_stack_ is never set anywhere.

109

Is the cast needed?

compiler-rt/lib/dfsan/dfsan_thread.h
55

If we use the proper function ptr type here and in the constructor, we can remove some unnecessary casting to void * and back.

compiler-rt/test/dfsan/pthread.c
19

Is the cast needed?

21

Nit: let's use a const variable instead of magic number 24.

stephan.yichao.zhao marked 8 inline comments as done.

addressed comments

compiler-rt/lib/dfsan/dfsan.cpp
434

Yes. This branch cannot happen. Removed.

compiler-rt/lib/dfsan/dfsan_custom.cpp
486–488

We will use it after origin tracking is added.

compiler-rt/lib/dfsan/dfsan_thread.cpp
70

removed

77

removed it with stack_switching_. In MSan/ASan threads, they expose APIs to allow users tell them when a fiber thread switch contexts. DFSan does not have the case yet.

109

removed.

compiler-rt/lib/dfsan/dfsan_thread.h
55

This start_routine_trampoline_ will have a different type with and without origin tracking like this

typedef void *(*thread_callback_trampoline_t)(void *, void *, dfsan_label,
                                              dfsan_label *);
typedef void *(*thread_callback_origin_trampoline_t)(
    void *, void *, dfsan_label, dfsan_label *, dfsan_origin, dfsan_origin *);

If we use different types here at the C code of dfsan_custom.c, we have to define two functions with similar code.
See the pthread_create wrappers in https://reviews.llvm.org/D95835#change-GvgDqkOr0aQn
Or maybe we can delay how to change this in the late origin tracking related change?

compiler-rt/test/dfsan/pthread.c
19

removed.

21

added kNumThreads=4.

Harbormaster completed remote builds in B87987: Diff 321563.
morehouse accepted this revision.Feb 5 2021, 8:08 AM
morehouse added inline comments.
compiler-rt/lib/dfsan/dfsan_thread.cpp
62

Please address this lint.

This revision is now accepted and ready to land.Feb 5 2021, 8:08 AM
compiler-rt/lib/dfsan/dfsan_thread.cpp
62

For some reason the local lint conflicts with the one used here, they propose two different styles... maybe they use different versions..?
For the submission, I will try make the local one happy, so it does not block check in. We will change the function in the next diffs. Then we will see if the conflict is gone.

This revision was landed with ongoing or failed builds.Feb 5 2021, 9:39 AM
This revision was automatically updated to reflect the committed changes.