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