labath added inline comments.
================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:389
+ m_enabled_extensions = flags;
+ return llvm::Error::success();
+ }
----------------
mgorny wrote:
> 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.
I think I agree with you. Let's just drop error handling altogether. Maybe add
something like `assert(features && ~m_process_factory.GetSupportedExtensions()
== {})` to `GDBRemoteCommunicationServerLLGS::SetEnabledExtensions`.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3562-3567
+ (process_features & NativeProcessProtocol::Extension::fork) ==
+ NativeProcessProtocol::Extension::fork)
+ m_fork_events_supported = true;
+ else if (x == "vfork-events+" &&
+ (process_features & NativeProcessProtocol::Extension::vfork) ==
+ NativeProcessProtocol::Extension::vfork)
----------------
Maybe drop the `== Extension::[v]fork` part?
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:102-103
+ bool m_fork_events_supported = false;
+ bool m_vfork_events_supported = false;
+
----------------
I am wondering if this should just be `NativeProcessProtocol::Extension
m_enabled_extensions;` Can we think of an extension that would belong to
`NativeProcessProtocol::Extension`, but we would not want to store its state in
the GDBRemoteCommunicationServerLLGS class?
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
