[PATCH] D18826: Add auto-exporting of symbols from tools so that plugins work on Windows

[Chris Bieneman via llvm-commits]

beanz added a reviewer: silvas.
beanz added a subscriber: silvas.
beanz added a comment.

I agree with @rnk's comments about philosophical design, but we shouldn't force this to wait on a giant header annotation pass.

The CMake looks good. I have a few comments about the Python. Also might be nice to get someone like @silvas to take a look at the Python too. He has provided excellent review feedback on Python code in the past.


================
Comment at: cmake/modules/AddLLVM.cmake:725
@@ +724,3 @@
+    add_custom_command(OUTPUT ${exported_symbol_file}
+                       COMMAND ${PYTHON_EXECUTABLE} ${LLVM_MAIN_SRC_DIR}/utils/extract_symbols.py --mangling=${mangling} ${static_libs} > ${exported_symbol_file}
+                       WORKING_DIRECTORY ${LLVM_LIBRARY_OUTPUT_INTDIR}
----------------
Nit: Since you have this in a python script it would be cleaner to have a -o option rather than a pipe here.

================
Comment at: utils/extract_symbols.py:29
@@ +28,3 @@
+def dumpbin_get_symbols(lib):
+    process = subprocess.Popen(['dumpbin','/symbols',lib], bufsize=1,
+                               stdout=subprocess.PIPE, stdin=subprocess.PIPE)
----------------
Rather than calling subprocess.Popen you might consider using subprocess.check_output. It would significantly reduce the amount of boiler plate code in this python script.

================
Comment at: utils/extract_symbols.py:334
@@ +333,3 @@
+              # Assume that if nm exists then so does objdump (should be safe as
+              # both are part of binutils)
+              'nm' : (nm_get_symbols, objdump_is_32bit_windows),
----------------
This is not a safe assumption. Apple ships nm but not objdump. In place of objdump we historically shipped otool. The most recent Xcode ships llvm-objdump, but you can't rely on that being present.


Repository:
  rL LLVM

http://reviews.llvm.org/D18826