Python script to compile and link the integration tests using llvm's clang-cl

syzygy/integration_tests/make_integration_tests_clang.py

Index: syzygy/integration_tests/make_integration_tests_clang.py
diff --git a/syzygy/integration_tests/make_integration_tests_clang.py b/syzygy/integration_tests/make_integration_tests_clang.py
new file mode 100644
index 0000000000000000000000000000000000000000..f138fab8c1a51172d5097294927469cbf19e7232
--- /dev/null
+++ b/syzygy/integration_tests/make_integration_tests_clang.py
@@ -0,0 +1,39 @@
+#!/usr/bin/env python
+# Copyright 2017 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import optparse
+import os
+import re
+import subprocess
+import sys
+
+
+def main():
+  parser = optparse.OptionParser(usage='%prog [options]')
+  parser.add_option('--src-dir',

[Sébastien Marchand, 2017/06/21 15:32:49]

You don't need this, you can automatically find it (see
https://github.com/google/syzygy/blob/30b171f90991d6332499da77309c8c1a6d93198...)

[njanevsk, 2017/06/21 19:26:49]

That command gives the path to the script/file. I need the path to the
repository directory. GYP provides this. I don't think there will be an easier
way in Python to get the src directory.

[Sébastien Marchand, 2017/06/21 19:51:36]

Yes, look at the line following the one I linked to.
_SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
_SRC_DIR = os.path.abspath(os.path.join(_SCRIPT_DIR, os.pardir))

+    help='The src directory where the syzygy repository is located.')
+  parser.add_option('--release-dir', help='Syzygy build directory.')

[Sébastien Marchand, 2017/06/21 15:32:49]

We tend to call this --build-dir

[njanevsk, 2017/06/21 19:26:49]

Done.

+  parser.add_option('--input_files', help='files to be compiled and linked')

[Sébastien Marchand, 2017/06/21 15:32:49]

be consistent, you're using dashes for the other flags so don't use underscores
here, use a dash too.

[njanevsk, 2017/06/21 19:26:49]

Done.

+  
+  options, _ = parser.parse_args()
+  
+  if not options.src_dir:
+    parser.error('--src-dir is required')
+  if not options.release_dir:
+    parser.error('--release-dir is required')
+  if not options.input_files:
+    parser.error('--input_files is required')
+
+  clang_path = options.src_dir + "/third_party/llvm-build/Release+Asserts/bin/clang-cl.exe"

[Sébastien Marchand, 2017/06/21 15:32:49]

The 80 chars / line rule also apply to Python scrits :), you can use parenthesis
for this.

You could also move this path to a constant at the top of the file, e.g.:
_CLANG_CL_PATH = os.path.join(_SRC_DIR, 'third_party', 'llvm-build',
'Release+Asserts', 'bin', 'clang-cl.exe')

Not that in this case _SRC_DIR is also a constant, see my comment for the
--src-dir flag.

[njanevsk, 2017/06/21 19:26:49]

Done.

[Sébastien Marchand, 2017/06/21 19:51:36]

You haven't addressed the second part of this comment.

+  compiler_flags = '-c -fsanitize=address -mllvm -asan-instrumentation-with-call-threshold=0 -mllvm -asan-stack=0 -DUNICODE -D_UNICODE -DNOMINMAX -D_CRT_SECURE_NO_WARNINGS -I' + options.src_dir

[Sébastien Marchand, 2017/06/21 15:32:49]

Ooch, having this as a list will make it much easier to read, maintain, e.g.:

compiler_flags = [
  '-c',
  '-fsanitize=address',
  ...
  _SRC_DIR
]

Then to use this as a string you can just do ' '.join(compiler_flags)

[njanevsk, 2017/06/21 19:26:49]

Done.

+  
+  source_files = re.findall(r'\b(\w+.cc)\b', options.input_files)

[Sébastien Marchand, 2017/06/21 15:32:49]

I'd prefer to have something that works for all the source file extensions (cc,
cpp, Cpp...), could we just assume that all the source files are C++ source
files?

[njanevsk, 2017/06/21 19:26:49]

Yes we can but in that case I will need to have at least two list variables in
the gyp file. One for the source files, one for the header files, and one for
the remaining. Or just one for the source files, and one for the rest.

If you think is better approach I can do it that way.

[Sébastien Marchand, 2017/06/21 19:51:36]

I think that I'll keep this as 2 lists. A lot of the integration_test files
aren't needed for the Clang version of integration_tests.dll, e,g, the
"bb_entry*" one and the "coverage*" ones.

+  print subprocess.check_output('"' + clang_path + '" ' + compiler_flags + ' ' + ' '.join(source_files), shell = True)  

[Sébastien Marchand, 2017/06/21 15:32:49]

Same thing, use a list:

compile_command = [
  clang_path
]
compile_command.extend(compiler_flags)
compile_command.extend(source_files)

ret = subprocess.call(compile_command)
if ret != 0:
  # Log a meaningful error

[njanevsk, 2017/06/21 19:26:49]

Done.

+  object_files = [filename[:-3] + ".o" for filename in source_files]

[Sébastien Marchand, 2017/06/21 15:32:49]

Use os.path.splitext to get the name of the file.

[njanevsk, 2017/06/21 19:26:49]

Done.

+
+  print subprocess.check_output('"' + clang_path + '" ' + '-m32' + ' ' + ' '.join(object_files) + ' -o' + ' ' + options.release_dir + '\integration_tests_dll.dll' + ' /link /dll ' + options.release_dir + '\export_dll.dll.lib ' + options.release_dir + '\syzyasan_rtl.dll.lib' + ' -defaultlib:libcmt', shell = True) 

[Sébastien Marchand, 2017/06/21 15:32:49]

Same comments as above. I'd prefer to split this into several functions:
- One to do the compile step
- One to do the link one.

[njanevsk, 2017/06/21 19:26:49]

Done.

+
+if __name__ == '__main__':
+  sys.exit(main())