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

[Abhishek via Phabricator via lldb-commits]

abhishek.aggarwal added a comment.

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



================
Comment at: tools/intel-features/CMakeLists.txt:16
+endif()
+
+if (NOT LLDB_DISABLE_PYTHON AND LLDB_BUILD_INTEL_PT)
----------------
labath wrote:
> Could we avoid building the shared library altogether if both features are OFF?
yes. Adding the check.


================
Comment at: tools/intel-features/cli-wrapper.cpp:27
+bool lldb::PluginInitialize(lldb::SBDebugger debugger) {
+  PTPluginInitialize(debugger);
+  MPXPluginInitialize(debugger);
----------------
labath wrote:
> You will need some ifdef magic to make sure these still compile when the feature is off.
Fixed it.


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


================
Comment at: tools/intel-features/scripts/lldb-intel-features.swig:9
+
+/* Various liblldb typedefs that SWIG needs to know about.*/
+#define __extension__ /* Undefine GCC keyword to make Swig happy when
----------------
labath wrote:
> There appear to be no typedefs here.
Forgot to remove this. Doing it.


================
Comment at: tools/intel-features/scripts/lldb-intel-features.swig:14
+   as INT32_MAX should only be defined if __STDC_LIMIT_MACROS is. */
+#define __STDC_LIMIT_MACROS
+%include "python-typemaps.txt"
----------------
labath wrote:
> You are already defining this as a part of the swig invocation in cmake.
removing it.


https://reviews.llvm.org/D33035