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

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.


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


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3592-3595
+  llvm::Error error = process.SetEnabledExtensions(flags);
+  if (error)
+    LLDB_LOG_ERROR(log, std::move(error),
+                   "Enabling protocol extensions failed: {0}");
----------------
labath wrote:
> ... 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...
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.


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

https://reviews.llvm.org/D100153