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

Tue Apr 13 00:10:49 PDT 2021

labath added a comment.

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

In D100153#2678223 <https://reviews.llvm.org/D100153#2678223>, @mgorny wrote:

> 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.

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

================
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}");
----------------
... 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...

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

https://reviews.llvm.org/D100153