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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a subscriber: ravitheja.
abhishek.aggarwal added inline comments.


================
Comment at: tools/intel-pt/CMakeLists.txt:53
+
+if (NOT LLDB_DISABLE_PYTHON)
+  target_link_libraries(lldbIntelPT PRIVATE
----------------
labath wrote:
> All of this needs to go away. I think you only needed it because you are plucking NativeProcessLinux internals, so fixing that should fix this too.
Fixing this.


================
Comment at: tools/intel-pt/Decoder.cpp:27
+                              std::string &result) {
+  uint32_t error_code = sberror.GetError();
+  switch (error_code) {
----------------
abhishek.aggarwal wrote:
> 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.
Currently, the codes are generated by lldb server and remote packets don't support sending error strings from lldb server to lldb client.
Now, I can think of 2 ways:

1.  modify remote packets to send error strings as well (in addition to error codes).
2.  or decode error codes and generate error strings at lldb client side

Which one do you prefer? @labath prefers 1st one (in discussions of https://reviews.llvm.org/D33674). If you also prefers 1st one then I (or @ravitheja) can work on modifying packets accordingly and submit a separate patch for that.

My idea is to keep error codes and its decoding logic here for now. Once we submit patch of modified packets and that patch gets approved, I can remove this decoding function from here all together. Please let me know what you prefer.


https://reviews.llvm.org/D33035