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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
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...



More information about the lldb-commits mailing list