[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature

[Greg Clayton via Phabricator via lldb-commits]

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

So we don't want clients of SBStructuredData to have to text scrape and write manual decoders. See my inlined comments on the minimal modifications we need to make to SBStructureData first. This then removes a ton of code from your patch. The SBStructureData modifications should probably be done in separate patch as well. internally lldb_private::StructuredData has everything (dicts, array, strings, integers, bools, nulls) inherit from lldb_private::StructuredData::Object. Then SBStructuredData owns a StructuredDataImpl which has a lldb_private::StructuredData::Object shared pointer, so a lldb::SBStructureData can be any object (dicts, array, strings, integers, bools, nulls). So we should expose the ability to get data from SBStructuredData. See my inlined SBStructureData additions I proposed. We really don't want anyone doing manual JSON text scraping.



================
Comment at: tools/CMakeLists.txt:8
 add_subdirectory(lldb-mi)
+option(LLDB_BUILD_INTEL_PT "Enable Building of Intel(R) Processor Trace Tool" OFF)
+if (LLDB_BUILD_INTEL_PT)
----------------
Can we default this to enabled?


================
Comment at: tools/intel-pt/Decoder.cpp:18
+// Other libraries and framework includes
+#include "../../source/Plugins/Process/Linux/NativeProcessLinux.h"
+#include "lldb/API/SBModule.h"
----------------
This kind of reach in to internal plug-in sources shouldn't happen as it violates the boundaries. Remove and see comment below.


================
Comment at: tools/intel-pt/Decoder.cpp:27
+                              std::string &result) {
+  uint32_t error_code = sberror.GetError();
+  switch (error_code) {
----------------
We really shouldn't be interpreting integer error codes here. The string in the "sberror" should be what you use. Modify the code that creates these errors in the first place to also put a string values you have here into the lldb_private::Error to begin with and remove this function.


================
Comment at: tools/intel-pt/Decoder.cpp:177
+  sberror.Clear();
+  CheckDebuggerID(sbprocess, sberror);
+  if (!sberror.Success())
----------------
Why do we care which debugger this belongs to?


================
Comment at: tools/intel-pt/Decoder.cpp:200
+
+  const char *custom_trace_params = s.GetData();
+  std::string trace_params(custom_trace_params);
----------------
We meed to add API to SBStructuredData to expose some of the stuff from lldb_private::StructuredData so you don't end up text scraping here. Just do what you need for now but I would suggest at the very least:

```
class SBStructuredData {
  enum Type {
    eTypeArray,
    eTypeDictionary,
    eTypeString,
    eTypeInteger,
    eTypeBoolean
  }
  // Get the type of data in this structured data.
  Type GetType();
  // Get a dictionary for a key value given a key from the top level of this structured data if type is eTypeDictionary
  SBStructuredData GetValueForKey(const char *key);
  // Get the value of this object as a string if type is eTypeString
  bool GetStringValue(char *s, size_t max_len);
  // Get an integer of this object as an integer if type is eTypeInteger
  bool GetIntegerValue(uint64_t &value);
  ...
};
```
 See cleanup code below if we do this.


================
Comment at: tools/intel-pt/Decoder.cpp:211-212
+  }
+  std::size_t pos = trace_params.find(trace_tech_key);
+  if ((pos == std::string::npos)) {
+    sberror.SetErrorStringWithFormat(
----------------
```
if (!sbstructdata.GetValueForKey("trace-tech").IsValid())
```


================
Comment at: tools/intel-pt/Decoder.cpp:218-219
+  }
+  pos = trace_params.find(trace_tech_value);
+  if ((pos == std::string::npos)) {
+    sberror.SetErrorStringWithFormat(
----------------
```
if (!sbstructdata.GetValueForKey("intel-pt").IsValid())
```


================
Comment at: tools/intel-pt/Decoder.cpp:538-547
+  const char *custom_trace_params = s.GetData();
+  if (!custom_trace_params || (s.GetSize() == 0)) {
+    sberror.SetErrorStringWithFormat("lldb couldn't provide cpuinfo");
+    return;
+  }
+
+  long int family = 0, model = 0, stepping = 0, vendor = 0;
----------------
No text scraping. Please use SBStructureData methods that we will need to add.


================
Comment at: tools/intel-pt/Decoder.cpp:602-603
+
+void Decoder::Parse(const std::string &trace_params, const std::string &key,
+                    lldb::SBError &sberror, std::string &result) {
+  std::string key_value_pair_separator(",");
----------------
Remove this function, add real API to SBStructuredData


================
Comment at: tools/intel-pt/Decoder.cpp:1046-1047
+// SBDebugger with which this tool instance is associated.
+void Decoder::CheckDebuggerID(lldb::SBProcess &sbprocess,
+                              lldb::SBError &sberror) {
+  if (!sbprocess.IsValid()) {
----------------
Remove this function?


================
Comment at: tools/intel-pt/Decoder.h:86
+//----------------------------------------------------------------------
+class Info : public lldb::SBTraceOptions {
+public:
----------------
Should this be named better? IntelTraceOptions? Also does it need to inherit from lldb::SBTraceOptions? Would it make more sense to have this be a "has a" where SBTraceOptions is a member variable?


================
Comment at: tools/intel-pt/Decoder.h:90
+
+  virtual ~Info() {}
+
----------------
Does this need to be virtual? Remember that lldb::SBTraceOptions ins't virtual since it is in the public API.


================
Comment at: tools/intel-pt/Decoder.h:131-134
+  Decoder(lldb::SBDebugger &sbdebugger)
+      : m_mapProcessUID_mapThreadID_TraceInfo_mutex(),
+        m_mapProcessUID_mapThreadID_TraceInfo(),
+        m_debugger_user_id(sbdebugger.GetID()) {}
----------------
Do you actually need the debugger here? It might be nice to avoid this and just get the debugger from the process:

```
auto debugger = process.GetTarget().GetDebugger();
```


================
Comment at: tools/intel-pt/Decoder.h:177
+  // which Decoder class instance was constructed.
+  void CheckDebuggerID(lldb::SBProcess &sbprocess, lldb::SBError &sberror);
+
----------------
You probably don't need this function as you shouldn't have to store the SBDebugger anywhere in this class as you can always get it from the process:

```
auto debugger = process.GetTarget().GetDebugger();
```

Why do we care which debugger this belongs to?


https://reviews.llvm.org/D33035