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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: tools/intel-features/CMakeLists.txt:50
+install(TARGETS lldbIntelFeatures
+  LIBRARY DESTINATION bin)
----------------
"bin" sounds wrong here. Shouldn't this go ti lib${LLVM_LIBDIR_SUFFIX}?
To properly handle DLL targets (i don't know whether ipt support windows) you'd need something like (see function add_lldb_library):
```
        install(TARGETS ${name}
          COMPONENT ${name}
          RUNTIME DESTINATION bin
          LIBRARY DESTINATION lib${LLVM_LIBDIR_SUFFIX}
          ARCHIVE DESTINATION lib${LLVM_LIBDIR_SUFFIX})
```
Although it may be than you can just call that function yourself.


================
Comment at: tools/intel-features/intel-mpx/CMakeLists.txt:5
+
+set(SOURCES ${SOURCES} "intel-mpx/cli-wrapper-mpxtable.cpp" PARENT_SCOPE)
+
----------------
Normally we build a single .a file for each source folder, which are then linked into other targets as appropriate, and I don't see a reason to deviate from this here.

Same goes for the ipt subfolder.


================
Comment at: tools/intel-features/scripts/python-typemaps.txt:1
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
----------------
Could we just use standard lldb typemaps? I don't see anything ipt specific here...


================
Comment at: tools/intel-pt/Decoder.cpp:27
+                              std::string &result) {
+  uint32_t error_code = sberror.GetError();
+  switch (error_code) {
----------------
abhishek.aggarwal wrote:
> 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.
How about we just avoid interpreting the error codes for now and print a generic "error 47" message instead. This avoids the awkward state, where we have two enums that need to be kept in sync, which is a really bad idea.

I think having more generic error code will be very useful - there are a lot of packets that would benefit from more helpful error messages.


https://reviews.llvm.org/D33035