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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Jun 26 06:58:37 PDT 2017

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
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
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. */
+%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.


More information about the lldb-commits mailing list