[Lldb-commits] [PATCH] D33034: Tool for using Intel(R) Processor Trace hardware feature
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
[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 &result) {
+ 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 &value);
> ...
> };
> ```
> 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 &trace_params, const std::string &key,
+lldb::SBError &sberror, std::string &result) {
+ 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 &sbprocess,
+ lldb::SBError &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 implementat
[Lldb-commits] [PATCH] D33434: Added new API to SBStructuredData class
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 &&
- breadcru
[Lldb-commits] [PATCH] D33434: Added new API to SBStructuredData class
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.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.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 (message
[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 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 &result) {
+ 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.aggarwal added a comment.
Hi Pavel .. My comments are inlined. Please let me know if you have more
concerns. I am submitting the patch with all the proposed changes.
Comment at: tools/intel-features/CMakeLists.txt:50
+install(TARGETS lldbIntelFeatures
+ LIBRARY DESTINATION bin)
labath wrote:
> "bin" sounds wrong here. Shouldn't this go ti lib${LLVM_LIBDIR_SUFFIX}?
> To properly handle DLL targets (i don't know whether ipt support windows)
> you'd need something like (see function add_lldb_library):
> ```
> install(TARGETS ${name}
> COMPONENT ${name}
> RUNTIME DESTINATION bin
> LIBRARY DESTINATION lib${LLVM_LIBDIR_SUFFIX}
> ARCHIVE DESTINATION lib${LLVM_LIBDIR_SUFFIX})
> ```
> Although it may be than you can just call that function yourself.
Fixing it.
Comment at: tools/intel-features/intel-mpx/CMakeLists.txt:5
+
+set(SOURCES ${SOURCES} "intel-mpx/cli-wrapper-mpxtable.cpp" PARENT_SCOPE)
+
labath wrote:
> Normally we build a single .a file for each source folder, which are then
> linked into other targets as appropriate, and I don't see a reason to deviate
> from this here.
>
> Same goes for the ipt subfolder.
Agreed. I am submitting the changes.
Comment at: tools/intel-features/scripts/python-typemaps.txt:1
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
labath wrote:
> Could we just use standard lldb typemaps? I don't see anything ipt specific
> here...
Unfortunately, we can't because that file uses lldb_private::PythonString,
lldb_private::PythonList etc and for that I will have to link to
liblldbPluginScripInterpreterPython.
Comment at: tools/intel-pt/Decoder.cpp:27
+ std::string &result) {
+ uint32_t error_code = sberror.GetError();
+ switch (error_code) {
labath wrote:
> abhishek.aggarwal wrote:
> > abhishek.aggarwal wrote:
> > > clayborg wrote:
> > > > We really shouldn't be interpreting integer error codes here. The
> > > > string in the "sberror" should be what you use. Modify the code that
> > > > creates these errors in the first place to also put a string values you
> > > > have here into the lldb_private::Error to begin with and remove this
> > > > function.
> > > Removing this function.
> > Currently, the codes are generated by lldb server and remote packets don't
> > support sending error strings from lldb server to lldb client.
> > Now, I can think of 2 ways:
> >
> > 1. modify remote packets to send error strings as well (in addition to
> > error codes).
> > 2. or decode error codes and generate error strings at lldb client side
> >
> > Which one do you prefer? @labath prefers 1st one (in discussions of
> > https://reviews.llvm.org/D33674). If you also prefers 1st one then I (or
> > @ravitheja) can work on modifying packets accordingly and submit a separate
> > patch for that.
> >
> > My idea is to keep error codes and its decoding logic here for now. Once we
> > submit patch of modified packets and that patch gets approved, I can remove
> > this decoding function from here all together. Please let me know what you
> > prefer.
> How about we just avoid interpreting the error codes for now and print a
> generic "error 47" message instead. This avoids the awkward state, where we
> have two enums that need to be kept in sync, which is a really bad idea.
>
> I think having more generic error code will be very useful - there are a lot
> of packets that would benefit from more helpful error messages.
I am removing this function as @ravitheja is working on enabling lldb remote
packets to send error strings directly. So, we don't even need "error 47"
message.
https://reviews.llvm.org/D33035
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[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: tools/intel-features/intel-pt/interface/PTD
[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
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"
+%in
[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"
+
+%in
[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 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.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/API/SBDefines.h"
[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 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.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.aggarwal added a comment. In https://reviews.llvm.org/D33035#799656, @clayborg wrote: > 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. Hi Greg .. Thanks a lot for your feedback and detailed explanation. I am choosing to follow what LLDB SB API are following. Removing std::vector<> from interface. Submitting a new patch. 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 "../P
[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 > &error); > > ``` > > 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.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.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#833876, @clayborg wrote: > I was out on vacation, sorry for the delay. Looks good. Thanks a lot Greg for your feedback. I am merging the patch now. https://reviews.llvm.org/D33035 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
