Uploaded SVN Revision Number - 307773 to fix Android Builder.

Correcting mistakes.

Support for Hex encoded strings and more error checking.

Forgot this in the doc.

Updating Doc and changing feature to be enabled by client.

Yeah that would be broken, as long as we make the error packet more than 3 bytes, then it won't work with the older lldb clients.

With this patch, I see the extra packet to query from server is probably unnecessary. I mean the error code is still there and it should be sufficient for the client to detect an error. As long as it does not mistake it for something else it should not be a problem ? .

changes from feedback.

Removing Destroy() function.

In D33674#790653, @labath wrote:

In D33674#790595, @ravitheja wrote:

Yes you can start looking at it. The thing with munmap stuff is the following, you basically don't want to give the user an opportunity to have an uninitialized or instances which have been destroyed but not deleted.
So I removed the Destroy function from the public space. Now the user need not worry about Destroy function.

Ok, so that was one of my reasons for doing this. The other one is internal code cleanlyness -- it's much easier to verify that the code is healthy by just looking at it when the initialization and deinitialization is close together. unique_ptr allows you to do that. In this code

if ((result = mmap(..., size)) != MAP_FAILED)
  ptr_up = std::unique_ptr(result, mmap_deleter(size));

the init and deinit code is on adjecant line, and it's guaranteed than memory will be freed. Here:

first_function() {
ptr = mmap(..., size);
ref=ArrayRef(ptr, size-1);
...
}

...

second_function() {
  ...
  // Remember to increment size by one
  munmap(ref.data(), ref.size()+1);
  ... 
}

it's not so obvious because the code is far apart and you need to carry state around. To verify correctness I need to look at the two pieces of code, and then find all possible code paths between them.

Ok I will write it like that. Please tell me if u want more changes, I will do them all together for the next patch.

Regarding the testing strategy, we have tests at two levels, one at the SB API level and the other at the tool level.

Cool, are you going to put some of those tests in this patch?

The problem with uploading tests is that they need minimum Broadwell machine and a newer Linux kernel (> 4.2 something )

Yes you can start looking at it. The thing with munmap stuff is the following, you basically don't want to give the user an opportunity to have an uninitialized or instances which have been destroyed but not deleted.
So I removed the Destroy function from the public space. Now the user need not worry about Destroy function.

More changes from past feedback.

Changes for last feedback.

In D33674#783893, @labath wrote:

In D33674#783861, @ravitheja wrote:

Although a bit confusing, there is more flexibility for the user.I must also point out that the trace buffer available is not unlimited and there can be situations where a user might simultaneously want to trace newly spawned threads with a smaller buffer and trace an individual thread with perhaps a bigger buffer size. Tracing all threads is definitely important coz the user might not want to separately start tracing on each new thread. Now the current design might be a bit confusing but I am willing to document it well with examples and uses cases.

I agree that having the ability to trace new threads from the moment they spawn is important. The confusion arises from the fact that we are treating "trace all threads" and "trace new threads" as a single concept. I think the cleanest solution for me would be to split those up, so that we would have three operations: (1) trace a specific thread, (2) trace new threads, (3) trace all threads. Then (1) and (2) can be combined in arbitrary ways, and (3) is mutually exclusive with anything else. It could be I am over-designing this though. I'd like to hear the opinion of anyone else that is reading this.

Start tracing the whole process
Ok. Traceid = 1
Start tracing thread 47
Error. Thread 47 is already traced. (???)

Although a bit confusing, there is more flexibility for the user.I must also point out that the trace buffer available is not unlimited and there can be situations where a user might simultaneously want to trace newly spawned threads with a smaller buffer and trace an individual thread with perhaps a bigger buffer size. Tracing all threads is definitely important coz the user might not want to separately start tracing on each new thread. Now the current design might be a bit confusing but I am willing to document it well with examples and uses cases.

Just to make things clear, I will explain a use case

Changes suggested in previous round of feedback.

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.

@labath Thanx for fixing the 32 bit builds, I am sorry for the trouble caused.

Using uint64_t everywhere.

Running clang-format.

Modifying string literals in Unit tests.

Updates from last review.

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

Adding unit tests and making the packets fully JSON.

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.

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.

In 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 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.

Changes for the feedback recieved in first round of review.

Fixing few header file inclusions.

New diff with requested changes.

Changes for review.

Changes according to the comments.

Hello,

I am working to enable support for Processor Trace in LLDB. Last year I had received recommendations on the lldb mailing list on how this should/could be done. To summarize, the functionality is as follows ->

• The actual trace gathering and OS specific trace handling is implemented in the server.
• We have a generic trace abstraction at the SB API and remote packet level.
• The lldb-server will conform to the trace abstraction so that trace technology and OS specific details can be contained in the lldb-server.
• The actual trace interpretation will be left to tools existing outside lldb which will use the SB API's to obtain raw trace data and interpret it.
• The trace architecture provides the possibility of adding support for multiple trace types without lldb having to interpret it (this also prevents lldb code base to bloat up due to support of several trace technologies).

Removing other files.

Renaming testcase

Adding testcase

@jasonmolenda The approach suggested seems promising, I look forward to it, if u want I can test it at my end ? once I see it. But I must say this patch also fixes the row offset calculation, so the unwindplan specified in the eh_frame plan is specified for the complete PLT section and not w.r.t the function symbol, so the offset used for picking the row from the unwindplan should be calculated taking into consideration this point.

@labath In order to reproduce this situation without the help of standard library, I would have to write handwritten assembly and the CFI directives for that, is that fine ?

so regarding this particular situation I want to give little more insight ->
It starts out from here

0x40143a <+346>: movabsq $0x403e32, %rdi           ; imm = 0x403E32 
0x401444 <+356>: movb   $0x0, %al
0x401446 <+358>: callq  0x400d30                  ; symbol stub for: printf
0x40144b <+363>: movq   0x6071c0, %rdi
0x401453 <+371>: movl   %eax, -0xdc(%rbp)
0x401459 <+377>: callq  0x400ed0                  ; symbol stub for: fflush  (jump to next disassembly block 1)
0x40145e <+382>: movl   $0x40, %esi
0x401463 <+387>: leaq   -0xb0(%rbp), %rdi
0x40146a <+394>: movq   0x607158, %rdx
0x401472 <+402>: movl   %eax, -0xe0(%rbp)

Checks for nullptr