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