This is an archive of the discontinued LLVM Phabricator instance.

[LSAN] More LSAN interface tweaking.
ClosedPublic

Authored by kstoimenov on Dec 14 2022, 11:25 AM.

Details

Summary

Main goal is to remove thread registry dependency from the interface because HWASAN is using its own code to manage threads.

Diff Detail

Event Timeline

kstoimenov created this revision.Dec 14 2022, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 11:25 AM
kstoimenov requested review of this revision.Dec 14 2022, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 11:25 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.
compiler-rt/lib/asan/asan_thread.cpp
553

I am not sure I like that not suspended/unsuspended logic is exposed to asan

I am not sure what is the problem here for HWASAN
It's going to use ThreadContextBase anyway?

fmayer added a subscriber: fmayer.Dec 14 2022, 12:09 PM

nit: move second sentence of commit message into separate paragraph.

kstoimenov retitled this revision from [LSAN] More LSAN interface tweaking. Main goal is to remove thread registry dependency from the interface because HWASAN is using it's own code to manage threads. to [LSAN] More LSAN interface tweaking. .Dec 14 2022, 12:31 PM
kstoimenov edited the summary of this revision. (Show Details)
kstoimenov added a comment.EditedDec 14 2022, 12:44 PM

I am not sure what is the problem here for HWASAN
It's going to use ThreadContextBase anyway?

Currently HWASAN is using HwasanThreadList to manage the threads. I am thinking that I could adapt this class to do what the ThreadRegistry does, but for that I will have to clean up the interface. So I am trying to get rid of anything that is ThreadRegistry related including ThreadContextBase.

vitalybuka added inline comments.Jan 5 2023, 2:30 PM
compiler-rt/lib/asan/asan_thread.cpp
553

Probably it would be nicer if you have
oid RunCallbackForEachRunningThreadLocked(SomeCBWithoutThreadContext

PTAL. This patch should go in before the rest of the code.

vitalybuka added inline comments.Jan 9 2023, 3:18 PM
compiler-rt/lib/asan/asan_thread.cpp
553

Probably it would be nicer if you have
oid RunCallbackForEachRunningThreadLocked(SomeCBWithoutThreadContext

Why you don't want to go this way?

kstoimenov added inline comments.Jan 11 2023, 12:46 PM
compiler-rt/lib/asan/asan_thread.cpp
553

In some cases like for example GetAdditionalThreadContextPtrs for the LSAN implementation the actual callback is empty. In the current implementation we would still enumerate all threads and call an empty callback which is less efficient.

vitalybuka added inline comments.Jan 11 2023, 3:41 PM
compiler-rt/lib/asan/asan_thread.cpp
553

Which implementations?

Also even in extremely thread apps, number of threads is a relatively small number

kstoimenov added inline comments.Jan 11 2023, 4:02 PM
compiler-rt/lib/asan/asan_thread.cpp
553

What I mean is that if we use RunCallbackForEachThreadLocked and the callback is empty we will still run a loop over all threads and call an empty callback. Also it is not only about the loop, but we also lock the allocator when we run the loop.

Let me try to do it your way.

kstoimenov retitled this revision from [LSAN] More LSAN interface tweaking. to [LSAN] More LSAN interface tweaking..

After pull.

kstoimenov added inline comments.Jan 12 2023, 9:21 AM
compiler-rt/lib/asan/asan_thread.cpp
553

I tried using exposing generic RunCallbackForEachRunningThreadLocked, but it is very kludgy. In this case RunCallbackForEachRunningThreadLocked and each callback have to be implemented by individual client (hwasan, asan, lsan). This leads to a problem with the callback because to implement RunCallbackForEachRunningThreadLocked I need to cast the generic callback that is passed into the concrete type, but this is not possible. I tried using a lambda, but I need to capture the original callback and according to the C++ standard this cannot be use and function pointer. See D141621 for details.

vitalybuka accepted this revision.Jan 12 2023, 1:40 PM

if you agree with the last version then, please "arc patch D140039" and submit

This revision is now accepted and ready to land.Jan 12 2023, 1:40 PM
kstoimenov accepted this revision.Jan 12 2023, 2:20 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jan 12 2023, 5:23 PM

Looks like this doesn't build on Windows: http://45.33.8.238/win/73274/step_4.txt

Please take a look and revert for now if it takes a while to fix.

Looks like this doesn't build on Windows: http://45.33.8.238/win/73274/step_4.txt

Please take a look and revert for now if it takes a while to fix.

If you revert instead of a fix forward would you mind reverting rGf001e50f955c3cdf2deb79e38a9fd19c9a781882 too?

vitalybuka reopened this revision.Jan 12 2023, 5:30 PM

Looks like this doesn't build on Windows: http://45.33.8.238/win/73274/step_4.txt

Please take a look and revert for now if it takes a while to fix.

If you revert instead of a fix forward would you mind reverting rGf001e50f955c3cdf2deb79e38a9fd19c9a781882 too?

I'll revert

This revision is now accepted and ready to land.Jan 12 2023, 5:30 PM
vitalybuka updated this revision to Diff 488828.EditedJan 12 2023, 5:53 PM

reupload correct snapshot

This revision was landed with ongoing or failed builds.Jan 12 2023, 5:58 PM
This revision was automatically updated to reflect the committed changes.

Hmm, the update I did to lsan_common_fuchsia.cpp is fine for clang_rt.lsan, but unfortunately it breaks clang_rt.asan. I'm pretty sure adding lsan_thread.cpp to LSAN_COMMON_SOURCES is wrong, but it isn't even tenable because then clang_rt.asan gets multiple definition errors between lsan_thread.cpp and asan_thread.cpp. I'm wondering what the best way to go about this is? In lsan we want to use GetLsanThreadRegistryLocked but in asan we want GetAsanThreadRegistryLocked instead. Though this seems erroneous. Do folks have any other suggestions?

Hmm, the update I did to lsan_common_fuchsia.cpp is fine for clang_rt.lsan, but unfortunately it breaks clang_rt.asan. I'm pretty sure adding lsan_thread.cpp to LSAN_COMMON_SOURCES is wrong, but it isn't even tenable because then clang_rt.asan gets multiple definition errors between lsan_thread.cpp and asan_thread.cpp. I'm wondering what the best way to go about this is? In lsan we want to use GetLsanThreadRegistryLocked but in asan we want GetAsanThreadRegistryLocked instead. Though this seems erroneous. Do folks have any other suggestions?

Do you have a link with the error you are getting?