Page MenuHomePhabricator

Implementation of remote packets for Trace data.
ClosedPublic

Authored by ravitheja on Apr 27 2017, 1:59 AM.

Details

Summary

The changes consist of new packets for trace manipulation and
trace collection. The new packets are also documented. The packets
are capable of providing custom trace specific parameters to start
tracing and also retrieve such configuration from the server.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
zturner added inline comments.Apr 28 2017, 9:02 AM
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
182–183

Return the Error instead of taking it as an output parameter.

186

Also, return the Error as before.

189–198

Same in all of these cases. Try to return Error objects whenever possible.

Also remove the virtual keyword from all of these. They are already marked override, so virtual is redundant.

ravitheja updated this revision to Diff 98599.May 11 2017, 2:14 AM
ravitheja edited edge metadata.

Changes for the feedback recieved in first round of review.

ravitheja marked 28 inline comments as done.May 11 2017, 2:25 AM
ravitheja added inline comments.
docs/lldb-gdb-remote.txt
212

Yes thats what is currently being done, the JSON Dictionary obtained at SB layer is being passed down to the server through the remote packets.

include/lldb/Host/common/NativeProcessProtocol.h
13

The traceOptions are already merged into lldb code, please refer to ->https://reviews.llvm.org/D29581

429–430

The threadid is already inside the TraceOptions, which needs to be set by the user, I will update the comments here.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1162

I think its not possible to use empty function coz it checks the size of m_packet string inside and not the current m_index.

1392

Not possible I think

source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
179

Yes u are right my mistake.

ravitheja marked 2 inline comments as done.May 11 2017, 4:47 AM

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.

@labath I was considering writing Unit tests for the remote packets but I thought then I have to write the mock implementation for the trace operations as well, which might end up being a bigger piece of code than the actual packet parsing code.

After this patch, I will upload the actual server side code doing the trace stuff for linux, that part of code has unit tests for some core functions.

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 JSON from the SB layer. JSON packets start with lower case j, not sure why some were upper cased?

docs/lldb-gdb-remote.txt
212

I think JSON packets start with lower case 'j'

217–218

Need to remove this bit about semicolon separates key/value pairs as everything is now in the JSON

239

Maybe state that all threads will be traced if this is not supplied?

256

So I am not sure why we are mixing GDB remote key:value; with JSON here? I was thinking the packet would be jTrace:XXXXX where XXXX is the JSON that contains all key value pairs and it also might have an extra jparams key whose value is the JSON from the SB interface.

If we need more than 1 jTrace command it could be "jTraceStart:XXXX" and "jTraceStop:XXXX"

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
187–188

lower case j

196–197

lower case j

199–200

lower case j

1111

lower case j

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
192

lower case j

196

lower case j

source/Utility/StringExtractorGDBRemote.cpp
282

lower case j

283

lower case j

285

lower case j

zturner added inline comments.May 11 2017, 10:19 AM
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1126

Should you be checking the return value of getAsInteger here? What if it didn't parse as an integer? Confusingly, the function returns true if it failed and false if it suceeded, so watch out.

1201

Can you invert the conditional here and use an early exit from the loop if there's an error?

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.

@labath I was considering writing Unit tests for the remote packets but I thought then I have to write the mock implementation for the trace operations as well, which might end up being a bigger piece of code than the actual packet parsing code.

I don't think that is the case. If you look at the tests in TestGdbRemoteCommunicationClient.cpp, all of them are quite simple. E.g. a test for TestGdbRemoteCommunicationClient could be something like:

TraceOptions opt;
opt.setType(...);
opt.setBufferSize(...);
opt.setMetaBufferSize(); // with an appropriate TraceOptions constructor, this could be a one-liner
  std::future<user_id_t> result = std::async(std::launch::async, [&] {
    return client.StartTrace(opt, error);
  });
HandlePacket(server, "JTrace:start:type:dead:buffersize:beef:metabuffersize:baad:threadid:f00d:jparams:...", "47");
ASSERT_EQ(47, result.get());
ASSERT_TRUE(error.Success());

This depends on the packet code being in GdbRemoteCommunicationClient and not ProcessGdbRemote as it is now, but moving it there is a good idea anyway -- ProcessGdbRemote should do higher level stuff, packet parsing should be left in the GDBRCClient.

The server side code unfortunately cannot be tested this way, but I believe I will get around to enabling that soon as well.

After this patch, I will upload the actual server side code doing the trace stuff for linux, that part of code has unit tests for some core functions.

The problem with that test vector is that the test will only run on linux, and only on processors that support the feature, which means there's a very high chance it will end up being dead code (my guesstimate is our bot does not have the required hardware feature). What is also quite difficult to test with that approach is the failure cases (what happens if the server sends a malformed packed or the response misses some data, etc.).

include/lldb/Utility/StringExtractor.h
114

The name looks wrong. Either use CamelCase or snake_case, don't try to mix the two. (I'd propose camel case).

TraceOptions opt;
opt.setType(...);
opt.setBufferSize(...);
opt.setMetaBufferSize(); // with an appropriate TraceOptions constructor, this could be a one-liner
  std::future<user_id_t> result = std::async(std::launch::async, [&] {
    return client.StartTrace(opt, error);
  });
HandlePacket(server, "JTrace:start:type:dead:buffersize:beef:metabuffersize:baad:threadid:f00d:jparams:...", "47");
ASSERT_EQ(47, result.get());
ASSERT_TRUE(error.Success());

This depends on the packet code being in GdbRemoteCommunicationClient and not ProcessGdbRemote as it is now, but moving it there is a good idea anyway -- ProcessGdbRemote should do higher level stuff, packet parsing should be left in the GDBRCClient.

The server side code unfortunately cannot be tested this way, but I believe I will get around to enabling that soon as well.

@labath Can I do the unit tests in a seperate patch after this patch ? It would be easier for me and perhaps also easier to review.

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?

ravitheja added a comment.EditedMay 18 2017, 5:01 AM

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.

Ok so I will upload what I have right now, and let the people review it in parallel, while I work on the unit tests and I will add them here in the next differential to this patch. I won't merge this patch until then.

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.

Ok so I will upload what I have right now, and let the people review it in parallel, while I work on the unit tests and I will add them here in the next differential to this patch. I won't merge this patch until then.

Uploading them to the phabricator is fine, of course. I would even say recommended. Feel free to do that.

ravitheja updated this revision to Diff 99716.May 22 2017, 12:21 AM

Adding unit tests and making the packets fully JSON.

Sorry I had to update, since the Error class has been changed to Status.

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:

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
3172

I don't think we need the "j" in the "jparams" now that the whole packet is json.

3288

I'm wondering whether we should be sending the trace type as an opaque number -- it's much easier to allocate IDs and manage compatibility if this is a string field. Greg, what do you think?

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1142

What if jparams isn't a dictionary?

1298

this is dead code. empty will never return true here. The process will just get killed if it fails to allocate.

However, you are right that we should check the allocation as we are allocating a buffer of user-provided size. I recommend explicitly allocating a uint8_t[] using nothrow new. It doesn't stop us from getting killed if the kernel overcommits, but it's better than nothing.

Alternatively you could also change the API to leave the memory allocation to the callee, which will be in a better position do determine whether the size is reasonable.

krytarowski added inline comments.
include/lldb/Host/common/NativeProcessProtocol.h
352

This is Linuxism. Not every OS can trace on per-thread basis.

ravitheja added inline comments.May 22 2017, 7:26 AM
include/lldb/Host/common/NativeProcessProtocol.h
352

Yes , the trace id (uid in the example u commented) could refer to a trace instance on a process or a thread. So the following two things can happen ->

  1. uid corresponds to a trace instance on a process. In this case thread only needs to specified if the user wants to stop tracing on an individual thread inside a process. If thread level tracing is not supported then thread arg need not be specified.
  1. uid corresponds to a trace instance on a thread. In which case the thread argument need not specified and the trace instance should be able to stop the trace.

So to summarize the thread argument only comes into picture when thread level tracing is supported and the user wants to stop tracing on an individual thread of a process being traced.

clayborg requested changes to this revision.May 22 2017, 12:07 PM

Close, some minor fixes.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
3293

Might be simpler to just do:

if (!custom_params_sp->GetAsDictionary())
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
188

Rename to match previous inline comments

191

Rename to match previous inline comments

194

Rename to match previous inline comments

197

Rename to match previous inline comments

200

Rename to match previous inline comments

1141

verify this is a dictionary first before the static cast below.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
192

"Handle_jTraceStart" to make it match the packet name of "jTraceStart"

194

"Handle_jTraceRead"

196

Handle_jTraceStop

198

Handle_ jTraceConfigRead

unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
400

Use the R"( so you don't have to desensitize the double quotes and so you can format nicely:

const char *json = R"(
jTraceStart: {
  "buffersize" : 8192,
  "metabuffersize" : 8192,
  "threadid" : 35,
  "type" : 1,
  "jparams" : {
    "psb" : 1,
    "tracetech" : "intel-pt"
  }
})";
HandlePacket(server, json, "1");
409

Ditto

428

Ditto, but inline the R"(...)"

435

Ditto, but inline the R"(...)"

457

Ditto, but inline the R"(...)"

493

Ditto, but inline the R"(...)"

527

Use the R"(

551

Ditto, but inline the R"(...)"

560

Use the R"(

569

Use the R"(

This revision now requires changes to proceed.May 22 2017, 12:07 PM
ravitheja added inline comments.May 23 2017, 1:56 AM
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
3293

Yes that could be dangerous to do, coz it returns a pointer to the Dictionary , while setTraceParams needs a shared_pointer and I would have a to create a shared_pointer which is not really being shared.

unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
400

That won't match coz the new lines and extra spaces cause mismatch in the packet string produced. Also unfortunately the Dump Function in the StrucuturedData::Dictionary adds spaces around the colons between "Key" : Value , so removing all whitespaces won't work. Now I can still add some functions to format the expexted string according to the one printed by Dump function, but that would be more code.

ravitheja updated this revision to Diff 99870.May 23 2017, 2:58 AM
ravitheja edited edge metadata.

Updates from last review.

labath added inline comments.May 23 2017, 3:13 AM
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
400

I've been wanting to add make these tests smarter now that they are getting more use. I'll take a todo item here to add a smarter way to compare json fields here. For now you can at least do the R" thing Greg suggests.

ravitheja added inline comments.May 23 2017, 3:17 AM
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
400

Yes I can do the R thing but then i can't format it nicely, as Greg suggested.

labath added inline comments.May 23 2017, 3:19 AM
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
400

Yes, but it will still avoid the need for escaping quotes, which is an improvement in itself.

ravitheja updated this revision to Diff 99891.May 23 2017, 5:51 AM

Modifying string literals in Unit tests.

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.

ravitheja updated this revision to Diff 100051.May 24 2017, 1:45 AM

Running clang-format.

labath accepted this revision.May 24 2017, 2:42 AM

Looks good as far as I am concerned (please wait for greg's ok though)

clayborg accepted this revision.May 24 2017, 9:01 AM

If all R"( strings are under 80 characters, this is good to go. If not, just fix and submit, no extra review needed.

This revision is now accepted and ready to land.May 24 2017, 9:01 AM
ravitheja closed this revision.May 26 2017, 4:46 AM
labath added inline comments.May 26 2017, 7:00 AM
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1304

Hey, ravi. You're leaking memory here. Please put this in a std::unique_ptr<uint8_t[]> so we make sure it get's cleaned up. (No need to request formal review)

clayborg added inline comments.May 26 2017, 1:45 PM
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
1304

use std::vector, I agree with Zach

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.