[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 #23 from Mark Rowe (bdash) <mrowe at apple.com>  2014-04-08 15:48:36 PST ---
(From update of attachment 228898)
View in context: https://bugs.webkit.org/attachment.cgi?id=228898&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        * build_index.py: Added.

This should have a more descriptive name if it is going to live at the top level of the JavaScriptCore source directory.

> Source/JavaScriptCore/Configurations/CompileRuntimeToLLVMIR.xcconfig:36
> +JSVALUE_MODEL = $(JSVALUE_MODEL_$(CURRENT_ARCH));
> +JSVALUE_MODEL_ = UNKNOWN_JSVALUE_MODEL;
> +JSVALUE_MODEL_armv7 = 32_64;
> +JSVALUE_MODEL_armv7k = 32_64;
> +JSVALUE_MODEL_armv7s = 32_64;
> +JSVALUE_MODEL_arm64 = 64;
> +JSVALUE_MODEL_i386 = 32_64;
> +JSVALUE_MODEL_x86_64 = 64;

Are these even used by anything? I know they exist in JavaScriptCore.xcconfig too, but I can't see any other references to JSVALUE_MODEL.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6552
> +			name = "Build Symbol Index Table";

This phase now does two different things: 1) builds the symbol index, and 2) it copies bitcode files in to the framework. The name doesn't reflect the latter, and it wasn't obvious to me that's what it was doing without reading very closely. The fact this is an inline script rather than in a standalone file over multiple lines certainly didn't help.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6559
> +		};

Since these are bitcode files rather than object files it'd be worth renaming them to a more appropriate extension.

I think it's not great that we're hardcoding knowledge of which architectures support FTL in these script phases. Can we put that logic only in the build_index.py script and have this shell script work on all architectures we're building for, using build_index.py's exit status to communicate that there's no need to copy object files around?

I'm sad that we're adding yet another script phase to JavaScriptCore that doesn't support proper dependency tracking.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6560
> +		559CD06A18F487A800F9ADC0 /* ShellScript */ = {

I'm having a hard time following what this script phase does.  A descriptive name would help.

> Source/JavaScriptCore/build_index.py:21
> +dir = os.getenv("OBJECT_FILE_DIR_" + os.getenv("CURRENT_VARIANT")) + "/" + current_arch

Is this still used? It looks like the script phases expect this to use object files from elsewhere?

> Source/JavaScriptCore/build_index.py:29
> +new_loc = built_products_dir + "/" + os.getenv("JAVASCRIPTCORE_RESOURCES_DIR") + "/JavaScriptCoreRuntime/" + current_arch

new_loc is still a bad variable name. It's not descriptive of what it represents, and it doesn't conform to the style guidelines of avoiding unnecessary abbreviations.

os.path.join could be clearer than manually concatenating with slashes.

It seems a little redundant to have a JavaScriptcore prefix on a folder that is within the JavaScriptCore framework.

> Source/JavaScriptCore/build_index.py:31
> +symbol_table_location = new_loc + "/JavaScriptCoreRuntime.symtbl"

Ditto about the redundant prefix.

> Source/JavaScriptCore/build_index.py:35
> +modified = False

This could be more descriptive. Something like symbol_table_is_out_of_date, for instance.

> Source/JavaScriptCore/build_index.py:40
> +    time_modified = os.path.getmtime(symbol_table_location)

Time what was modified? symbol_table_modification_time perhaps?

> Source/JavaScriptCore/build_index.py:44
> +    bitcode_basename = os.path.basename(bitcode_file)
> +    binary_file = dir + "/" + bitcode_basename

Why the switch from bitcode to binary to describe this?

> Source/JavaScriptCore/build_index.py:51
> +    lines = runBash("nm -U -j " + binary_file).splitlines()

In an earlier review I mentioned the issues with building command lines as strings rather than arrays, and your runBash function appears unnecessary in face of functions provided by the subprocess module.

> Source/JavaScriptCore/build_index.py:58
> +    sys.exit()

When early-exiting here, the output of the script will be only "Building Index Table". That seems odd.

> Source/JavaScriptCore/build_index.py:64
> +            symbol, _, loc = line[:-1].partition(" ")

loc is still a bad name.

> Source/JavaScriptCore/build_index.py:65
> +            if not symbol in symbol_table:

Why is it necessary to check for existence first?

> Source/JavaScriptCore/build_index.py:67
> +

Given that you always merge symbols from the updated bitcode files in to the existing symbol table, we'll never remove symbols once they enter this table. Even deleting files from JavaScriptCore won't result in their symbols being removed.

What impact will having entries in the symbol table that don't correspond to actual symbols in the bitcode have on JavaScriptCore's behavior?

> Source/JavaScriptCore/build_index.py:73
> +    for symbol,location in symbol_list:

Missing space after the comma.

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