[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

Mon Mar 6 14:18:02 PST 2017

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

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.

================
Comment at: include/lldb/API/SBProcess.h:267
+  lldb::SBTrace StartTrace(SBTraceOptions &options, lldb::SBError &error,
+                           lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID);
+
----------------
Seems like the thread_id should go into the SBTraceOptions.

================
Comment at: include/lldb/API/SBTrace.h:16
+
+class SBTraceImpl;
+
----------------
Remove SB from SBTraceImpl. Anything "SB" prefixed should be in the lldb namespace and should be exported, This shouldn't be exported.

================
Comment at: include/lldb/API/SBTrace.h:97-99
+  /// @param[in] thread_id
+  ///     The thread_id could be optionally provided to obtain the
+  ///     configuration used by a particular thread.
----------------
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.

================
Comment at: include/lldb/API/SBTrace.h:109
+protected:
+  typedef std::shared_ptr<SBTraceImpl> SBTraceImplSP;
+
----------------
Use TraceImpl instead of SBTraceImpl

================
Comment at: include/lldb/API/SBTraceOptions.h:31
+  /// The returned parameters would be formatted as a JSON Array.
+  lldb::SBError getTraceParams(lldb::SBStream &ss);
+
----------------
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.

================
Comment at: include/lldb/API/SBTraceOptions.h:38
+  /// They should be formatted as a JSON Array.
+  void setTraceParams(lldb::SBStream &params);
+
----------------
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.

================
Comment at: include/lldb/API/SBTraceOptions.h:52
+
+  typedef std::shared_ptr<lldb_private::TraceOptions> TraceOptionsSP;
+  TraceOptionsSP m_traceoptions_sp;
----------------
We should move this typedef into lldb-forward.h

================
Comment at: include/lldb/API/SBTraceOptions.h:53
+  typedef std::shared_ptr<lldb_private::TraceOptions> TraceOptionsSP;
+  TraceOptionsSP m_traceoptions_sp;
+};
----------------
If we move this to lldb-forward.h, this will become "lldb::TraceOptionsSP"

================
Comment at: include/lldb/Target/Process.h:78
+
+  void setTraceParams(std::string &params) {
+    StructuredData::ObjectSP dict_obj = StructuredData::ParseJSON(params);
----------------
We should start with SBStructuredData from the API, and then this function will just take a StructuredData::ObjectSP dict_obj

================
Comment at: include/lldb/lldb-enumerations.h:721

+enum TraceType { eTraceTypeNone = 0, eTraceTypeProcessorTrace };
+
----------------
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, ...".

================
Comment at: scripts/interface/SBTrace.i:12
+
+class LLDB_API SBTrace {
+public:
----------------
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?

================
Comment at: scripts/interface/SBTraceOptions.i:22
+
+  lldb::SBError getTraceParams(lldb::SBStream &ss);
+
----------------
Use lldb::SBStructuredData instead of SBStream

================
Comment at: source/API/SBTrace.cpp:30
+  ProcessSP process_sp(GetSP());
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
+  error.Clear();
----------------
We should use the new LLDB_LOG macros for all new logging.

================
Comment at: source/API/SBTrace.cpp:33-34
+
+  if (log)
+    log->Printf("SBProcess:: %s", __FUNCTION__);
+
----------------
We should use the new LLDB_LOG macros for all new logging.

================
Comment at: source/API/SBTrace.cpp:41-43
+    if (log)
+      log->Printf("SBProcess:: %s bytes_read - %" PRIx64, __FUNCTION__,
+                  bytes_read);
----------------
ditto

================
Comment at: source/API/SBTrace.cpp:55-56
+
+  if (log)
+    log->Printf("SBProcess:: %s", __FUNCTION__);
+
----------------
We should use the new LLDB_LOG macros for all new logging.

================
Comment at: source/API/SBTrace.cpp:64-66
+    if (log)
+      log->Printf("SBProcess:: %s bytes_read - %" PRIx64, __FUNCTION__,
+                  bytes_read);
----------------
ditto

================
Comment at: source/API/SBTrace.cpp:76-77
+
+  if (log)
+    log->Printf("SBProcess:: %s", __FUNCTION__);
+
----------------
We should use the new LLDB_LOG macros for all new logging.

================
Comment at: source/API/SBTrace.cpp:123-126
+  if (!m_trace_impl_sp)
+    return false;
+  if (!GetSP())
+    return false;
----------------
Isn't one of these redundant?

================
Comment at: source/API/SBTraceOptions.cpp:37
+
+lldb::SBError SBTraceOptions::getTraceParams(lldb::SBStream &ss) {
+  const lldb_private::StructuredData::DictionarySP dict_obj =
----------------
use SBStructuredData

================
Comment at: source/API/SBTraceOptions.cpp:54
+
+void SBTraceOptions::setTraceParams(lldb::SBStream &params) {
+  if (params.GetData() == NULL)
----------------
use SBStructuredData

https://reviews.llvm.org/D29581