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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a comment.

Hi Pavel .. My comments are inlined. Please let me know if you have more concerns. I am submitting the patch with all the proposed changes.



================
Comment at: tools/intel-features/CMakeLists.txt:50
+install(TARGETS lldbIntelFeatures
+  LIBRARY DESTINATION bin)
----------------
labath wrote:
> "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.
Fixing it.


================
Comment at: tools/intel-features/intel-mpx/CMakeLists.txt:5
+
+set(SOURCES ${SOURCES} "intel-mpx/cli-wrapper-mpxtable.cpp" PARENT_SCOPE)
+
----------------
labath wrote:
> 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.
Agreed. I am submitting the changes.


================
Comment at: tools/intel-features/scripts/python-typemaps.txt:1
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
----------------
labath wrote:
> Could we just use standard lldb typemaps? I don't see anything ipt specific here...
Unfortunately, we can't because that file uses lldb_private::PythonString, lldb_private::PythonList etc and for that I will have to link to liblldbPluginScripInterpreterPython.


================
Comment at: tools/intel-pt/Decoder.cpp:27
+                              std::string &result) {
+  uint32_t error_code = sberror.GetError();
+  switch (error_code) {
----------------
labath wrote:
> 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.
I am removing this function as @ravitheja is working on enabling lldb remote packets to send error strings directly. So, we don't even need "error 47" message.


https://reviews.llvm.org/D33035