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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
