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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added inline comments.


================
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)
----------------
clayborg wrote:
> Can we default this to enabled?
The tool has a dependency on a decoder library. People who are not interested in building this tool will have to explicitly set this flag OFF to avoid build failures caused by absence of this decoder library on their system. I wanted to avoid that. But if you think enabling it by default is a better idea then I will change it.


================
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"
----------------
clayborg wrote:
> This kind of reach in to internal plug-in sources shouldn't happen as it violates the boundaries. Remove and see comment below.
I am removing this include from here.


================
Comment at: tools/intel-pt/Decoder.cpp:27
+                              std::string &result) {
+  uint32_t error_code = sberror.GetError();
+  switch (error_code) {
----------------
clayborg wrote:
> 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.
Removing this function.


================
Comment at: tools/intel-pt/Decoder.cpp:177
+  sberror.Clear();
+  CheckDebuggerID(sbprocess, sberror);
+  if (!sberror.Success())
----------------
clayborg wrote:
> Why do we care which debugger this belongs to?
Please see my reply to your comment in Decoder.h file.


================
Comment at: tools/intel-pt/Decoder.cpp:200
+
+  const char *custom_trace_params = s.GetData();
+  std::string trace_params(custom_trace_params);
----------------
clayborg wrote:
> 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.
I am on it.


================
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;
----------------
clayborg wrote:
> No text scraping. Please use SBStructureData methods that we will need to add.
Working on it.


================
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(",");
----------------
clayborg wrote:
> Remove this function, add real API to SBStructuredData
Working on it.


================
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()) {
----------------
clayborg wrote:
> Remove this function?
Please see reply to your comment in Decoder.h file.


================
Comment at: tools/intel-pt/Decoder.h:86
+//----------------------------------------------------------------------
+class Info : public lldb::SBTraceOptions {
+public:
----------------
clayborg wrote:
> 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?
1. I am changing its name to "TraceOptions".
2. Inheriting it from SBTraceOptions makes life easier in the sense of reusing the APIs without re-defining them here. This is a backend class. Exposing any new API in PTInfo class (in case SBTraceOptions class undergo any change/addition in future) will require change only in PTDecoder.h file and corresponding implementation file. Plus, concept wise both "SBTraceOptions" and "Info" represent trace options. These were the motives behind using inheritance. Am I missing some important thing here? Please let me know.


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


================
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()) {}
----------------
clayborg wrote:
> 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();
> ```
Please see my reply to your next comment below in this file.


================
Comment at: tools/intel-pt/Decoder.h:177
+  // which Decoder class instance was constructed.
+  void CheckDebuggerID(lldb::SBProcess &sbprocess, lldb::SBError &sberror);
+
----------------
clayborg wrote:
> 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?
As per my understanding, the Unique IDs of SBProcess instances will be unique within 1 SBDebugger instance but will not be unique across two different SBDebugger instances. If I am wrong then I don't need to store Debugger ID here. Otherwise, for each SBProcess instance provided by user through public APIs of the Tool, I will have to check which particular SBDebugger instance it belongs to. Please correct me if I am wrong.


https://reviews.llvm.org/D33035