[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Fri Apr 28 04:13:27 PDT 2017
labath added a reviewer: zturner.
labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.
I am adding Zachary, as he usually has good ideas about APIs.
All in all, it's not a very controversal change, but I have a bunch of inline comments about the choice of API. They can be summed up as:
- use more ArrayRef
- use less shared pointers
- use LLDB_LOG
I quite like that you have added just the packet plumbing code without an concrete implementation. However, that is still a significant amount of parsing code that should be accompanied by a test. The test suite for the client side of the protocol is ready (TestGdbRemoteCommunicationClient), so I'd like to see at least that.
================
Comment at: docs/lldb-gdb-remote.txt:212
//----------------------------------------------------------------------
+// QTrace:1:type:<type>;
+//
----------------
ravitheja wrote:
> clayborg wrote:
> > Should we make all these new packets JSON based to start with? "jTrace"? If we have any need for JSON at all in this or the other new packets I would say lets just go with JSON packets. They are currently prefixed with "j". If we go this route we should specify the mandatory key/value pairs in the header doc. We should also allow a JSON dictionary from the trace config up at the SBTrace layer to make it into this packet?
> I am fine prefixing the packets with "j", I did not understand what you meant by the last sentence. At the moment I am waiting for Pavel or Tamas to reply or anyone else and based on the complete feedback, I will upload a new patch if needed.
I think Greg meant you should also change the packet format to take a json argument instead of key-value pairs (The packet should be JTrace if it modifies the state, right ?).
If there is a gdb equivalent which matches this functionality sufficiently closely, then I would prefer to stick to that. However, given that we already are attaching a json field to the packet, I think it would make sense to go json all the way.
================
Comment at: docs/lldb-gdb-remote.txt:236
+// metabuffersize The size of buffer to hold meta data M
+// used for decoding the trace data.
+// ========== ====================================================
----------------
The code seems to be sending some json dictionaries, but I see no mention of that here.
================
Comment at: docs/lldb-gdb-remote.txt:239
+//
+// Each tracing instance is identified by a user id which is returned
+// as the reply to this packet. In case the tracing failed to begin an
----------------
can we call this "trace id"? I'd like to avoid people confusing this with *real* UIDs.
================
Comment at: docs/lldb-gdb-remote.txt:244
+
+send packet: QTrace:1:type:<type>;buffersize:<buffersize>;
+read packet: <userid>/E<error code>
----------------
You seem to be putting "arguments" here, and you also have "mandatory" arguments down below. This is mostly a style issue, but I propose you either go with "put all mandatory arguments in the summary and only list optional arguments below", or with "list all arguments in the table below" approach.
================
Comment at: docs/lldb-gdb-remote.txt:269
+//
+// An empty response is sent in case of success else an error code is
+// returned.
----------------
Empty response means "packet not supported". You should send OK in this case.
================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:13
+#include "lldb/Core/TraceOptions.h"
#include "lldb/Host/MainLoop.h"
----------------
You are missing the traceOptions file from this patch.
================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:333
+ //------------------------------------------------------------------
+ virtual lldb::user_id_t StartTracing(lldb::tid_t thread, TraceOptions &config,
+ Error &error) {
----------------
Hard to say without seeing TraceOptions contents, but I would hope this argument can be const. If you want these functions to modify it, then maybe a different api would be called for.
================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:370
+ lldb::tid_t thread = LLDB_INVALID_THREAD_ID) {
+ Error error;
+ error.SetErrorString("Not implemented");
----------------
return Error("Not implemented");
================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:396
+ virtual size_t GetTraceData(lldb::user_id_t uid, lldb::tid_t thread,
+ Error &error, void *buf, size_t buf_size,
+ size_t offset = 0) {
----------------
llvm::MutableArrayRef<uint8_t> instead of buf+size pair. Maybe we could even pass in a reference to the MutableArrayRef so that the function can truncate it to the actual size?
================
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:407
+ virtual size_t GetTraceMetaData(lldb::user_id_t uid, lldb::tid_t thread,
+ Error &error, void *buf, size_t buf_size,
+ size_t offset = 0) {
----------------
ditto
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1143
+ StringExtractor value_extractor(value);
+ uint64_t extracted_value = value_extractor.GetU64(fail_value, 16);
+
----------------
There is a StringRef function which does this directly.
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1155
+ else {
+ StructuredData::ObjectSP key_object = StructuredData::ParseJSON(value.data());
+ custom_params->AddItem(name, key_object);
----------------
There seems to be very little error handling in here. Do you want the tracing to proceed in this was not valid json? Do you want the tracing to proceed if buffersize was not specified (you list it as mandatory).
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1171
+
+ options.setMetaDataBufferSize(metabuffersize);
+ options.setTraceBufferSize(buffersize);
----------------
If this is going to be a dumb struct, then do we need the getter/setter pairs?
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1179
+ if (tid != LLDB_INVALID_THREAD_ID)
+ uid = m_debugged_process_sp->StartTracing(tid, options, error);
+ else
----------------
What do you think about adding tid to the "options" class and just having one "StartTracing" function?
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1210
+ if (uid == fail_value || packet.GetChar('\0') != ';') {
+ if (log)
+ log->Printf("GDBRemoteCommunicationServerLLGS::%s IllFormed type",
----------------
`LLDB_LOG(log, "Illformed type")` is a much more concise way to write this.
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h:178
+ virtual lldb::user_id_t StartTrace(lldb::TraceOptionsSP &options,
+ Error &error) override;
----------------
`const TraceOptions &options` ? Having a reference to a shared pointer seems like a huge overkill here.
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h:185
+ virtual size_t GetData(lldb::user_id_t uid, lldb::tid_t thread_id,
+ Error &error, void *buf, size_t size,
+ size_t offset = 0) override;
----------------
MutableArrayRef
https://reviews.llvm.org/D32585
More information about the lldb-commits
mailing list