[webkit-reviews] review granted: [Bug 130523] Need target to produce llvm ir : [Attachment 228916] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has granted Matthew Mirman
<mmirman at apple.com>'s request for review:
Bug 130523: Need target to produce llvm ir
https://bugs.webkit.org/show_bug.cgi?id=130523

Attachment 228916: Added target to produce llvm ir for JavaScriptCore.	WIP for
C++ inlining.
https://bugs.webkit.org/attachment.cgi?id=228916&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=228916&action=review


r=me with a few comments. If you upload a version that addresses those we can
get it cq+d.

> Source/JavaScriptCore/ChangeLog:12
> +	   * build_symbol_table_index.py: Added.
> +	   * build_symbol_table_index.sh: Added.
> +	   * Configurations/CompileRuntimeToLLVMIR.xcconfig: Added.
> +	   * copy_llvm_ir_to_derived_sources.sh: Added.

All of the other scripts at the top level of JavaScriptCore are named with
lower-case-and-hyphens rather than using underscores.

> Source/JavaScriptCore/build_symbol_table_index.py:20
> +    print ("Failed to build index table at " + binary_file_directory)

You've an unnecessary space after print.

> Source/JavaScriptCore/build_symbol_table_index.py:38
> +    bitcode_basename = os.path.basename(bitcode_file)
> +    binary_file = os.path.join(binary_file_directory, bitcode_basename[:-2]
+ "o")

Why do we need to look at the version of the file with a .o suffix? Is that
because the .bc file has the wrong modification time due to being copied? Can
we switch to using "cp -p" to preserve the modification time on the .bc file,
then ignore the .o files in this script?

> Source/JavaScriptCore/build_symbol_table_index.sh:4
> +OBJ_DIR=${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCoreRuntime
> +SAVE_DIR=${BUILT_PRODUCTS_DIR}/${JAVASCRIPTCORE_RESOURCES_DIR}/Runtime

These variable names don't match what the directories are. The former is the
derived sources directory, not an object directory, and the latter is the
installed location.

> Source/JavaScriptCore/copy_llvm_ir_to_derived_sources.sh:4
> +OBJ_DIR=${TARGET_TEMP_DIR}/Objects-${CURRENT_VARIANT}
> +SAVE_DIR=${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCoreRuntime

I'd suggest renaming SAVE_DIR to match whatever you change OBJ_DIR to in the
other script.