[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature
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.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
clayborg added a comment. 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.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
clayborg added a comment. Much better on the API with Swig. Just a few inlined questions around GetRawBytes. Comment at: tools/intel-features/intel-pt/PTDecoder.h:54-55 + + // Get raw bytes of the instruction + const uint8_t *GetRawBytes() const; + 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 ``` Comment at: tools/intel-features/intel-pt/PTDecoder.h:58 + // Get size of raw bytes of the instruction + uint8_t GetRawBytesSize() const; + This might not be needed if GetRawBytes is changed to: ``` size_t GetRawBytes(void *buf, size_t size) const; ``` 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.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
clayborg added a comment. In https://reviews.llvm.org/D33035#799404, @abhishek.aggarwal wrote: > 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. It really comes down to the issue of how stable you want this API to be. If this API is a "you are expected to rebuild any tools you make against this API every time we release a new version" then you can really do anything you want. If you want a stable API to build against, you could choose to follow the LLDB public API rules: no inheritance, fixed number of member variables that never change (which usually means a std::shared_ptr, std::unique_ptr or a T*), and no virtual functions. This effectively renders the C++ API into a C API, but it does force you to have your public API classes have an internal implementation (T) and then the class that you export for your API be the public facing API that everyone uses externally. The nice thing about this is that swig has a really easy time creating all of the script bindings for you if you do it this way. So it really depends on what your future use case of this API is. >> 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 believe it would fix your issue for you. You write the glue code that can translate any argument (std::vector) into something more python like like a list of PTInstruction python objects. We do this in a lot of places with LLDB. >> 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. Now I understand. I didn't realize this was a stand alone tool. The main thing to figure out is how stable you want this C++ API that you are exporting to be. If that isn't an issue, feel free to use anything you want in that API. If stability is an issue you want to solve and you want to vend a C++ API, you might think about adopting LLDB's public API rules to help ensure stability. 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.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
clayborg added a comment. 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. 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 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? 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
labath added a comment. In https://reviews.llvm.org/D33035#798925, @abhishek.aggarwal wrote: > In https://reviews.llvm.org/D33035#798885, @labath wrote: > > > 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. I think we can start with an email to lldb-dev and see how it goes. Nobody on the llvm side uses swig. I'm not sure what would be a better solution either... I don't like either of them :) 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.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
labath added a comment. 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. 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.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.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.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
labath 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(
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.
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.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.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.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.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
labath added a comment.
Hi Abhishek,
Thank you for incorporating the changes. I still see some room for improvement
in the cmake files. I realize that this is something most people are not
familiar with, and is not exactly glamorous work, but it's still part of our
codebase and we should make sure it follows best practices.
Comment at: tools/intel-features/CMakeLists.txt:16
+endif()
+
+if (NOT LLDB_DISABLE_PYTHON AND LLDB_BUILD_INTEL_PT)
Could we avoid building the shared library altogether if both features are OFF?
Comment at: tools/intel-features/cli-wrapper.cpp:27
+bool lldb::PluginInitialize(lldb::SBDebugger debugger) {
+ PTPluginInitialize(debugger);
+ MPXPluginInitialize(debugger);
You will need some ifdef magic to make sure these still compile when the
feature is off.
Comment at: tools/intel-features/intel-mpx/CMakeLists.txt:9
+
+set(MPX_DEPS ${MPX_DEPS} LLVMSupport PARENT_SCOPE)
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.
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
There appear to be no typedefs here.
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"
You are already defining this as a part of the swig invocation in cmake.
Comment at: tools/intel-features/scripts/python-typemaps.txt:1
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
abhishek.aggarwal wrote:
> 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.
Ok, let's leave this as-is then. We could try factoring out common part of
those typemaps, but I'm not sure that's such a good idea at the moment.
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.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.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
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 ) {
+ 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
[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature
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
labath added a comment. In https://reviews.llvm.org/D33035#767029, @abhishek.aggarwal wrote: > 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. Yes I remember that. But, as you say, that was 1.5 years ago, and we haven't heard anything since. Honestly, I had assumed you abandoned that work until you reappeared last month. :) So I think it's worth updating that thread, as new things may have come up since then (for example, the decision whether to merge the intel stuff into a single library). 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.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] D33035: Tool for using Intel(R) Processor Trace hardware feature
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] D33035: Tool for using Intel(R) Processor Trace hardware feature
labath added a comment. 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. 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? We probably can't, as this code depends on a third party library. In any case, this option should go to LLDBConfig.cmake Comment at: tools/intel-pt/CMakeLists.txt:42 + +add_library(lldbIntelPT SHARED + PTDecoder.cpp any reason you're not using add_lldb_library here? Comment at: tools/intel-pt/CMakeLists.txt:53 + +if (NOT LLDB_DISABLE_PYTHON) + target_link_libraries(lldbIntelPT PRIVATE 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. 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
krytarowski added a comment. In https://reviews.llvm.org/D33035#751280, @clayborg wrote: > In https://reviews.llvm.org/D33035#751264, @krytarowski wrote: > > > Can we please use the generic process plugin code, not the specific Linux > > one? If the generic code is insufficient - we should adjust it and alter > > instances of it (Linux and BSD). > > > There should be no need to ever #include something from a plug-in in code the > gets loaded externally. We should rely on the error strings themselves, not > be comparing them to some value. SBError has an error type (generic, posix, > mach-kernel, expression results, windows). It we are going to do anything we > can make up a new set of them, but the code in this patch is relying on the > values, when it should just be using the string value. If the string value > wasn't set correctly, fix that is the code that creates the errors. Thank you for explanation. This is even better and ideal. 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
clayborg added a comment. In https://reviews.llvm.org/D33035#751264, @krytarowski wrote: > Can we please use the generic process plugin code, not the specific Linux > one? If the generic code is insufficient - we should adjust it and alter > instances of it (Linux and BSD). There should be no need to ever #include something from a plug-in in code the gets loaded externally. We should rely on the error strings themselves, not be comparing them to some value. SBError has an error type (generic, posix, mach-kernel, expression results, windows). It we are going to do anything we can make up a new set of them, but the code in this patch is relying on the values, when it should just be using the string value. If the string value wasn't set correctly, fix that is the code that creates the errors. 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
krytarowski added a comment. Can we please use a generic process plugin code, not the specific Linux one? If the generic code is insufficient - we should adjust it and alter instances of it (Linux and BSD). 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
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
So we don't want clients of SBStructuredData to have to text scrape and write
manual decoders. See my inlined comments on the minimal modifications we need
to make to SBStructureData first. This then removes a ton of code from your
patch. The SBStructureData modifications should probably be done in separate
patch as well. internally lldb_private::StructuredData has everything (dicts,
array, strings, integers, bools, nulls) inherit from
lldb_private::StructuredData::Object. Then SBStructuredData owns a
StructuredDataImpl which has a lldb_private::StructuredData::Object shared
pointer, so a lldb::SBStructureData can be any object (dicts, array, strings,
integers, bools, nulls). So we should expose the ability to get data from
SBStructuredData. See my inlined SBStructureData additions I proposed. We
really don't want anyone doing manual JSON text scraping.
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)
Can we default this to enabled?
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"
This kind of reach in to internal plug-in sources shouldn't happen as it
violates the boundaries. Remove and see comment below.
Comment at: tools/intel-pt/Decoder.cpp:27
+ std::string ) {
+ uint32_t error_code = sberror.GetError();
+ switch (error_code) {
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.
Comment at: tools/intel-pt/Decoder.cpp:177
+ sberror.Clear();
+ CheckDebuggerID(sbprocess, sberror);
+ if (!sberror.Success())
Why do we care which debugger this belongs to?
Comment at: tools/intel-pt/Decoder.cpp:200
+
+ const char *custom_trace_params = s.GetData();
+ std::string trace_params(custom_trace_params);
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.
Comment at: tools/intel-pt/Decoder.cpp:211-212
+ }
+ std::size_t pos = trace_params.find(trace_tech_key);
+ if ((pos == std::string::npos)) {
+sberror.SetErrorStringWithFormat(
```
if (!sbstructdata.GetValueForKey("trace-tech").IsValid())
```
Comment at: tools/intel-pt/Decoder.cpp:218-219
+ }
+ pos = trace_params.find(trace_tech_value);
+ if ((pos == std::string::npos)) {
+sberror.SetErrorStringWithFormat(
```
if (!sbstructdata.GetValueForKey("intel-pt").IsValid())
```
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;
No text scraping. Please use SBStructureData methods that we will need to add.
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(",");
Remove this function, add real API to SBStructuredData
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()) {
Remove this function?
Comment at: tools/intel-pt/Decoder.h:86
