Page MenuHomePhabricator

Initial implementation of SB APIs for Tracing support.
ClosedPublic

Authored by ravitheja on Feb 6 2017, 2:26 AM.

Details

Summary

This patch introduces new SB APIs for tracing support
inside LLDB. The idea is to gather trace data from
LLDB and provide it through this APIs to external
tools integrating with LLDB. These tools will be
responsible for interpreting and presenting the
trace data to their users.

The patch implements the following new SB APIs ->
-> StartTrace - starts tracing with given parameters
-> StopTrace - stops tracing.
-> GetTraceData - read the trace data .
-> GetMetaData - read the meta data assosciated with the trace.
-> GetTraceConfig - read the trace configuration

Tracing is associated with a user_id that is returned
by the StartTrace API and this id needs to be used
for accessing the trace data and also Stopping
the trace. The user_id itself may map to tracing
the complete process or just an individual thread.
The APIs require an additional thread parameter
when the user of these APIs wishes to perform
thread specific manipulations on the tracing instances.
The patch also includes the corresponding
python wrappers for the C++ based APIs.

Event Timeline

ravitheja created this revision.Feb 6 2017, 2:26 AM

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

The complete patch is too big hence I have made the following three patches ->

  • The first patch adds support for generic SB APIs for trace.
  • Second one will introduce generic remote packets.
  • The last one adds Processor Trace support for Linux in lldb-server.
clayborg requested changes to this revision.Feb 13 2017, 9:50 AM

Overall comments:

  • Check the SB API guidelines and submit new patch (https://lldb.llvm.org/SB-api-coding-rules.html)
  • It might be nice to make a lldb_private::Trace and a lldb::SBTrace object and have that be the object we used instead of adding many calls to SBProcess. You get a SBTrace object out of a SBProcess with:
class SBProcess
{
  SBTrace StartTrace(SBTraceOptions &opts, SBError &error);
  SBTrace GetTrace(); // Get current trace object, might return invalid/empty SBTrace object
};

Then all calls to start, stop, get config, get data, get metadata, are on the SBTrace object.

See inlined comments for all the details.

include/lldb/API/SBProcess.h
240

remove "sb" from name

243–244

Custom parameters should be passed in via JSON. This allows much more structured data and allows more complex settings to be specified.

257

remove "sb" from name

268

Remove "sb" from the variable names.

268

Should we return a trace object instead of a tracing ID? Maybe return a SBTrace object?

269

Should the thread be in the SBTraceOptions? What if you want to trace N threads, but not all? Seems like this would be better represented in the SBTraceOptions.

278

remove "sb" from name

281–288

Should we make a SBTraceData object instead of getting a buffer of bytes? I haven't looked past this code to the code below, so I am not sure how this data will be used. Just thinking out loud here.

290–291

Should the trace data not be a stream? Can you elaborate on why you think you need the offset?

304

If we return an SBTrace object from SBProcess:: StartTrace() then this function would be moved to the SBTrace object

313

If we return an SBTrace object from SBProcess:: StartTrace() then this function would be moved to the SBTrace object

342

If we return an SBTrace object from SBProcess:: StartTrace() then this function would be moved to the SBTrace object

include/lldb/API/SBTraceOptions.h
22–26

Generally anything in the lldb::SB* API we don't inline anything. If you change your implementation, you can have clients break.

Please read the SB API guidelines: https://lldb.llvm.org/SB-api-coding-rules.html

There are many violations of this in this file that will need to get fixed.

61–65

After reading SB API guidelines, you will need to move all member variables into an Impl class above and replace with a std::unique_ptr or std::shared_ptr to your Impl class that will contain these items. Let me know if the SB API guidelines aren't clear on why this is needed. You might want to just make it a std::unique_ptr to your TraceOptions class below.

include/lldb/Host/common/NativeProcessProtocol.h
335 ↗(On Diff #87206)

Use a lldb_private::StructuredData::Dictionary here. This will allow the types to be passed in as JSON.

361 ↗(On Diff #87206)

Either rename the TraceOptions class to Trace, or keep the TraceOptions and make a new Trace class that takes a TraceOptions class during construction. I would vote to just have a Trace class. Seems weird to have a TraceOptions::StartTracing(...) function.

374 ↗(On Diff #87206)

Ditto above comment.

396 ↗(On Diff #87206)

Ditto

481 ↗(On Diff #87206)

Should this be:

std::shared_ptr<lldb_private::Trace m_trace_sp;

This gives ownership to this plug-in? Otherwise what happens when a process goes away? If we forget to clean it up we would leak it?

include/lldb/Target/Process.h
97

Change to be:

lldb_private::StructuredData::Dictionary m_trace_params;

This will allow trace params to be specified as JSON.

2783–2787

return a "std::shared_ptr<Trace>" or lldb::TraceSP which is a typedef to the previous shared pointer?

scripts/interface/SBProcess.i
411–425

It would be nice to have just:

class SBProcess
{
  // Start tracing and get the trace object back
  SBTrace StartTrace(SBTraceOptions &options, lldb::SBError &error);
  // Get any existing trace object in the process
  SBTrace SBProcess::GetTrace();
};

Then move have:

class SBTrace
{
  void GetConfig(..., lldb::tid_t thread_id);
  SBError StartTrace(lldb::tid_t thread_id);
  SBError StopTrace(lldb::tid_t thread_id);
  size_t GetData(SBError &error, void *buf, size_t size, size_t offset, lldb::tid_t thread_id);
  size_t GetMetaData(SBError &error, void *buf, size_t size, size_t offset, lldb::tid_t thread_id);
};
scripts/interface/SBTraceOptions.i
13

Adjust as needed from above comments.

This revision now requires changes to proceed.Feb 13 2017, 9:50 AM
ravitheja updated this revision to Diff 90461.Mar 3 2017, 4:57 AM
ravitheja edited edge metadata.

Changes according to the comments.

ravitheja added inline comments.Mar 3 2017, 5:24 AM
include/lldb/API/SBProcess.h
269

We wanted to keep some symmetry among the APIs , the threadid could go in the TraceOptions as well.

281–288

So the idea we have is that we don't want to interpret the trace data inside lldb. Now one could go and make a class SBTraceData but it would just be a buffer of trace data, which is why we left it as a buffer of bytes.

290–291

For processor Trace implementation buffer was more suitable for decoding purposes. The reason for offset was that it gives the plugin the chance to request partial segments of the trace data which it could decode.

Greg is taking a couple of weeks off between jobs, so he may not get to this right away. But it should wait on his approval since he had substantial comments.

I'm pretty sure Kate isn't doing any work on lldb these days, I wouldn't wait on her to review patches.

clayborg requested changes to this revision.Mar 6 2017, 2:17 PM

Much better. Found some extra stuff. In general we should be using SBStructuredData when ever we want to get/set stuff from structured data like JSON, XML or Apple plist, etc. If we make the APIs use SBStructuredData instead of using a SBStream, we can add constructors to SBStructuredData to init with JSON, XML, Apple plist, and more. All this will be parsed into a SBStructuredData or StructuredData::ObjectSP underneath, and then all people will use the StructuredData::ObjectSP on the inside.

include/lldb/API/SBProcess.h
267

Seems like the thread_id should go into the SBTraceOptions.

include/lldb/API/SBTrace.h
17

Remove SB from SBTraceImpl. Anything "SB" prefixed should be in the lldb namespace and should be exported, This shouldn't be exported.

98–100

Do the SBTraceOptions allow you to specify different things for different threads? If not, this parameter probably isn't needed, or could be extracted from the SBTraceOptions class itself. Seems like this isn't needed.

110

Use TraceImpl instead of SBTraceImpl

include/lldb/API/SBTraceOptions.h
32

Seems like this might be better as:

lldb::SBStructuredData getTraceParams(lldb::SBError &error);

The SBStructuredData can then emit to JSON or XML or any other format eventually.

39

This should probably be:

void setTraceParams(lldb::SBStructuredData &params);

Then we can add extra functions to SBStructuredData that allow you construct one from XML, JSON, Apple plist, or any other structured data.

53

We should move this typedef into lldb-forward.h

54

If we move this to lldb-forward.h, this will become "lldb::TraceOptionsSP"

include/lldb/Target/Process.h
80

We should start with SBStructuredData from the API, and then this function will just take a StructuredData::ObjectSP dict_obj

include/lldb/lldb-enumerations.h
721

Maybe a bit more documentation here for eTraceTypeProcessorTrace? Saying something like "this requests the raw trace buffer bytes from what ever CPU you are using, ...".

scripts/interface/SBTrace.i
13

Do we want something in here that explains what kind of trace buffer data you will be getting? I am thinking that even though we know we ask for "eTraceTypeProcessorTrace", this might not be enough for a plug-in to interpret these bytes. Do we need something like:

class SBTrace {
const char *GetTraceDataName();
};

And for intel it might return "intel processor trace version 2.0"? Or Maybe it would. be better as:

class SBTrace {
lldb::SBStructuredData GetTraceDataInfo();
};

This could return data that could be accessed as JSON. The version could be encoded in here. Maybe the exact CPU type, or CPU ID could also be encoded. I am guessing that the intel processor trace has already changed format from CPU to CPU and might also change in the future? This would help the plug-in that will interpret the trace byte to extract them correctly?

scripts/interface/SBTraceOptions.i
23

Use lldb::SBStructuredData instead of SBStream

source/API/SBTrace.cpp
31

We should use the new LLDB_LOG macros for all new logging.

34–35

We should use the new LLDB_LOG macros for all new logging.

42–44

ditto

56–57

We should use the new LLDB_LOG macros for all new logging.

65–67

ditto

77–78

We should use the new LLDB_LOG macros for all new logging.

124–127

Isn't one of these redundant?

source/API/SBTraceOptions.cpp
38

use SBStructuredData

55

use SBStructuredData

This revision now requires changes to proceed.Mar 6 2017, 2:17 PM
ravitheja added inline comments.Mar 7 2017, 6:17 AM
include/lldb/API/SBTraceOptions.h
39

Hi, this would also mean we make SBTraceOptions a friend class of SBStructuredData so that we can get access to the StructuredDataObject inside ?

clayborg added inline comments.Mar 7 2017, 8:49 AM
include/lldb/API/SBTraceOptions.h
39

Yeah you need a way for internal clients to get the StructuredData::ObjectSP from the SBStructuredData. You probably need to allow access to the StructuredDataImpl class that is currently defined in SBStructuredData.cpp. So a few things need to happen:

  • move StructuredDataImpl somewhere into its own internal header file so that other clients in LLDB can access it
  • make a protected member function in SBStructuredData that hands out a "StructuredDataImpl *" to allow clients to access it
  • anyone that needs to get to the StructuredData::ObjectSP will then call a new accessor that must be added on StructuredDataImpl to retrieve it
  • add new ways to load the SBStructuredData from JSON (something like SBStructuredData::SetFromJSON(...))
ravitheja added inline comments.Mar 10 2017, 2:27 AM
scripts/interface/SBTrace.i
13

Hello, Couldn't this be done in the GetTraceConfig (currently I am doing this) , as the TraceOptions already has a StructuredData for custom parameters and configurations, any trace technology could just convey any special or trace specific configurations in the custom parameters of TraceOptions ?

ravitheja updated this revision to Diff 93850.Apr 3 2017, 6:09 AM
ravitheja edited edge metadata.

Changes for review.

Very close. A few main things:

  • JSON should use dictionaries and key/value pairs for all of the settings. Not sure if it was a typo where JSON array was mentioned a bunch of times?
  • Custom settings should be in a dictionary within the JSON using the plug-in's name as the key This allows us to easily detect which settings are supposed to be custom. Any custom settings must be understood or we get an error back.
  • New header file for TraceOptions

Other comments are inlined. But this is very close. Sorry for the delay in reviewing. Feel free to ping earlier if this happens again (after a day of no review)

include/lldb/API/SBProcess.h
241

Change "TraceOptions" to "trace options" since we are not mentioning the class name here.

243

The settings should be a JSON dictionary. I am hoping the JSON array above is a typo? If not we should fix this.

251

Is GetTraceConfig an internal API? If so, lets not mention it here. You general comments should suffice.

252–253

If might be nice to say "any custom parameters should be passed in a dictionary within the main JSON dictionary using the plugin name as the key. If your tracing plugin is named "intel-trace", then your JSON could look like:

{ "buffer-size" : 123, "intel-trace : { "custom-param-1" : 123, "custom-param2" : 234 }}
include/lldb/API/SBStructuredData.h
41–45

It is dangerous to change the API like this since we have code the links against the old unique_ptr and might do the wrong thing if an older binary links against a newer LLDB. Do you really need this changed? All parameters should be "SBStructuredData &" so there should be no copying on the way in...

include/lldb/API/SBTraceOptions.h
20–22

I would make two copies: one default and the one that takes params. Default values can cause problems in the future if any new params are added before the existing ones. I am also fine just having the default version and requiring people to call the setters. I will leave that decision up to you

31

JSON dictionary. The dict can contain an array, but everything should be key/value pairs in JSON.

38

JSON dictionary.

include/lldb/Target/Process.h
61

We should make a TraceOptions.h header file for this class

62–65

avoid default params.

include/lldb/lldb-enumerations.h
733

Should we be this specific with this define? Many processors have processor trace. We could start a trace with SBTraceOptions and then get a SBTrace object back, then ask SBTrace::GetPluginName() to get back "intel-processor-trace". I would prefer to keep the enum generic since that allows to easier merging in the future. We will want to expose ARM trace and other trace types in the future and we don't want everyone to have to edit this file and add their enum here. So I would make the GetTraceConfig() or SBTrace::GetPluginName() return the name of the trace, which will allow you to abstractly return the data and abstractly request tracing.

ravitheja updated this revision to Diff 95930.Apr 20 2017, 4:22 AM

New diff with requested changes.

ravitheja marked 7 inline comments as done.Apr 20 2017, 4:24 AM
ravitheja added inline comments.
include/lldb/API/SBProcess.h
251

No, its not an internal API, so just to throw some light to it, Processor trace integration for Linux does not support all buffer sizes so we are rounding the buffer sizes to the nearest supported buffer size and through GetTraceConfig we plan to communicate the actual trace buffer size being.

ravitheja marked 2 inline comments as done.Apr 20 2017, 4:27 AM
clayborg accepted this revision.Apr 25 2017, 1:58 PM

Looks good, lets start with this and iterate.

This revision is now accepted and ready to land.Apr 25 2017, 1:58 PM
ravitheja updated this revision to Diff 96682.Apr 26 2017, 1:59 AM

Fixing few header file inclusions.

ravitheja closed this revision.Apr 26 2017, 2:01 AM