[Lldb-commits] [PATCH] D100153: [lldb] [Process] Introduce protocol extension support API

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:389
+    m_enabled_extensions = flags;
+    return llvm::Error::success();
+  }
----------------
mgorny wrote:
> labath wrote:
> > 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?
> 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.
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`.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3562-3567
+        (process_features & NativeProcessProtocol::Extension::fork) ==
+            NativeProcessProtocol::Extension::fork)
+      m_fork_events_supported = true;
+    else if (x == "vfork-events+" &&
+             (process_features & NativeProcessProtocol::Extension::vfork) ==
+                 NativeProcessProtocol::Extension::vfork)
----------------
Maybe drop the `== Extension::[v]fork` part?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:102-103
 
+  bool m_fork_events_supported = false;
+  bool m_vfork_events_supported = false;
+
----------------
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100153/new/

https://reviews.llvm.org/D100153