I see. In that case, at least store it in a std::unique_ptr.
On Mon, May 29, 2017 at 12:22 AM Ravitheja Addepally via Phabricator <
revi...@reviews.llvm.org> wrote:
> ravitheja added a comment.
>
> Hello, the reason I switched to allocating with new (std::nothrow)
> uint8_t[byte_count]; was beca
ravitheja added a comment.
Hello, the reason I switched to allocating with new (std::nothrow)
uint8_t[byte_count]; was because the byte_count is actually user defined and I
do want to catch cases where lldb is not able to allocate the requested buffer
size.
https://reviews.llvm.org/D32585
clayborg added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1304
+ // Allocate the response buffer.
+ uint8_t *buffer = new (std::nothrow) uint8_t[byte_count];
+ if (buffer == nullptr)
labath wrote:
> Hey
Or just use a std vector
On Fri, May 26, 2017 at 7:00 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:
> labath added inline comments.
>
>
>
> Comment at:
> source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1304
> + // Allocate the response
labath added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1304
+ // Allocate the response buffer.
+ uint8_t *buffer = new (std::nothrow) uint8_t[byte_count];
+ if (buffer == nullptr)
Hey, ravi. You're lea
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
If all R"( strings are under 80 characters, this is good to go. If not, just
fix and submit, no extra review needed.
https://reviews.llvm.org/D32585
___
labath accepted this revision.
labath added a comment.
Looks good as far as I am concerned (please wait for greg's ok though)
https://reviews.llvm.org/D32585
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mai
ravitheja updated this revision to Diff 100051.
ravitheja added a comment.
Running clang-format.
https://reviews.llvm.org/D32585
Files:
docs/lldb-gdb-remote.txt
include/lldb/API/SBTrace.h
include/lldb/Core/TraceOptions.h
include/lldb/Host/common/NativeProcessProtocol.h
include/lldb/Ta
clayborg added a comment.
Looks like we are down to just running clang format on the code so the R"(
strings don't go over 80 chars and this is good to go from me.
https://reviews.llvm.org/D32585
___
lldb-commits mailing list
lldb-commits@lists.llv
ravitheja updated this revision to Diff 99891.
ravitheja added a comment.
Modifying string literals in Unit tests.
https://reviews.llvm.org/D32585
Files:
docs/lldb-gdb-remote.txt
include/lldb/API/SBTrace.h
include/lldb/Core/TraceOptions.h
include/lldb/Host/common/NativeProcessProtocol.h
labath added inline comments.
Comment at:
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:400
+
+ HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"jparams\" :
{\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"metabuffersize\" :
8192,\"threadid\" : 35,\"
ravitheja added inline comments.
Comment at:
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:400
+
+ HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"jparams\" :
{\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"metabuffersize\" :
8192,\"threadid\" : 35
labath added inline comments.
Comment at:
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:400
+
+ HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"jparams\" :
{\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"metabuffersize\" :
8192,\"threadid\" : 35,\"
ravitheja updated this revision to Diff 99870.
ravitheja added a comment.
Updates from last review.
https://reviews.llvm.org/D32585
Files:
docs/lldb-gdb-remote.txt
include/lldb/API/SBTrace.h
include/lldb/Core/TraceOptions.h
include/lldb/Host/common/NativeProcessProtocol.h
include/lldb
ravitheja added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3293
+ if (custom_params_sp) {
+if (custom_params_sp->GetType() !=
StructuredData::Type::eTypeDictionary) {
+ error.SetErrorString("Invalid Conf
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Close, some minor fixes.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3293
+ if (custom_params_sp) {
+if (custom
ravitheja added inline comments.
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:352
+ lldb::tid_t thread = LLDB_INVALID_THREAD_ID) {
+return Status("Not implemented");
+ }
krytarowski wrote:
> This is Linuxism. Not ever
krytarowski added inline comments.
Comment at: include/lldb/Host/common/NativeProcessProtocol.h:352
+ lldb::tid_t thread = LLDB_INVALID_THREAD_ID) {
+return Status("Not implemented");
+ }
This is Linuxism. Not every OS can trace on p
labath added a comment.
Thank you for taking the time to write the test. Just a couple of more comments
on things I noticed when going through this again:
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3172
+ if (custom_params)
+json_packe
ravitheja added a comment.
Sorry I had to update, since the Error class has been changed to Status.
https://reviews.llvm.org/D32585
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
ravitheja updated this revision to Diff 99716.
ravitheja added a comment.
Adding unit tests and making the packets fully JSON.
https://reviews.llvm.org/D32585
Files:
docs/lldb-gdb-remote.txt
include/lldb/API/SBTrace.h
include/lldb/Core/TraceOptions.h
include/lldb/Host/common/NativeProce
labath added a comment.
In https://reviews.llvm.org/D32585#758391, @ravitheja wrote:
> Well nothings preventing me from doing so, I have the changes for that were
> suggested to me for this revision. I thought I would first upload them, so
> that people can look at the parallel while I upload t
ravitheja added a comment.
Well nothings preventing me from doing so, I have the changes for that were
suggested to me for this revision. I thought I would first upload them, so that
people can look at the parallel while I upload the linux server code and Unit
tests.
https://reviews.llvm.org/
labath added a comment.
llvm policy is to commit tests alongside the code under test. I also think it's
easier to review as you have the code and the test on the same screen.
What's the reason that prevents you from doing that?
https://reviews.llvm.org/D32585
___
ravitheja added a comment.
> TraceOptions opt;
> opt.setType(...);
> opt.setBufferSize(...);
> opt.setMetaBufferSize(); // with an appropriate TraceOptions constructor,
> this could be a one-liner
> std::future result = std::async(std::launch::async, [&] {
> return client.StartT
labath added a comment.
In https://reviews.llvm.org/D32585#752115, @ravitheja wrote:
> In https://reviews.llvm.org/D32585#740632, @labath wrote:
>
> > I quite like that you have added just the packet plumbing code without an
> > concrete implementation. However, that is still a significant amoun
zturner added inline comments.
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1125
+uint64_t extracted_value;
+value.getAsInteger(16, extracted_value);
+
Should you be checking the return value of `getAsInteger` here?
clayborg added a comment.
So my question is: if we go the jTrace packet route, I would like to not use
GDB remote key/value pairs, and just go full JSON. You can make a new JSON dict
and pass all of the GDB remote key/value pairs inside and then you can add a
"jparams" key whose value is the JS
ravitheja marked 2 inline comments as done.
ravitheja added a comment.
In https://reviews.llvm.org/D32585#740632, @labath wrote:
> 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 t
ravitheja marked 28 inline comments as done.
ravitheja added inline comments.
Comment at: docs/lldb-gdb-remote.txt:212
//--
+// QTrace:1:type:;
+//
clayborg wrote:
> labath wrote:
> > ravitheja
ravitheja updated this revision to Diff 98599.
ravitheja added a comment.
Changes for the feedback recieved in first round of review.
https://reviews.llvm.org/D32585
Files:
docs/lldb-gdb-remote.txt
include/lldb/API/SBTrace.h
include/lldb/Core/TraceOptions.h
include/lldb/Host/common/Nati
zturner added inline comments.
Comment at: docs/lldb-gdb-remote.txt:214
+//
+// BRIEF
+// Packet for starting tracing of type lldb::TraceType. The following
I just noticed that none of our documentation uses doxygen? Weird.
Comment at: includ
clayborg added inline comments.
Comment at: docs/lldb-gdb-remote.txt:212
//--
+// QTrace:1:type:;
+//
labath wrote:
> ravitheja wrote:
> > clayborg wrote:
> > > Should we make all these new pack
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
ravitheja added inline comments.
Comment at: docs/lldb-gdb-remote.txt:212
//--
+// QTrace:1:type:;
+//
clayborg wrote:
> Should we make all these new packets JSON based to start with? "jTrace"?
clayborg added a comment.
This patch does nicely follow the way other GDB remote packets are implemented.
I wonder if we should just have a "jTrace" packet that is JSON from the start?
This is more of a question to anyone that cares about the direction of the GDB
remote protocol we are using. W
36 matches
Mail list logo
