This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [Core] Split read thread support into ThreadedCommunication
ClosedPublic

Authored by mgorny on Sep 3 2022, 1:34 AM.

Details

Summary

Split the read thread support from Communication into a dedicated
ThreadedCommunication subclass. The read thread support is used only
by a subset of Communication consumers, and it adds a lot of complexity
to the base class. Furthermore, having a dedicated subclass makes it
clear whether a particular consumer needs to account for the possibility
of read thread being running or not.

The modules currently calling StartReadThread() are updated to use
ThreadedCommunication. The remaining modules use the simplified
Communication class.

SBCommunication is changed to use ThreadedCommunication in order
to avoid changing the public API.

CommunicationKDP is updated in order to (hopefully) compile with
the new code. However, I do not have a Darwin box to test it, so I've
limited the changes to the bare minimum.

GDBRemoteCommunication is updated to become a Broadcaster directly.
Since it does not inherit from ThreadedCommunication, its event
support no longer collides with the one used for read thread and can
be implemented cleanly. The support for
eBroadcastBitReadThreadDidExit is removed from the code -- since
the read thread was not used, this event was never reported.

Sponsored by: The FreeBSD Foundation

Diff Detail

Event Timeline

mgorny created this revision.Sep 3 2022, 1:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2022, 1:34 AM
Herald added a subscriber: arichardson. · View Herald Transcript
mgorny requested review of this revision.Sep 3 2022, 1:34 AM
labath added a comment.Sep 5 2022, 5:34 AM

I like this a lot.

lldb/source/Core/CMakeLists.txt
57

I think you're missing this file.

mgorny updated this revision to Diff 457984.Sep 5 2022, 7:11 AM

Add the missing file.

mgorny marked an inline comment as done.Sep 5 2022, 7:11 AM
mgorny added inline comments.
lldb/source/Core/CMakeLists.txt
57

Indeed I were. Thanks for noticing this before I did git clean -dfx ;-).

labath accepted this revision.Sep 6 2022, 2:17 AM
This revision is now accepted and ready to land.Sep 6 2022, 2:17 AM
This revision was automatically updated to reflect the committed changes.
mgorny marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2022, 4:09 AM

CommunicationKDP is updated in order to (hopefully) compile with the new code. However, I do not have a Darwin box to test it, so I've limited the changes to the bare minimum.

Compilation fails:

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp:232:47: error: unknown type name 'm_bytes_mutex'
  std::lock_guard<std::recursive_mutex> guard(m_bytes_mutex);

Should we revert?

I created D133365, which solves the compilation issue but I don't know whether it's semantically correct.