This is an archive of the discontinued LLVM Phabricator instance.

[NFC][sanitizer] Add class to track thread arg and retval
ClosedPublic

Authored by vitalybuka on May 8 2023, 12:25 AM.

Details

Summary

We need something to keep arg and retval pointers for leak checking.
Pointers should keept alive even after thread exited, until the thread
is detached or joined.
We should not put this logic into ThreadRegistry as we need the the
same for the ThreadList of HWASAN.

Diff Detail

Event Timeline

vitalybuka created this revision.May 8 2023, 12:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2023, 12:25 AM
Herald added a subscriber: Enna1. · View Herald Transcript
vitalybuka requested review of this revision.May 8 2023, 12:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2023, 12:25 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added a subscriber: dvyukov.

@dvyukov ThreadArgRetval::Join is alternative to ThreadRegistry::ConsumeThreadUserId+ThreadRegistry::SetThreadUserId

BTW. I tried to extend ThreadRegistry::live_ but it didn't look good.
Also we need these functionality for HWASAN without Registry anyway.
We could make ThreadArgRetval track pthread -> Tids mapping as well.
However live_ is tsan only and ThreadArgRetval is asan/lsan/hwasan only, so I decided to keep them separated for now.

Enna1 added inline comments.May 8 2023, 8:12 PM
compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp
21

call CheckLocked(); in CreateLocked ?

66

typo: Thread

compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.h
55

Please keep singular and plural consistent, "Mark", "stores"

69

typo: thread

compiler-rt/lib/sanitizer_common/tests/sanitizer_thread_arg_retval_test.cpp
122

typo: thread

thurston added inline comments.May 10 2023, 4:38 PM
compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.cpp
42–43

If it can't find the thread, why does it quietly return instead of aborting?
Other functions are stricter with 'CHECK(t);'.

46

Nit: 'retrieved'

vitalybuka marked 7 inline comments as done.

update

thurston accepted this revision.May 11 2023, 10:39 AM
This revision is now accepted and ready to land.May 11 2023, 10:39 AM
This revision was landed with ongoing or failed builds.May 11 2023, 2:44 PM
This revision was automatically updated to reflect the committed changes.
eugenis added inline comments.May 11 2023, 3:03 PM
compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.h
87

isn't that already done, in the form of Lock/Unlock?