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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits