[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 Greg .. Can you please review the changes? Please let me know if something 
needs to be changed. Thanks :)


https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a comment.

In https://reviews.llvm.org/D33035#816337, @clayborg wrote:

> I am not sure how sensitive typemaps are to the names of the arguments? Maybe 
> we can just have you use different names and then the two typemaps won't 
> collide? You can also write some manual code to do the remapping without 
> typemaps.
>
> Regardless of whether you are going to change the API or not, you will need 
> to write something to make it work in python.
>
>   const uint8_t * ptdecoder_private:: Instruction::GetRawBytes() const;
>
>
> What does swig do right now when the above code is converted to python? It 
> needs to make a python string from this and the GetRawBytesSize()...


Hi Greg .. You are right. I checked the swig converted code for this API and it 
was not using GetRawBytesSize(). I have changed the C++ API signature as you 
had suggested and wrote a new typemap to handle it. It is mostly similar to the 
one defined in lldb typemaps but doesn't use internal lldb implementation of 
PythonBytes. Let me know if something is not right. I am submitting new patch 
set. Thanks a lot for your feedback :)

In https://reviews.llvm.org/D33035#816337, @clayborg wrote:

> I am not sure how sensitive typemaps are to the names of the arguments? Maybe 
> we can just have you use different names and then the two typemaps won't 
> collide? You can also write some manual code to do the remapping without 
> typemaps.
>
> Regardless of whether you are going to change the API or not, you will need 
> to write something to make it work in python.
>
>   const uint8_t * ptdecoder_private:: Instruction::GetRawBytes() const;
>
>
> What does swig do right now when the above code is converted to python? It 
> needs to make a python string from this and the GetRawBytesSize()...





https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a comment.

In https://reviews.llvm.org/D33035#803801, @clayborg wrote:

> Much better on the API with Swig. Just a few inlined questions around 
> GetRawBytes.


Hi Greg .. Thanks for feedback. My comments are inlined.




Comment at: tools/intel-features/intel-pt/PTDecoder.h:54-55
+
+  // Get raw bytes of the instruction
+  const uint8_t *GetRawBytes() const;
+

clayborg wrote:
> In the python version of this function, this should return a python string 
> that contains the bytes or return None of there is nothing. Is that how it 
> currently works? One alternate way of doing this is doing what we do with 
> lldb::SBProcess:
> ```
>   size_t ReadMemory(addr_t addr, void *buf, size_t size, lldb::SBError 
> );
> 
> ```
> 
> In python, we use a type map to detect the args "void *buf, size_t size", and 
> we turn this into:
> 
> ```
> def ReadMemory(self, addr, error):
> return # Python string with bytes from read memory
> ```
> 
> So one option would be to change GetRawBytes() into:
> 
> ```
> size_t GetRawBytes(void *buf, size_t size) const;
> ```
> 
> The return value is now many bytes were filled in. If "buf" is NULL or "size" 
> is zero, you can return the length that you would need as the return value.
> 
> And have the python version be:
> 
> ```
> def GetRawBytes(self):
> return # bytes in a python string or None
> ```
> 
Hi Greg.. As per my understanding, if we change ReadMemory() function as you 
proposed then both typemaps:
%typemap(in) (void *buf, size_t size) 
%typemap(argout) (void *buf, size_t size) 

will have to be defined. Am i right?

If it is so then I am afraid that I can't reuse these maps from lldb as the 2nd 
typemap refers to lldb_private::PythonBytes which is an internal library of 
lldb. Please let me know if I am wrong.


https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal updated this revision to Diff 105434.
abhishek.aggarwal added a comment.

Removed std::vector<> from public APIs


https://reviews.llvm.org/D33035

Files:
  tools/CMakeLists.txt
  tools/intel-features/CMakeLists.txt
  tools/intel-features/README.txt
  tools/intel-features/cli-wrapper.cpp
  tools/intel-features/intel-mpx/CMakeLists.txt
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.h
  tools/intel-features/intel-mpx/test/Makefile
  tools/intel-features/intel-mpx/test/README.txt
  tools/intel-features/intel-mpx/test/TestMPXTable.py
  tools/intel-features/intel-mpx/test/main.cpp
  tools/intel-features/intel-pt/CMakeLists.txt
  tools/intel-features/intel-pt/Decoder.cpp
  tools/intel-features/intel-pt/Decoder.h
  tools/intel-features/intel-pt/PTDecoder.cpp
  tools/intel-features/intel-pt/PTDecoder.h
  tools/intel-features/intel-pt/README_CLI.txt
  tools/intel-features/intel-pt/README_TOOL.txt
  tools/intel-features/intel-pt/interface/PTDecoder.i
  tools/intel-features/scripts/CMakeLists.txt
  tools/intel-features/scripts/lldb-intel-features.swig
  tools/intel-features/scripts/python-typemaps.txt
  tools/intel-mpx/CMakeLists.txt
  tools/intel-mpx/IntelMPXTablePlugin.cpp
  tools/intel-mpx/test/Makefile
  tools/intel-mpx/test/README.txt
  tools/intel-mpx/test/TestMPXTable.py
  tools/intel-mpx/test/main.cpp

Index: tools/intel-mpx/CMakeLists.txt
===
--- tools/intel-mpx/CMakeLists.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Linux")
-  return ()
-endif ()
-
-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
-add_library(lldb-intel-mpxtable SHARED
-  IntelMPXTablePlugin.cpp
-  )
-
-target_link_libraries(lldb-intel-mpxtable
-  PUBLIC liblldb LLVMSupport)
-
-install(TARGETS lldb-intel-mpxtable
-  LIBRARY DESTINATION bin)
Index: tools/intel-features/scripts/python-typemaps.txt
===
--- /dev/null
+++ tools/intel-features/scripts/python-typemaps.txt
@@ -0,0 +1,15 @@
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
+// typemap for a char buffer
+%typemap(in) (char *dst, size_t dst_len) {
+   if (!PyInt_Check($input)) {
+   PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+   return NULL;
+   }
+   $2 = PyInt_AsLong($input);
+   if ($2 <= 0) {
+   PyErr_SetString(PyExc_ValueError, "Positive integer expected");
+   return NULL;
+   }
+   $1 = (char *) malloc($2);
+}
Index: tools/intel-features/scripts/lldb-intel-features.swig
===
--- /dev/null
+++ tools/intel-features/scripts/lldb-intel-features.swig
@@ -0,0 +1,16 @@
+%module lldbIntelFeatures
+
+%{
+#include "lldb/lldb-public.h"
+#include "intel-pt/PTDecoder.h"
+using namespace ptdecoder;
+%}
+
+/* Undefine GCC keyword to make Swig happy when processing glibc's stdint.h */
+#define __extension__
+
+/* Combined python typemap for all features */
+%include "python-typemaps.txt"
+
+/* Feature specific python interface files*/
+%include "../intel-pt/interface/PTDecoder.i"
Index: tools/intel-features/scripts/CMakeLists.txt
===
--- /dev/null
+++ tools/intel-features/scripts/CMakeLists.txt
@@ -0,0 +1,37 @@
+file(GLOB_RECURSE SWIG_SOURCES *.swig)
+
+set(FLAGS
+  -c++
+  -shadow
+  -python
+  -D__STDC_LIMIT_MACROS
+  -D__STDC_CONSTANT_MACROS
+  )
+
+set(INCLUDES
+  -I${LLDB_SOURCE_DIR}/include
+  -I${LLDB_SOURCE_DIR}/tools/intel-features/intel-pt
+  )
+
+set(OUTPUT_PYTHON_WRAPPER
+  ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
+
+set(OUTPUT_PYTHON_SCRIPT_DIR
+  ${CMAKE_CURRENT_BINARY_DIR}
+  )
+
+find_package(SWIG REQUIRED)
+add_custom_command(
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py
+  DEPENDS ${SWIG_SOURCES}
+  COMMAND ${SWIG_EXECUTABLE} ${FLAGS} ${INCLUDES} -o ${OUTPUT_PYTHON_WRAPPER} -outdir ${OUTPUT_PYTHON_SCRIPT_DIR} ${SWIG_SOURCES}
+  COMMENT "Generating python wrapper for features library")
+
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp PROPERTIES GENERATED 1)
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py PROPERTIES GENERATED 1)
+
+add_custom_target(intel-features-swig_wrapper ALL
+  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
Index: tools/intel-features/intel-pt/interface/PTDecoder.i
===
--- /dev/null
+++ tools/intel-features/intel-pt/interface/PTDecoder.i
@@ -0,0 +1,10 @@
+%include "stdint.i"
+
+%include "lldb/lldb-defines.h"
+%include "lldb/lldb-enumerations.h"
+%include "lldb/lldb-forward.h"
+%include "lldb/lldb-types.h"
+
+%include "lldb/API/SBDefines.h"
+
+%include 

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a comment.

In https://reviews.llvm.org/D33035#799058, @clayborg wrote:

> So std::vector shouldn't be used in a public API. You should make a class 
> that wraps your objects. LLDB's public API has lldb::SBInstruction and 
> lldb::SBInstructionList as examples. std::vector on other systems compiles 
> differently for debug and release builds and things will crash.


Hi Greg .. Does it go for the tools built on top of LLDB (e.g. in my case where 
feature library is not a part of liblldb shared lib but built on top of it)? If 
yes, then I will proceed to remove std::vector from C++ public API of the tool 
and create a new class for it.

> If you need this via swig for internal kind of stuff, then use a typemap 
> where you map std::vector to a list() with T instances in it in python.

I want to provide a python interface for the tool's C++ public API as well so 
that the API can be used in python modules as well. Therefore, I think 
typemapping to list() will not solve the problem. Am I right?

> I am a bit confused here as well. Are we exporting a plug-in specific python 
> bindings for the Intel PT data? It seems like it would be nice to wrap this 
> API into the SBTrace or other lldb interface? Am I not understanding this 
> correctly?

Probably, I didn't understand this comment of yours. We are not exporting 
python binding for Intel PT data. It is a python binding for Tool's C++ API and 
this Tool is built on top of LLDB. Did I answer your question? Please let me 
know if I misunderstood your comment.


https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a comment.

In https://reviews.llvm.org/D33035#798885, @labath wrote:

> In https://reviews.llvm.org/D33035#798857, @abhishek.aggarwal wrote:
>
> > Hi Pavel .. I could remove all exception handling code from source files. 
> > However, I am still wondering how to disable exception handling while 
> > providing python functions for C++ APIs of the features. Can you suggest 
> > something here? The whole problem is occurring because of the use of 
> > vector<> as an argument to one of the C++ APIs. To provide a python 
> > interface for these APIs, I am forced to include std_vector.i (please see 
> > tools/intel-features/intel-pt/interface/PTDecoder.i file) which is 
> > introducing exception handling code.
>
>
> Hm... that's a bit of a drag. I guess the SB API never ran into this problem 
> because it always provides it's own vector-like classes (SBModuleList, 
> SBFileSpecList, etc.). I guess the most canonical way would be to follow that 
> example and have your own class for a list of instructions. However, that is 
> a bit annoying, so I do see a case for making code generated by swig as an 
> exception to the rule.


Hmm .. Is the decision about enabling exception handling only for swig 
generated code lie in LLDB's community's hands or we will have to ask LLVM 
community for this too? I can add another class to resolve this problem but I 
am not sure whether it is the right way to go.


https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 .. I could remove all exception handling code from source files. 
However, I am still wondering how to disable exception handling while providing 
python functions for C++ APIs of the features. Can you suggest something here? 
The whole problem is occurring because of the use of vector<> as an argument to 
one of the C++ APIs. To provide a python interface for these APIs, I am forced 
to include std_vector.i (please see 
tools/intel-features/intel-pt/interface/PTDecoder.i file) which is introducing 
exception handling code.


https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal updated this revision to Diff 105163.
abhishek.aggarwal added a comment.

Removed exception handling code


https://reviews.llvm.org/D33035

Files:
  tools/CMakeLists.txt
  tools/intel-features/CMakeLists.txt
  tools/intel-features/README.txt
  tools/intel-features/cli-wrapper.cpp
  tools/intel-features/intel-mpx/CMakeLists.txt
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.h
  tools/intel-features/intel-mpx/test/Makefile
  tools/intel-features/intel-mpx/test/README.txt
  tools/intel-features/intel-mpx/test/TestMPXTable.py
  tools/intel-features/intel-mpx/test/main.cpp
  tools/intel-features/intel-pt/CMakeLists.txt
  tools/intel-features/intel-pt/Decoder.cpp
  tools/intel-features/intel-pt/Decoder.h
  tools/intel-features/intel-pt/PTDecoder.cpp
  tools/intel-features/intel-pt/PTDecoder.h
  tools/intel-features/intel-pt/README_CLI.txt
  tools/intel-features/intel-pt/README_TOOL.txt
  tools/intel-features/intel-pt/interface/PTDecoder.i
  tools/intel-features/scripts/CMakeLists.txt
  tools/intel-features/scripts/lldb-intel-features.swig
  tools/intel-features/scripts/python-typemaps.txt
  tools/intel-mpx/CMakeLists.txt
  tools/intel-mpx/IntelMPXTablePlugin.cpp
  tools/intel-mpx/test/Makefile
  tools/intel-mpx/test/README.txt
  tools/intel-mpx/test/TestMPXTable.py
  tools/intel-mpx/test/main.cpp

Index: tools/intel-mpx/CMakeLists.txt
===
--- tools/intel-mpx/CMakeLists.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Linux")
-  return ()
-endif ()
-
-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
-add_library(lldb-intel-mpxtable SHARED
-  IntelMPXTablePlugin.cpp
-  )
-
-target_link_libraries(lldb-intel-mpxtable
-  PUBLIC liblldb LLVMSupport)
-
-install(TARGETS lldb-intel-mpxtable
-  LIBRARY DESTINATION bin)
Index: tools/intel-features/scripts/python-typemaps.txt
===
--- /dev/null
+++ tools/intel-features/scripts/python-typemaps.txt
@@ -0,0 +1,15 @@
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
+// typemap for a char buffer
+%typemap(in) (char *dst, size_t dst_len) {
+   if (!PyInt_Check($input)) {
+   PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+   return NULL;
+   }
+   $2 = PyInt_AsLong($input);
+   if ($2 <= 0) {
+   PyErr_SetString(PyExc_ValueError, "Positive integer expected");
+   return NULL;
+   }
+   $1 = (char *) malloc($2);
+}
Index: tools/intel-features/scripts/lldb-intel-features.swig
===
--- /dev/null
+++ tools/intel-features/scripts/lldb-intel-features.swig
@@ -0,0 +1,16 @@
+%module lldbIntelFeatures
+
+%{
+#include "lldb/lldb-public.h"
+#include "intel-pt/PTDecoder.h"
+using namespace ptdecoder;
+%}
+
+/* Undefine GCC keyword to make Swig happy when processing glibc's stdint.h */
+#define __extension__
+
+/* Combined python typemap for all features */
+%include "python-typemaps.txt"
+
+/* Feature specific python interface files*/
+%include "../intel-pt/interface/PTDecoder.i"
Index: tools/intel-features/scripts/CMakeLists.txt
===
--- /dev/null
+++ tools/intel-features/scripts/CMakeLists.txt
@@ -0,0 +1,37 @@
+file(GLOB_RECURSE SWIG_SOURCES *.swig)
+
+set(FLAGS
+  -c++
+  -shadow
+  -python
+  -D__STDC_LIMIT_MACROS
+  -D__STDC_CONSTANT_MACROS
+  )
+
+set(INCLUDES
+  -I${LLDB_SOURCE_DIR}/include
+  -I${LLDB_SOURCE_DIR}/tools/intel-features/intel-pt
+  )
+
+set(OUTPUT_PYTHON_WRAPPER
+  ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
+
+set(OUTPUT_PYTHON_SCRIPT_DIR
+  ${CMAKE_CURRENT_BINARY_DIR}
+  )
+
+find_package(SWIG REQUIRED)
+add_custom_command(
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py
+  DEPENDS ${SWIG_SOURCES}
+  COMMAND ${SWIG_EXECUTABLE} ${FLAGS} ${INCLUDES} -o ${OUTPUT_PYTHON_WRAPPER} -outdir ${OUTPUT_PYTHON_SCRIPT_DIR} ${SWIG_SOURCES}
+  COMMENT "Generating python wrapper for features library")
+
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp PROPERTIES GENERATED 1)
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py PROPERTIES GENERATED 1)
+
+add_custom_target(intel-features-swig_wrapper ALL
+  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
Index: tools/intel-features/intel-pt/interface/PTDecoder.i
===
--- /dev/null
+++ tools/intel-features/intel-pt/interface/PTDecoder.i
@@ -0,0 +1,12 @@
+%include "stdint.i"
+%include "std_vector.i"
+
+%include "lldb/lldb-defines.h"
+%include "lldb/lldb-enumerations.h"
+%include "lldb/lldb-forward.h"
+%include "lldb/lldb-types.h"
+
+%include 

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added inline comments.



Comment at: tools/intel-features/intel-pt/Decoder.cpp:411
+std::string image_path(image_complete_path, path_length);
+try {
+  readExecuteSectionInfos.emplace_back(

labath wrote:
> abhishek.aggarwal wrote:
> > labath wrote:
> > > We can't have exceptions in llvm code. You will have to achieve this 
> > > differently. Your trick with manually adding -fexceptions will not work 
> > > anyway if the rest of the code is compiled without exceptions. Although 
> > > I'm not really sure why you need to protect this vector append in 
> > > particular, as we don't do this for any other vector elsewhere.
> > I kept the exception handling around stl containers only to catch bad_alloc 
> > exception because if this exception occurs, I didn't want the code to just 
> > exit but provide user with whatever amount of instruction log is available 
> > in the vector. That much amount of instruction log might still be helpful 
> > to the user. What is your opinion on that?
> > 
> > Plus, If rest of the code is not being compiled with -fexceptions but just 
> > this file, will it not solve the purpose? Let me know what you think about 
> > it. I can make changes accordingly then.
> I don't think there's any negotiating on this point (not with me anyway, 
> you'd need to take this much higher up). But here's what I think anyway:
> - the exception catch will be generally useless as most (all?) current OSs 
> will overcommit virtual memory (and then just kill you if they really run out 
> of memory and swap space)
> - even if you disable overcommit, chances are you will hit an OOM in one of 
> the zillion other places which allocate memory (all of which are unchecked) 
> instead of here. So this single catch will not make a difference.
Got it. Then I remove exception handling code from here. Submit the patch 
again. Thanks for elaborating.


https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a comment.

Thanks for your review Pavel. My comments are inlined. Let me know your opinion 
:)




Comment at: tools/intel-features/intel-pt/Decoder.cpp:411
+std::string image_path(image_complete_path, path_length);
+try {
+  readExecuteSectionInfos.emplace_back(

labath wrote:
> We can't have exceptions in llvm code. You will have to achieve this 
> differently. Your trick with manually adding -fexceptions will not work 
> anyway if the rest of the code is compiled without exceptions. Although I'm 
> not really sure why you need to protect this vector append in particular, as 
> we don't do this for any other vector elsewhere.
I kept the exception handling around stl containers only to catch bad_alloc 
exception because if this exception occurs, I didn't want the code to just exit 
but provide user with whatever amount of instruction log is available in the 
vector. That much amount of instruction log might still be helpful to the user. 
What is your opinion on that?

Plus, If rest of the code is not being compiled with -fexceptions but just this 
file, will it not solve the purpose? Let me know what you think about it. I can 
make changes accordingly then.


https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal updated this revision to Diff 104430.
abhishek.aggarwal added a comment.

Fixed one minor thing in CMakeFile


https://reviews.llvm.org/D33035

Files:
  tools/CMakeLists.txt
  tools/intel-features/CMakeLists.txt
  tools/intel-features/README.txt
  tools/intel-features/cli-wrapper.cpp
  tools/intel-features/intel-mpx/CMakeLists.txt
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.h
  tools/intel-features/intel-mpx/test/Makefile
  tools/intel-features/intel-mpx/test/README.txt
  tools/intel-features/intel-mpx/test/TestMPXTable.py
  tools/intel-features/intel-mpx/test/main.cpp
  tools/intel-features/intel-pt/CMakeLists.txt
  tools/intel-features/intel-pt/Decoder.cpp
  tools/intel-features/intel-pt/Decoder.h
  tools/intel-features/intel-pt/PTDecoder.cpp
  tools/intel-features/intel-pt/PTDecoder.h
  tools/intel-features/intel-pt/README_CLI.txt
  tools/intel-features/intel-pt/README_TOOL.txt
  tools/intel-features/intel-pt/interface/PTDecoder.i
  tools/intel-features/scripts/CMakeLists.txt
  tools/intel-features/scripts/lldb-intel-features.swig
  tools/intel-features/scripts/python-typemaps.txt
  tools/intel-mpx/CMakeLists.txt
  tools/intel-mpx/IntelMPXTablePlugin.cpp
  tools/intel-mpx/test/Makefile
  tools/intel-mpx/test/README.txt
  tools/intel-mpx/test/TestMPXTable.py
  tools/intel-mpx/test/main.cpp

Index: tools/intel-mpx/CMakeLists.txt
===
--- tools/intel-mpx/CMakeLists.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Linux")
-  return ()
-endif ()
-
-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
-add_library(lldb-intel-mpxtable SHARED
-  IntelMPXTablePlugin.cpp
-  )
-
-target_link_libraries(lldb-intel-mpxtable
-  PUBLIC liblldb LLVMSupport)
-
-install(TARGETS lldb-intel-mpxtable
-  LIBRARY DESTINATION bin)
Index: tools/intel-features/scripts/python-typemaps.txt
===
--- /dev/null
+++ tools/intel-features/scripts/python-typemaps.txt
@@ -0,0 +1,15 @@
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
+// typemap for a char buffer
+%typemap(in) (char *dst, size_t dst_len) {
+   if (!PyInt_Check($input)) {
+   PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+   return NULL;
+   }
+   $2 = PyInt_AsLong($input);
+   if ($2 <= 0) {
+   PyErr_SetString(PyExc_ValueError, "Positive integer expected");
+   return NULL;
+   }
+   $1 = (char *) malloc($2);
+}
Index: tools/intel-features/scripts/lldb-intel-features.swig
===
--- /dev/null
+++ tools/intel-features/scripts/lldb-intel-features.swig
@@ -0,0 +1,16 @@
+%module lldbIntelFeatures
+
+%{
+#include "lldb/lldb-public.h"
+#include "intel-pt/PTDecoder.h"
+using namespace ptdecoder;
+%}
+
+/* Undefine GCC keyword to make Swig happy when processing glibc's stdint.h */
+#define __extension__
+
+/* Combined python typemap for all features */
+%include "python-typemaps.txt"
+
+/* Feature specific python interface files*/
+%include "../intel-pt/interface/PTDecoder.i"
Index: tools/intel-features/scripts/CMakeLists.txt
===
--- /dev/null
+++ tools/intel-features/scripts/CMakeLists.txt
@@ -0,0 +1,37 @@
+file(GLOB_RECURSE SWIG_SOURCES *.swig)
+
+set(FLAGS
+  -c++
+  -shadow
+  -python
+  -D__STDC_LIMIT_MACROS
+  -D__STDC_CONSTANT_MACROS
+  )
+
+set(INCLUDES
+  -I${LLDB_SOURCE_DIR}/include
+  -I${LLDB_SOURCE_DIR}/tools/intel-features/intel-pt
+  )
+
+set(OUTPUT_PYTHON_WRAPPER
+  ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
+
+set(OUTPUT_PYTHON_SCRIPT_DIR
+  ${CMAKE_CURRENT_BINARY_DIR}
+  )
+
+find_package(SWIG REQUIRED)
+add_custom_command(
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py
+  DEPENDS ${SWIG_SOURCES}
+  COMMAND ${SWIG_EXECUTABLE} ${FLAGS} ${INCLUDES} -o ${OUTPUT_PYTHON_WRAPPER} -outdir ${OUTPUT_PYTHON_SCRIPT_DIR} ${SWIG_SOURCES}
+  COMMENT "Generating python wrapper for features library")
+
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp PROPERTIES GENERATED 1)
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py PROPERTIES GENERATED 1)
+
+add_custom_target(intel-features-swig_wrapper ALL
+  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
Index: tools/intel-features/intel-pt/interface/PTDecoder.i
===
--- /dev/null
+++ tools/intel-features/intel-pt/interface/PTDecoder.i
@@ -0,0 +1,13 @@
+%include "stdint.i"
+%include "std_string.i"
+%include "std_vector.i"
+
+%include "lldb/lldb-defines.h"
+%include "lldb/lldb-enumerations.h"
+%include "lldb/lldb-forward.h"
+%include "lldb/lldb-types.h"
+

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal updated this revision to Diff 104376.
abhishek.aggarwal added a comment.

Cmake files related changes

- Using lldb's cmake functions insted of vanilla ones for cmake files


https://reviews.llvm.org/D33035

Files:
  tools/CMakeLists.txt
  tools/intel-features/CMakeLists.txt
  tools/intel-features/README.txt
  tools/intel-features/cli-wrapper.cpp
  tools/intel-features/intel-mpx/CMakeLists.txt
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.h
  tools/intel-features/intel-mpx/test/Makefile
  tools/intel-features/intel-mpx/test/README.txt
  tools/intel-features/intel-mpx/test/TestMPXTable.py
  tools/intel-features/intel-mpx/test/main.cpp
  tools/intel-features/intel-pt/CMakeLists.txt
  tools/intel-features/intel-pt/Decoder.cpp
  tools/intel-features/intel-pt/Decoder.h
  tools/intel-features/intel-pt/PTDecoder.cpp
  tools/intel-features/intel-pt/PTDecoder.h
  tools/intel-features/intel-pt/README_CLI.txt
  tools/intel-features/intel-pt/README_TOOL.txt
  tools/intel-features/intel-pt/interface/PTDecoder.i
  tools/intel-features/scripts/CMakeLists.txt
  tools/intel-features/scripts/lldb-intel-features.swig
  tools/intel-features/scripts/python-typemaps.txt
  tools/intel-mpx/CMakeLists.txt
  tools/intel-mpx/IntelMPXTablePlugin.cpp
  tools/intel-mpx/test/Makefile
  tools/intel-mpx/test/README.txt
  tools/intel-mpx/test/TestMPXTable.py
  tools/intel-mpx/test/main.cpp

Index: tools/intel-mpx/CMakeLists.txt
===
--- tools/intel-mpx/CMakeLists.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Linux")
-  return ()
-endif ()
-
-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
-add_library(lldb-intel-mpxtable SHARED
-  IntelMPXTablePlugin.cpp
-  )
-
-target_link_libraries(lldb-intel-mpxtable
-  PUBLIC liblldb LLVMSupport)
-
-install(TARGETS lldb-intel-mpxtable
-  LIBRARY DESTINATION bin)
Index: tools/intel-features/scripts/python-typemaps.txt
===
--- /dev/null
+++ tools/intel-features/scripts/python-typemaps.txt
@@ -0,0 +1,15 @@
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
+// typemap for a char buffer
+%typemap(in) (char *dst, size_t dst_len) {
+   if (!PyInt_Check($input)) {
+   PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+   return NULL;
+   }
+   $2 = PyInt_AsLong($input);
+   if ($2 <= 0) {
+   PyErr_SetString(PyExc_ValueError, "Positive integer expected");
+   return NULL;
+   }
+   $1 = (char *) malloc($2);
+}
Index: tools/intel-features/scripts/lldb-intel-features.swig
===
--- /dev/null
+++ tools/intel-features/scripts/lldb-intel-features.swig
@@ -0,0 +1,16 @@
+%module lldbIntelFeatures
+
+%{
+#include "lldb/lldb-public.h"
+#include "intel-pt/PTDecoder.h"
+using namespace ptdecoder;
+%}
+
+/* Undefine GCC keyword to make Swig happy when processing glibc's stdint.h */
+#define __extension__
+
+/* Combined python typemap for all features */
+%include "python-typemaps.txt"
+
+/* Feature specific python interface files*/
+%include "../intel-pt/interface/PTDecoder.i"
Index: tools/intel-features/scripts/CMakeLists.txt
===
--- /dev/null
+++ tools/intel-features/scripts/CMakeLists.txt
@@ -0,0 +1,37 @@
+file(GLOB_RECURSE SWIG_SOURCES *.swig)
+
+set(FLAGS
+  -c++
+  -shadow
+  -python
+  -D__STDC_LIMIT_MACROS
+  -D__STDC_CONSTANT_MACROS
+  )
+
+set(INCLUDES
+  -I${LLDB_SOURCE_DIR}/include
+  -I${LLDB_SOURCE_DIR}/tools/intel-features/intel-pt
+  )
+
+set(OUTPUT_PYTHON_WRAPPER
+  ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
+
+set(OUTPUT_PYTHON_SCRIPT_DIR
+  ${CMAKE_CURRENT_BINARY_DIR}
+  )
+
+find_package(SWIG REQUIRED)
+add_custom_command(
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py
+  DEPENDS ${SWIG_SOURCES}
+  COMMAND ${SWIG_EXECUTABLE} ${FLAGS} ${INCLUDES} -o ${OUTPUT_PYTHON_WRAPPER} -outdir ${OUTPUT_PYTHON_SCRIPT_DIR} ${SWIG_SOURCES}
+  COMMENT "Generating python wrapper for features library")
+
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp PROPERTIES GENERATED 1)
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py PROPERTIES GENERATED 1)
+
+add_custom_target(intel-features-swig_wrapper ALL
+  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
Index: tools/intel-features/intel-pt/interface/PTDecoder.i
===
--- /dev/null
+++ tools/intel-features/intel-pt/interface/PTDecoder.i
@@ -0,0 +1,13 @@
+%include "stdint.i"
+%include "std_string.i"
+%include "std_vector.i"
+
+%include "lldb/lldb-defines.h"
+%include "lldb/lldb-enumerations.h"

[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 .. I have made the changes you suggested. My apologies for 
misinterpreting your previous comments but during written communications, it is 
sometimes difficult to interpret everything correctly. I have tried following 
LLDB's coding conventions and guidelines. Please let me know if I still missed 
things that you would have liked to see in this patch. Thanks for your patience 
:)




Comment at: tools/intel-features/CMakeLists.txt:16
+endif()
+
+if (NOT LLDB_DISABLE_PYTHON AND LLDB_BUILD_INTEL_PT)

labath wrote:
> Could we avoid building the shared library altogether if both features are 
> OFF?
yes. Adding the check.



Comment at: tools/intel-features/cli-wrapper.cpp:27
+bool lldb::PluginInitialize(lldb::SBDebugger debugger) {
+  PTPluginInitialize(debugger);
+  MPXPluginInitialize(debugger);

labath wrote:
> You will need some ifdef magic to make sure these still compile when the 
> feature is off.
Fixed it.



Comment at: tools/intel-features/intel-mpx/CMakeLists.txt:9
+
+set(MPX_DEPS ${MPX_DEPS} LLVMSupport PARENT_SCOPE)

labath wrote:
> What you want here is to define an INTERFACE dependency on the MPX library 
> instead.
> vanilla cmake way would be `target_link_libraries(lldbIntelMPX INTERFACE 
> LLVMSupport)`. **However**, we should use the llvm function instead, as that 
> also handles other llvm-specific magic (for example, this code will break if 
> someone does a LLVM_LINK_LLVM_DYLIB build).
> 
> So, I am asking for the third time:
> Have you tried using add_lldb_library instead?
> 
> The correct invocation should be `add_lldb_library(foo.cpp LINK_LIBS 
> Support)` and the rest of this file can just go away.
I am extremely sorry Pavel but I understood it now what you were trying to say 
in previous comments. Sorry about misinterpreting your comments before. I have 
used add_lldb_library function now. Please see them in the next patch set.



Comment at: tools/intel-features/scripts/lldb-intel-features.swig:9
+
+/* Various liblldb typedefs that SWIG needs to know about.*/
+#define __extension__ /* Undefine GCC keyword to make Swig happy when

labath wrote:
> There appear to be no typedefs here.
Forgot to remove this. Doing it.



Comment at: tools/intel-features/scripts/lldb-intel-features.swig:14
+   as INT32_MAX should only be defined if __STDC_LIMIT_MACROS is. */
+#define __STDC_LIMIT_MACROS
+%include "python-typemaps.txt"

labath wrote:
> You are already defining this as a part of the swig invocation in cmake.
removing it.


https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal updated this revision to Diff 103924.
abhishek.aggarwal added a comment.

Changes after feedback

- Created static libs for each hardware feature to combine it to generate 
single shared library
- Removed error codes decoding logic and directly using error strings


https://reviews.llvm.org/D33035

Files:
  tools/CMakeLists.txt
  tools/intel-features/CMakeLists.txt
  tools/intel-features/README.txt
  tools/intel-features/cli-wrapper.cpp
  tools/intel-features/intel-mpx/CMakeLists.txt
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.cpp
  tools/intel-features/intel-mpx/cli-wrapper-mpxtable.h
  tools/intel-features/intel-mpx/test/Makefile
  tools/intel-features/intel-mpx/test/README.txt
  tools/intel-features/intel-mpx/test/TestMPXTable.py
  tools/intel-features/intel-mpx/test/main.cpp
  tools/intel-features/intel-pt/CMakeLists.txt
  tools/intel-features/intel-pt/Decoder.cpp
  tools/intel-features/intel-pt/Decoder.h
  tools/intel-features/intel-pt/PTDecoder.cpp
  tools/intel-features/intel-pt/PTDecoder.h
  tools/intel-features/intel-pt/README_CLI.txt
  tools/intel-features/intel-pt/README_TOOL.txt
  tools/intel-features/intel-pt/interface/PTDecoder.i
  tools/intel-features/scripts/CMakeLists.txt
  tools/intel-features/scripts/lldb-intel-features.swig
  tools/intel-features/scripts/python-typemaps.txt
  tools/intel-mpx/CMakeLists.txt
  tools/intel-mpx/IntelMPXTablePlugin.cpp
  tools/intel-mpx/test/Makefile
  tools/intel-mpx/test/README.txt
  tools/intel-mpx/test/TestMPXTable.py
  tools/intel-mpx/test/main.cpp

Index: tools/intel-mpx/CMakeLists.txt
===
--- tools/intel-mpx/CMakeLists.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-if (NOT CMAKE_SYSTEM_NAME MATCHES "Linux")
-  return ()
-endif ()
-
-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
-add_library(lldb-intel-mpxtable SHARED
-  IntelMPXTablePlugin.cpp
-  )
-
-target_link_libraries(lldb-intel-mpxtable
-  PUBLIC liblldb LLVMSupport)
-
-install(TARGETS lldb-intel-mpxtable
-  LIBRARY DESTINATION bin)
Index: tools/intel-features/scripts/python-typemaps.txt
===
--- /dev/null
+++ tools/intel-features/scripts/python-typemaps.txt
@@ -0,0 +1,15 @@
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
+// typemap for a char buffer
+%typemap(in) (char *dst, size_t dst_len) {
+   if (!PyInt_Check($input)) {
+   PyErr_SetString(PyExc_ValueError, "Expecting an integer");
+   return NULL;
+   }
+   $2 = PyInt_AsLong($input);
+   if ($2 <= 0) {
+   PyErr_SetString(PyExc_ValueError, "Positive integer expected");
+   return NULL;
+   }
+   $1 = (char *) malloc($2);
+}
Index: tools/intel-features/scripts/lldb-intel-features.swig
===
--- /dev/null
+++ tools/intel-features/scripts/lldb-intel-features.swig
@@ -0,0 +1,18 @@
+%module lldbIntelFeatures
+
+%{
+#include "lldb/lldb-public.h"
+#include "intel-pt/PTDecoder.h"
+using namespace ptdecoder;
+%}
+
+/* Various liblldb typedefs that SWIG needs to know about.*/
+#define __extension__ /* Undefine GCC keyword to make Swig happy when
+ processing glibc's stdint.h. */
+/* The ISO C99 standard specifies that in C++ implementations limit macros such
+   as INT32_MAX should only be defined if __STDC_LIMIT_MACROS is. */
+#define __STDC_LIMIT_MACROS
+%include "python-typemaps.txt"
+
+/* Feature specific python interface files*/
+%include "../intel-pt/interface/PTDecoder.i"
Index: tools/intel-features/scripts/CMakeLists.txt
===
--- /dev/null
+++ tools/intel-features/scripts/CMakeLists.txt
@@ -0,0 +1,37 @@
+file(GLOB_RECURSE SWIG_SOURCES *.swig)
+
+set(FLAGS
+  -c++
+  -shadow
+  -python
+  -D__STDC_LIMIT_MACROS
+  -D__STDC_CONSTANT_MACROS
+  )
+
+set(INCLUDES
+  -I${LLDB_SOURCE_DIR}/include
+  -I${LLDB_SOURCE_DIR}/tools/intel-features/intel-pt
+  )
+
+set(OUTPUT_PYTHON_WRAPPER
+  ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
+
+set(OUTPUT_PYTHON_SCRIPT_DIR
+  ${CMAKE_CURRENT_BINARY_DIR}
+  )
+
+find_package(SWIG REQUIRED)
+add_custom_command(
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py
+  DEPENDS ${SWIG_SOURCES}
+  COMMAND ${SWIG_EXECUTABLE} ${FLAGS} ${INCLUDES} -o ${OUTPUT_PYTHON_WRAPPER} -outdir ${OUTPUT_PYTHON_SCRIPT_DIR} ${SWIG_SOURCES}
+  COMMENT "Generating python wrapper for features library")
+
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp PROPERTIES GENERATED 1)
+set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/lldbIntelFeatures.py PROPERTIES GENERATED 1)
+
+add_custom_target(intel-features-swig_wrapper ALL
+  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/IntelFeaturesPythonWrap.cpp
+  )
Index: 

[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 ) {
+  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

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a subscriber: ravitheja.
abhishek.aggarwal added inline comments.



Comment at: tools/intel-pt/CMakeLists.txt:53
+
+if (NOT LLDB_DISABLE_PYTHON)
+  target_link_libraries(lldbIntelPT PRIVATE

labath wrote:
> All of this needs to go away. I think you only needed it because you are 
> plucking NativeProcessLinux internals, so fixing that should fix this too.
Fixing this.



Comment at: tools/intel-pt/Decoder.cpp:27
+  std::string ) {
+  uint32_t error_code = sberror.GetError();
+  switch (error_code) {

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.


https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a comment.

In https://reviews.llvm.org/D33035#754640, @labath wrote:

> I don't really like that we are adding a public shared library for every tiny 
> intel feature. Could we at least merge this "plugin" with the existing 
> "intel-mpx plugin" to create one "intel support" library?
>
> Also, adding an external dependency probably deserves a discussion on 
> lldb-dev.


Hi Paval ... Before starting the development of this whole feature, we had 
submitted the full proposal to lldb dev list 1.5 years ago. During the 
discussions, it was proposed to keep the external dependency outside LLDB (i.e. 
not to be bundled in liblldb shared library). The External dependency required 
for this tool is not and will never be a part of lldb repository. Users who are 
interested to use this tool, will download this external dependency separately.




Comment at: tools/intel-pt/CMakeLists.txt:42
+
+add_library(lldbIntelPT SHARED
+  PTDecoder.cpp

labath wrote:
> any reason you're not using add_lldb_library here?
No. I will try with that.


https://reviews.llvm.org/D33035



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D33434: Added new API to SBStructuredData class

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal updated this revision to Diff 100376.
abhishek.aggarwal added a comment.

Updating https://reviews.llvm.org/D33434: Added new API to SBStructuredData 
class

- Removed inferior from test case (not required)
- fixed enum scope


https://reviews.llvm.org/D33434

Files:
  include/lldb/API/SBStructuredData.h
  include/lldb/Core/StructuredData.h
  include/lldb/Core/StructuredDataImpl.h
  include/lldb/lldb-enumerations.h
  
packages/Python/lldbsuite/test/python_api/sbstructureddata/TestStructuredDataAPI.py
  scripts/interface/SBStructuredData.i
  source/API/SBStructuredData.cpp
  source/API/SBThread.cpp
  source/Core/FormatEntity.cpp
  source/Core/StructuredData.cpp
  source/Target/Thread.cpp
  unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -15,6 +15,7 @@
 #include "lldb/Host/File.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/lldb-enumerations.h"
 
 #include "PythonTestSuite.h"
 
@@ -355,9 +356,9 @@
   list.AppendItem(PythonString(string_value1));
 
   auto array_sp = list.CreateStructuredArray();
-  EXPECT_EQ(StructuredData::Type::eTypeInteger,
+  EXPECT_EQ(lldb::eStructuredDataTypeInteger,
 array_sp->GetItemAtIndex(0)->GetType());
-  EXPECT_EQ(StructuredData::Type::eTypeString,
+  EXPECT_EQ(lldb::eStructuredDataTypeString,
 array_sp->GetItemAtIndex(1)->GetType());
 
   auto int_sp = array_sp->GetItemAtIndex(0)->GetAsInteger();
@@ -424,9 +425,9 @@
 
   auto array_sp = tuple.CreateStructuredArray();
   EXPECT_EQ(tuple.GetSize(), array_sp->GetSize());
-  EXPECT_EQ(StructuredData::Type::eTypeInteger,
+  EXPECT_EQ(lldb::eStructuredDataTypeInteger,
 array_sp->GetItemAtIndex(0)->GetType());
-  EXPECT_EQ(StructuredData::Type::eTypeString,
+  EXPECT_EQ(lldb::eStructuredDataTypeString,
 array_sp->GetItemAtIndex(1)->GetType());
 }
 
Index: source/Target/Thread.cpp
===
--- source/Target/Thread.cpp
+++ source/Target/Thread.cpp
@@ -51,6 +51,7 @@
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StreamString.h"
+#include "lldb/lldb-enumerations.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -397,7 +398,7 @@
   bool plan_overrides_trace =
 have_valid_stop_info && have_valid_completed_plan
 && (m_stop_info_sp->GetStopReason() == eStopReasonTrace);
-
+
   if (have_valid_stop_info && !plan_overrides_trace) {
 return m_stop_info_sp;
   } else if (have_valid_completed_plan) {
@@ -541,7 +542,7 @@
 saved_state.orig_stop_id = process_sp->GetStopID();
   saved_state.current_inlined_depth = GetCurrentInlinedDepth();
   saved_state.m_completed_plan_stack = m_completed_plan_stack;
-	
+
   return true;
 }
 
@@ -1994,52 +1995,49 @@
 thread_info->GetObjectForDotSeparatedPath("trace_messages");
 
 bool printed_activity = false;
-if (activity &&
-activity->GetType() == StructuredData::Type::eTypeDictionary) {
+if (activity && activity->GetType() == eStructuredDataTypeDictionary) {
   StructuredData::Dictionary *activity_dict = activity->GetAsDictionary();
   StructuredData::ObjectSP id = activity_dict->GetValueForKey("id");
   StructuredData::ObjectSP name = activity_dict->GetValueForKey("name");
-  if (name && name->GetType() == StructuredData::Type::eTypeString && id &&
-  id->GetType() == StructuredData::Type::eTypeInteger) {
+  if (name && name->GetType() == eStructuredDataTypeString && id &&
+  id->GetType() == eStructuredDataTypeInteger) {
 strm.Format("  Activity '{0}', {1:x}\n",
 name->GetAsString()->GetValue(),
 id->GetAsInteger()->GetValue());
   }
   printed_activity = true;
 }
 bool printed_breadcrumb = false;
-if (breadcrumb &&
-breadcrumb->GetType() == StructuredData::Type::eTypeDictionary) {
+if (breadcrumb && breadcrumb->GetType() == eStructuredDataTypeDictionary) {
   if (printed_activity)
 strm.Printf("\n");
   StructuredData::Dictionary *breadcrumb_dict =
   breadcrumb->GetAsDictionary();
   StructuredData::ObjectSP breadcrumb_text =
   breadcrumb_dict->GetValueForKey("name");
   if (breadcrumb_text &&
-  breadcrumb_text->GetType() == StructuredData::Type::eTypeString) {
+  breadcrumb_text->GetType() == eStructuredDataTypeString) {
 strm.Format("  Current Breadcrumb: {0}\n",
 breadcrumb_text->GetAsString()->GetValue());
   }
   printed_breadcrumb = true;
 }
-if (messages && messages->GetType() == StructuredData::Type::eTypeArray) {
+if 

[Lldb-commits] [PATCH] D33434: Added new API to SBStructuredData class

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal marked 2 inline comments as done.
abhishek.aggarwal added a comment.

Thanks for your suggestions. I have made changes according to feedback and 
submitting it here.


https://reviews.llvm.org/D33434



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D33434: Added new API to SBStructuredData class

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a comment.

My comments are inlined. Please let me know if something still needs to be 
changed.




Comment at: include/lldb/API/SBStructuredData.h:60
+  //--
+  size_t GetSize() const;
+

clayborg wrote:
> What is the user going to really do when getting the size from a dictionary? 
> I would vote to just make this for arrays and provide a IsEmpty() method if 
> you want to check if the dictionary or array is empty? I am open for reasons 
> why we should be able to get the size of the dictionary if there is a need, I 
> am just thinking out loud here.
I thought of the use case that a user might expect lldb to fill specific number 
of entries in a dictionary. This might serve as a failure check for the user 
right in the begining of his code without accessing each (key:value) pair 
inside it to conclude that enough number of entries are not filled inside the 
dictionary. I am leaving it as it is for now. However, if you disagree then I 
will change it according to your proposal.


https://reviews.llvm.org/D33434



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D33434: Added new API to SBStructuredData class

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal updated this revision to Diff 100108.
abhishek.aggarwal marked 9 inline comments as done.
abhishek.aggarwal added a comment.

Updating https://reviews.llvm.org/D33434: Added new API to SBStructuredData 
class

- Made changes according to feedback


https://reviews.llvm.org/D33434

Files:
  include/lldb/API/SBStructuredData.h
  include/lldb/Core/StructuredData.h
  include/lldb/Core/StructuredDataImpl.h
  include/lldb/lldb-enumerations.h
  packages/Python/lldbsuite/test/python_api/sbstructureddata/Makefile
  
packages/Python/lldbsuite/test/python_api/sbstructureddata/TestStructuredDataAPI.py
  packages/Python/lldbsuite/test/python_api/sbstructureddata/main.c
  scripts/interface/SBStructuredData.i
  source/API/SBStructuredData.cpp
  source/API/SBThread.cpp
  source/Core/FormatEntity.cpp
  source/Core/StructuredData.cpp
  source/Target/Thread.cpp
  unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -15,6 +15,7 @@
 #include "lldb/Host/File.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/lldb-enumerations.h"
 
 #include "PythonTestSuite.h"
 
@@ -355,9 +356,9 @@
   list.AppendItem(PythonString(string_value1));
 
   auto array_sp = list.CreateStructuredArray();
-  EXPECT_EQ(StructuredData::Type::eTypeInteger,
+  EXPECT_EQ(lldb::StructuredDataType::eStructuredDataTypeInteger,
 array_sp->GetItemAtIndex(0)->GetType());
-  EXPECT_EQ(StructuredData::Type::eTypeString,
+  EXPECT_EQ(lldb::StructuredDataType::eStructuredDataTypeString,
 array_sp->GetItemAtIndex(1)->GetType());
 
   auto int_sp = array_sp->GetItemAtIndex(0)->GetAsInteger();
@@ -424,9 +425,9 @@
 
   auto array_sp = tuple.CreateStructuredArray();
   EXPECT_EQ(tuple.GetSize(), array_sp->GetSize());
-  EXPECT_EQ(StructuredData::Type::eTypeInteger,
+  EXPECT_EQ(lldb::StructuredDataType::eStructuredDataTypeInteger,
 array_sp->GetItemAtIndex(0)->GetType());
-  EXPECT_EQ(StructuredData::Type::eTypeString,
+  EXPECT_EQ(lldb::StructuredDataType::eStructuredDataTypeString,
 array_sp->GetItemAtIndex(1)->GetType());
 }
 
Index: source/Target/Thread.cpp
===
--- source/Target/Thread.cpp
+++ source/Target/Thread.cpp
@@ -51,6 +51,7 @@
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StreamString.h"
+#include "lldb/lldb-enumerations.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -397,7 +398,7 @@
   bool plan_overrides_trace =
 have_valid_stop_info && have_valid_completed_plan
 && (m_stop_info_sp->GetStopReason() == eStopReasonTrace);
-
+
   if (have_valid_stop_info && !plan_overrides_trace) {
 return m_stop_info_sp;
   } else if (have_valid_completed_plan) {
@@ -541,7 +542,7 @@
 saved_state.orig_stop_id = process_sp->GetStopID();
   saved_state.current_inlined_depth = GetCurrentInlinedDepth();
   saved_state.m_completed_plan_stack = m_completed_plan_stack;
-	
+
   return true;
 }
 
@@ -1994,36 +1995,40 @@
 thread_info->GetObjectForDotSeparatedPath("trace_messages");
 
 bool printed_activity = false;
-if (activity &&
-activity->GetType() == StructuredData::Type::eTypeDictionary) {
+if (activity && activity->GetType() ==
+StructuredDataType::eStructuredDataTypeDictionary) {
   StructuredData::Dictionary *activity_dict = activity->GetAsDictionary();
   StructuredData::ObjectSP id = activity_dict->GetValueForKey("id");
   StructuredData::ObjectSP name = activity_dict->GetValueForKey("name");
-  if (name && name->GetType() == StructuredData::Type::eTypeString && id &&
-  id->GetType() == StructuredData::Type::eTypeInteger) {
+  if (name &&
+  name->GetType() == StructuredDataType::eStructuredDataTypeString &&
+  id &&
+  id->GetType() == StructuredDataType::eStructuredDataTypeInteger) {
 strm.Format("  Activity '{0}', {1:x}\n",
 name->GetAsString()->GetValue(),
 id->GetAsInteger()->GetValue());
   }
   printed_activity = true;
 }
 bool printed_breadcrumb = false;
-if (breadcrumb &&
-breadcrumb->GetType() == StructuredData::Type::eTypeDictionary) {
+if (breadcrumb && breadcrumb->GetType() ==
+  StructuredDataType::eStructuredDataTypeDictionary) {
   if (printed_activity)
 strm.Printf("\n");
   StructuredData::Dictionary *breadcrumb_dict =
   breadcrumb->GetAsDictionary();
   StructuredData::ObjectSP breadcrumb_text =
   breadcrumb_dict->GetValueForKey("name");
   if (breadcrumb_text &&
-  

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added inline comments.



Comment at: tools/CMakeLists.txt:8
 add_subdirectory(lldb-mi)
+option(LLDB_BUILD_INTEL_PT "Enable Building of Intel(R) Processor Trace Tool" 
OFF)
+if (LLDB_BUILD_INTEL_PT)

clayborg wrote:
> Can we default this to enabled?
The tool has a dependency on a decoder library. People who are not interested 
in building this tool will have to explicitly set this flag OFF to avoid build 
failures caused by absence of this decoder library on their system. I wanted to 
avoid that. But if you think enabling it by default is a better idea then I 
will change it.



Comment at: tools/intel-pt/Decoder.cpp:18
+// Other libraries and framework includes
+#include "../../source/Plugins/Process/Linux/NativeProcessLinux.h"
+#include "lldb/API/SBModule.h"

clayborg wrote:
> This kind of reach in to internal plug-in sources shouldn't happen as it 
> violates the boundaries. Remove and see comment below.
I am removing this include from here.



Comment at: tools/intel-pt/Decoder.cpp:27
+  std::string ) {
+  uint32_t error_code = sberror.GetError();
+  switch (error_code) {

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.



Comment at: tools/intel-pt/Decoder.cpp:177
+  sberror.Clear();
+  CheckDebuggerID(sbprocess, sberror);
+  if (!sberror.Success())

clayborg wrote:
> Why do we care which debugger this belongs to?
Please see my reply to your comment in Decoder.h file.



Comment at: tools/intel-pt/Decoder.cpp:200
+
+  const char *custom_trace_params = s.GetData();
+  std::string trace_params(custom_trace_params);

clayborg wrote:
> We meed to add API to SBStructuredData to expose some of the stuff from 
> lldb_private::StructuredData so you don't end up text scraping here. Just do 
> what you need for now but I would suggest at the very least:
> 
> ```
> class SBStructuredData {
>   enum Type {
> eTypeArray,
> eTypeDictionary,
> eTypeString,
> eTypeInteger,
> eTypeBoolean
>   }
>   // Get the type of data in this structured data.
>   Type GetType();
>   // Get a dictionary for a key value given a key from the top level of this 
> structured data if type is eTypeDictionary
>   SBStructuredData GetValueForKey(const char *key);
>   // Get the value of this object as a string if type is eTypeString
>   bool GetStringValue(char *s, size_t max_len);
>   // Get an integer of this object as an integer if type is eTypeInteger
>   bool GetIntegerValue(uint64_t );
>   ...
> };
> ```
>  See cleanup code below if we do this.
I am on it.



Comment at: tools/intel-pt/Decoder.cpp:538-547
+  const char *custom_trace_params = s.GetData();
+  if (!custom_trace_params || (s.GetSize() == 0)) {
+sberror.SetErrorStringWithFormat("lldb couldn't provide cpuinfo");
+return;
+  }
+
+  long int family = 0, model = 0, stepping = 0, vendor = 0;

clayborg wrote:
> No text scraping. Please use SBStructureData methods that we will need to add.
Working on it.



Comment at: tools/intel-pt/Decoder.cpp:602-603
+
+void Decoder::Parse(const std::string _params, const std::string ,
+lldb::SBError , std::string ) {
+  std::string key_value_pair_separator(",");

clayborg wrote:
> Remove this function, add real API to SBStructuredData
Working on it.



Comment at: tools/intel-pt/Decoder.cpp:1046-1047
+// SBDebugger with which this tool instance is associated.
+void Decoder::CheckDebuggerID(lldb::SBProcess ,
+  lldb::SBError ) {
+  if (!sbprocess.IsValid()) {

clayborg wrote:
> Remove this function?
Please see reply to your comment in Decoder.h file.



Comment at: tools/intel-pt/Decoder.h:86
+//--
+class Info : public lldb::SBTraceOptions {
+public:

clayborg wrote:
> Should this be named better? IntelTraceOptions? Also does it need to inherit 
> from lldb::SBTraceOptions? Would it make more sense to have this be a "has a" 
> where SBTraceOptions is a member variable?
1. I am changing its name to "TraceOptions".
2. Inheriting it from SBTraceOptions makes life easier in the sense of reusing 
the APIs without re-defining them here. This is a backend class. Exposing any 
new API in PTInfo class (in case SBTraceOptions class undergo any 
change/addition in future) will require change only in PTDecoder.h file and 
corresponding implementation file. Plus, concept wise both "SBTraceOptions" and 

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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal abandoned this revision.
abhishek.aggarwal added a comment.

Abandoning this change due to formatting problem with commit message.


https://reviews.llvm.org/D33034



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits