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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits