[Webkit-unassigned] [Bug 130523] Need target to produce llvm ir

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=130523





--- Comment #20 from Mark Rowe (bdash) <mrowe at apple.com>  2014-04-04 20:51:45 PST ---
(From update of attachment 228630)
View in context: https://bugs.webkit.org/attachment.cgi?id=228630&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        WIP for inlining C++.  Added a build target to produce llvm ir.

"LLVM IR"

> Source/JavaScriptCore/ChangeLog:12
> +        * Configurations/CompileRuntimeToLLVMir.xcconfig: Added.

"IR"

> Source/JavaScriptCore/Configurations/CompileRuntimeToLLVMir.xcconfig:24
> +#include "JavaScriptCore.xcconfig"

If you're going to #include this you'll want to reset things that it defines that aren't necessary in this target. At the very least that's PRODUCT_NAME, INSTALLHDRS_SCRIPT_PHASE, INFOPLIST_FILE.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2591
> +		55407AC818DA58AD00EFF7F2 /* JavaScriptCore.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = JavaScriptCore.framework; sourceTree = BUILT_PRODUCTS_DIR; };

Does this exist because Xcode thinks your new target is also generating something called JavaScriptCore.framework? It may be better to recreate this new target as a static library or similar, since it's clearly not generating a framework.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6342
> +		5540756218DA58AD00EFF7F2 /* CompileRuntimeToLLVMir */ = {

Feel free to use spaces in the target name: "Compile Runtime to LLVM IR".

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6360
> +			productName = JavaScriptCore;
> +			productReference = 55407AC818DA58AD00EFF7F2 /* JavaScriptCore.framework */;
> +			productType = "com.apple.product-type.framework";

These don't seem right. This isn't building something named JavaScriptCore, and it's not a framework.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:-5838
> -			productInstallPath = "${SYSTEM_LIBRARY_DIR}/Frameworks/WebKit.framework/Versions/A/Frameworks";

This is modifying the existing JavaScriptCore target. Probably best not to do this as part of your change.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6550
> +				"$(TARGET_TEMP_DIR)/Objects-normal/$(CURRENT_ARCH)/*.o",

As I mentioned in an earlier round of review, it's not correct to hard-code Objects-normal here. It's also not correct to depend on CURRENT_ARCH here. In a build involving multiple architectures, the script phase will be invoked only once. It'll need to know how to do the work for all relevant architectures on its own.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6554
> +				"$(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCoreRuntime.symtbl",

Will this file differ between architectures? If so, it'd probably be worth encoding the architecture in the name.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7819
> +				GCC_VERSION = com.apple.compilers.llvm.clang.1_0;

This shouldn't be here.

> Source/JavaScriptCore/build_index.py:6
> +import sys
> +import os
> +import subprocess
> +import glob

You may as well sort these imports like we do for #includes.

> Source/JavaScriptCore/build_index.py:13
> +def runBash(cmd):
> +    p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE)
> +    out = p.stdout.read().strip()
> +    return out

Why do you need to run things via the shell?

Can you have callers use subprocess.check_output instead of doing this?

> Source/JavaScriptCore/build_index.py:15
> +current_arch = os.getenv("CURRENT_ARCH")

As I mentioned above, relying on CURRENT_ARCH won't do what you want when building for multiple architectures (even when we only care about one of those architectures).

> Source/JavaScriptCore/build_index.py:24
> +new_loc = os.getenv("BUILT_PRODUCTS_DIR") + "/" + os.getenv("JAVASCRIPTCORE_RESOURCES_DIR") + "/JavaScriptCoreRuntime"
> +
> +symbol_table_loc = new_loc + "/JavaScriptCoreRuntime.symtbl"

<http://www.webkit.org/coding/coding-style.html#names-full-words> says "Use full words, except in the rare case where an abbreviation would be more canonical and easier to understand".

The path you're writing this file to doesn't seem to match the location specified in Xcode's script phase output. I think it'd make more sense to write it in to DerivedSources and then have the JavaScriptCore framework target copy it in to the Resources directory. Having this target dump stuff directly in to JavaScriptCore.framework before it even exists is kinda weird.

> Source/JavaScriptCore/build_index.py:30
> +runBash("mkdir -p " + new_loc)

os.makedirs?

> Source/JavaScriptCore/build_index.py:33
> +    if not os.path.isfile(f + ".bc") or os.path.getmtime(f) >= os.path.getmtime(f + ".bc"):

An early continue here would remove a level of nesting from the rest of the loop.

> Source/JavaScriptCore/build_index.py:35
> +        print("ASSEMBLING: " + f)

There's no need to yell.

> Source/JavaScriptCore/build_index.py:36
> +        runBash("/usr/local/bin/llvm-as " + f)

You can't rely on the contents of /usr/local/bin. This is the immediate cause of the build failure that resulted in this change being rolled out. As we talked about earlier in the week, you'll need to get this binary from the same place we look for LLVM headers / libraries.

> Source/JavaScriptCore/build_index.py:39
> +        bcfile = f + ".bc"
> +        bcbase = os.path.basename(bcfile)

bitcode_file
bitcode_basename

> Source/JavaScriptCore/build_index.py:42
> +        runBash("cp " + bcfile + " " + new_loc + "/" + bcbase)

Building command lines via string concatenation is a recipe for errors when dealing with paths that can contain arbitrary characters. The obvious case here is that a path containing spaces will give unexpected results.

In this case, shelling out seems unnecessary. You can just use shutil.copy.

> Source/JavaScriptCore/build_index.py:49
> +            sym, _, ty = l.partition(" ")
> +            if ty != "d" and ty != "D":

symbol
type?

if type not in ('d', 'D'):

arguably l.spit(' ', 1) is more obvious than l.partition(' '), and it simplifies the unpacking too.

> Source/JavaScriptCore/build_index.py:52
> +if modified:

An early return or exit would help with indentation here too.

> Source/JavaScriptCore/build_index.py:56
> +            for l in iter(file.readline, ''):

Isn't this the same as:
for line in file:

> Source/JavaScriptCore/build_index.py:65
> +        file.writelines(map(lambda (symbol, loc): symbol + " " + loc + "\n", symbol_list))

Trying to squish this on to one line hurts more than it helps:

for symbol in symbol_list:
    file.write("{} {}\n".format(symbol, location))

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.