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 ¶ms);
+
----------------
This should probably be:
```
void setTraceParams(lldb::SBStructuredData ¶ms);
```
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 ¶ms) {
+ 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 ¶ms) {
+ if (params.GetData() == NULL)
----------------
use SBStructuredData
https://reviews.llvm.org/D29581
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
