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
