labath added a comment.
This is going to be pretty big, so I'd say we start splitting this up. The
client & server bits can go into separate patches, and it would make things
simpler if the qSupported emission and parsing also got its own patches, as
that is going to be a lot less controversial.
I'm going to have more questions, but let me start with this: What should the
(v)fork-events extension actually mean? In the client, I guess it demonstrates
its willingness to receive the appropriate type of event. But what should it
mean on the server? Is it like "I promise to stop on every (v)fork", or just
something like "hey, I know this word"? The second meaning only makes sense if
these (v)fork-events come with some new protocol extensions/packets that the
client can use when communicating with the server. Such is the case with the
multiprocess extension (new thread id syntax), but I am not aware of anything
similar in for these features. Are there any? The gdb documentation (//This
feature indicates whether GDB supports fork event extensions to the remote
protocol. GDB does not use such extensions unless the stub also reports that it
supports them by including ‘fork-events+’ in its ‘qSupported’ reply. //) seems
to indicate they might exist, but I am not sure what would they be...
If there aren't any, then it would probably be better to use the first
"definition" of the feature (or something similar), as the client can
legitimately be interested in whether it will be able to intercept forks or
not. And if that's the case, then we should first consult the relevant
NativeProcess class before we reply (v)fork-events+.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:455
+ if (::strstr(response_cstr, "fork-events+"))
+ m_supports_fork_events = eLazyBoolYes;
----------------
mgorny wrote:
> Kamil noticed that this will also catch `vfork-events+`. I suppose I'll
> rewrite this function to iterate over split data as well.
Yeah, that should have been done a long time ago.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:1314-1322
+ "qXfer:features:read+",
+#if defined(__linux__) || defined(__NetBSD__) || defined(__FreeBSD__)
+ "QPassSignals+",
+ "qXfer:auxv:read+",
+ "qXfer:libraries-svr4:read+",
+#endif
+ "multiprocess+",
----------------
Ideally, all of these would be handled in the GDBRemoteCommunicationServerLLGS
class, since these features are meaningless for the platform server. Then you
also wouldn't need to define the m_(v)fork_events_supported members in this
class.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h:152
+ // to be returned in response.
+ virtual llvm::SmallVector<std::string, 16>
+ HandleFeatures(const llvm::ArrayRef<llvm::StringRef> client_features);
----------------
A SmallVector as a return type is fairly odd. One tends to use SmallVectorImpl
by-ref argument in this case, but a plain std::vector result is also fine.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2342-2344
+ StreamString ostr;
+ ostr.Printf("%" PRIu64 " %" PRIu64, pid_tid->first, pid_tid->second);
+ description = std::string(ostr.GetString());
----------------
description = llvm::formatv("{0} {1}", pid_tid->first, pid_tid->second).str()
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99864/new/
https://reviews.llvm.org/D99864
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
