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

[Pavel Labath via Phabricator via lldb-commits]

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