Page MenuHomePhabricator

[lldb] [Process] Introduce protocol extension support API
ClosedPublic

Authored by mgorny on Apr 8 2021, 4:21 PM.

Details

Summary

Introduce a NativeProcessProtocol API for indicating support for
protocol extensions and enabling them. LLGS calls
GetSupportedExtensions() method on the process factory to determine
which extensions are supported by the plugin. If the future is both
supported by the plugin and reported as supported by the client, LLGS
enables it and reports to the client as supported by the server.

The extension is enabled on the process instance by calling
SetEnabledExtensions() method. This is done after qSupported exchange
(if the debugger is attached to any process), as well as after launching
or attaching to a new inferior.

The patch adds 'fork' extension corresponding to 'fork-events+'
qSupported feature and 'vfork' extension for 'vfork-events+'.

Diff Detail

Event Timeline

mgorny requested review of this revision.Apr 8 2021, 4:21 PM
mgorny created this revision.

Note that I've included full fork-events+ and vfork-events+ as demo of the API. I can split them further and/or move to more relevant commits as I progress with the split.

mgorny updated this revision to Diff 336263.Apr 8 2021, 4:28 PM

Add to _KNOWN_QSUPPORTED_STUB_FEATURES and clang-format.

mgorny updated this revision to Diff 336516.Apr 9 2021, 10:44 AM

Fix uninitialized flags variable in GDBRemoteCommunicationServerLLGS::SetEnabledExtensions().

I like that (particularly the dependency graph :P). Let's just figure out the error handling..

Note that I've included full fork-events+ and vfork-events+ as demo of the API. I can split them further and/or move to more relevant commits as I progress with the split.

Maybe we could start by feature-ifying one of the existing fields currently controlled by ifdefs. Say, QPassSignals+. It does not have a client component, so it won't cover everything, but I think it will give us something.

lldb/include/lldb/Host/common/NativeProcessProtocol.h
359

Are you sure that returning success is the best "default" behavior? Maybe the base implementation should always return an error (as it does not implement any extensions)? Or return success, only if one enables an empty set of extensions?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3612–3615

... or actually, I am wondering if this should not be a hard error/assertion. In the current set up, I think it would be a programmer error if the factory reports an extension as supported, but then the instance fails to enable it...

mgorny added inline comments.Apr 13 2021, 2:45 AM
lldb/include/lldb/Host/common/NativeProcessProtocol.h
359

I'm not sure whether we need error handling here at all.

The current impl doesn't do anything but setting an instance var here. The original impl controlled events to PTRACE_SETOPTIONS but in the end I've decided to enable tracing fork/vfork unconditionally, and just using extensions to decide whether to handle it locally or defer to client.

I suppose it might make sense to review other patches first and get back to this one once we're sure what we need.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3612–3615

Technically, the original idea was that this would fail if ptrace() failed to set options. But as I said, this is no longer the case, so I'm not even sure if we need any error reporting here.

mgorny updated this revision to Diff 337122.Apr 13 2021, 6:00 AM

Rebase + clang-format.

labath added inline comments.Apr 21 2021, 2:52 AM
lldb/include/lldb/Host/common/NativeProcessProtocol.h
359

I think I agree with you. Let's just drop error handling altogether. Maybe add something like assert(features && ~m_process_factory.GetSupportedExtensions() == {}) to GDBRemoteCommunicationServerLLGS::SetEnabledExtensions.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3583–3588

Maybe drop the == Extension::[v]fork part?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
106–107

I am wondering if this should just be NativeProcessProtocol::Extension m_enabled_extensions; Can we think of an extension that would belong to NativeProcessProtocol::Extension, but we would not want to store its state in the GDBRemoteCommunicationServerLLGS class?

mgorny added inline comments.Apr 21 2021, 3:17 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3583–3588

Can't do that, enum class doesn't convert to bool. In fact, I tried a few more or less crazy ideas to make this work, and none worked ;-).

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
106–107

I can't think of any right now and I suppose even if there were one, there would probably be no big loss in using Extension here.

mgorny marked 4 inline comments as done.Apr 21 2021, 3:41 AM
mgorny added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3583–3588

The next best thing I was able to do is check != NativeProcessProtocol::Extension() which is a little bit shorter. Maybe I could try adding operator== and != against, say, nullptr or some special constant?

mgorny marked an inline comment as done.Apr 21 2021, 3:44 AM
labath added inline comments.Apr 21 2021, 4:10 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
3583–3588

An explicit bool cast works though, and is less fancy. Maybe if we combine this the m_enabled_extensions idea, we could do something like:

using Extension = NativeProcessProtocol::Extension;
Extension client_extensions{};
for (StringRef x : client_features)
  client_extensions |= llvm::StringSwitch<Extension>(x).Case("fork-events+", Extension::fork).Case("vfork-events+", Extension::vfork).Default({});
m_enabled_extensions = client_extensions & m_process_factory.GetSupportedExtensions();
if (bool(m_enabled_extensions & Extension::fork))
  ret.push_back("fork-events+");
if (bool(m_enabled_extensions & Extension::vfork))
  ret.push_back("vfork-events+");
mgorny updated this revision to Diff 339193.Apr 21 2021, 5:19 AM
mgorny marked an inline comment as done.

Implemented the suggested changes, including bool() use in SetEnabledExtensions(). Removed the client announcements of fork-events and vfork-events support.

I've left the server parts of fork-events and vfork-events. It won't trigger until both the process plugin and the client indicates support too.

labath accepted this revision.Apr 23 2021, 12:24 PM
This revision is now accepted and ready to land.Apr 23 2021, 12:24 PM
mgorny updated this revision to Diff 340177.Apr 23 2021, 3:19 PM

Added Extension bit for multiprocess. Made fork-events and vfork-events both depend on it — i.e. be implicitly disabled if multiprocess wasn't reported as supported.

Simplify GDBRemoteCommunicationServerLLGS::SetEnabledExtensions() — we can just take the value instead of copying it bit-by-bit ;-).

Move add_qSupported_packets() change from later patch. Add tests for qSupported responses (particularly, rejecting [v]fork-events if multiprocess is not present). The new tests are in fork category that's skipped on all platforms where fork/vfork isn't enabled yet (i.e. everywhere in this patch).

This revision was landed with ongoing or failed builds.Apr 24 2021, 2:26 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2021, 2:26 AM