|
Description
Matthew Mirman
2014-03-20 11:38:59 PDT
Created attachment 227310 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Did not make target a dependency for the JavaScriptCore target so it is currently not used.
Comment on attachment 227310 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
For starters, you don't want *all* of JSC to be inlineable. Not all of JSC is callable from JS. Only functions in runtime/ are.
Please give the new target a more descriptive name than JavaScriptCoreLLVM, remove the redundant configuration settings (e.g., INFOPLIST_FILE, GCC_VERSION, GCC_TREAT_WARNINGS_AS_ERRORS), and put the remaining configuration settings in a new .xcconfig file in JavaScriptCore/Configurations. It's not obvious to me from the diff where the file is being generated to. $(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore would be consistent with our other generated files. Created attachment 227321 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
(In reply to comment #3) > Please give the new target a more descriptive name than JavaScriptCoreLLVM, remove the redundant configuration settings (e.g., INFOPLIST_FILE, GCC_VERSION, GCC_TREAT_WARNINGS_AS_ERRORS), and put the remaining configuration settings in a new .xcconfig file in JavaScriptCore/Configurations. How do I get the target to read from the new .xcconfig file/can I auto-generate this file? > It's not obvious to me from the diff where the file is being generated to. $(BUILT_PRODUCTS_DIR)/DerivedSources/JavaScriptCore would be consistent with our other generated files. (In reply to comment #5) > (In reply to comment #3) > > Please give the new target a more descriptive name than JavaScriptCoreLLVM, remove the redundant configuration settings (e.g., INFOPLIST_FILE, GCC_VERSION, GCC_TREAT_WARNINGS_AS_ERRORS), and put the remaining configuration settings in a new .xcconfig file in JavaScriptCore/Configurations. > > How do I get the target to read from the new .xcconfig file/can I auto-generate this file? Start by adding the new .xcconfig to the Configurations group of the Xcode project. Then, in Xcode's project navigator (leftmost tab in the left sidebar in the main window), select the project entry at the very top. In the resulting configuration view, select the project at the top of the Project / Targets list. Expand each configuration. Next to each target on the right you'll see a pop-up menu that lets you pick a .xcconfig file to associate with the target. Associate your new .xcconfig file with your new target for each of the four configurations. The .xcconfig file itself should contain: 1) the standard license header 2) #include "FeatureDefines.xcconfig" and #include "Version.xcconfig" 3) The relevant configuration settings that your patch currently includes in the diff (OTHER_CFLAGS, OTHER_CPLUSPLUSFLAGS, SKIP_INSTALL?). Created attachment 228316 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Now it gets included into the dependencies for JavaScriptCore
Comment on attachment 228316 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. View in context: https://bugs.webkit.org/attachment.cgi?id=228316&action=review > Source/JavaScriptCore/Configurations/JavaScriptCoreRuntimeToLLVMir.xcconfig:1 > +// Copyright (C) 2009 Apple Inc. All rights reserved. 2014! > Source/JavaScriptCore/Configurations/JavaScriptCoreRuntimeToLLVMir.xcconfig:24 > +#include "JavaScriptCore.xcconfig" I don't think you want to pull in JavaScriptCore.xcconfig here. Targets typically #include only FeatureDefines.xcconfig and Version.xcconfig (unless they're following a common template like ToolExecutable.xcconfig). > Source/JavaScriptCore/Configurations/JavaScriptCoreRuntimeToLLVMir.xcconfig:26 > +CLANG_MODULES_AUTOLINK = YES; This doesn't seem relevant for this target. > Source/JavaScriptCore/Configurations/JavaScriptCoreRuntimeToLLVMir.xcconfig:31 > +INFOPLIST_FILE = Info.plist; Nor does this. > Source/JavaScriptCore/Configurations/JavaScriptCoreRuntimeToLLVMir.xcconfig:37 > +GCC_TREAT_WARNINGS_AS_ERRORS = YES; This is already set in Base.xcconfig, which is pulled in at the project level. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:5909 > + 5540756C18DA58AD00EFF7F2 /* Headers */ = { > + isa = PBXHeadersBuildPhase; I don't think this target needs a copy headers phase. We wouldn't do anything with the headers anyway. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7621 > + 5540756218DA58AD00EFF7F2 /* JavaScriptCoreRuntimeToLLVMir */ = { I'm not a huge fan of the name of this target. "Compile runtime to LLVM IR" would be more descriptive. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7640 > + productInstallPath = "${SYSTEM_LIBRARY_DIR}/Frameworks/WebKit.framework/Versions/A/Frameworks"; I'm not quite clear on what this specifies, but it doesn't seem right for something related to JavaScriptCore? > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7756 > + files = ( > + 5581AC2A18EB45D200D7CBF1 /* JavaScriptCoreRuntimeToLLVMir.xcconfig in Resources */, > + 5540756B18DA58AD00EFF7F2 /* framework.sb in Resources */, These don't look like they're really resources of this target. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7837 > + 5536CC9518EB53A30093F8DB /* ShellScript */ = { This script phase should have a more descriptive name. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7843 > + "$(TARGET_TEMP_DIR)/Objects-normal/$(CURRENT_ARCH)/*.o", Does Xcode expand wildcards in the input paths? I'm not sure whether it does or not. Does this work when building for multiple architectures? I suspect the script phase only gets run once, meaning you'll only handle a single architecture? Hardcoding "Objects-normal" doesn't seem great either. I think you can use something like $(OBJECT_FILE_DIR_$(CURRENT_VARIANT))/$(CURRENT_ARCH) to get the path you're after. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7850 > + shellScript = "import os\nimport subprocess\nimport glob\nfrom operator import itemgetter\n\ndef runBash(cmd):\n p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE)\n out = p.stdout.read().strip()\n return out #This is the stdout from the shell command\n\ndir = os.getenv(\"TARGET_TEMP_DIR\")+\"/Objects-normal/\"+os.getenv(\"CURRENT_ARCH\")\n\nsymbol_table_loc = os.getenv(\"BUILT_PRODUCTS_DIR\")+\"/DerivedSources/JavaScriptCoreRuntime.symtbl\"\n\nsymbol_table = {}\n\nmodified = False\n\nfor f in glob.iglob(dir + \"/*.o\"):\n if not os.path.isfile(f+\".bc\") or os.path.getmtime(f) >= os.path.getmtime(f+\".bc\"):\n modified = True\n print(\"ASSEMBLING: \"+f)\n runBash(\"llvm-as \"+f)\n print(\"APPENDING SYMBOLS\")\n lines = runBash(\"llvm-nm --defined-only -P \"+f+\".bc\").splitlines()\n for l in lines:\n sym,_,ty = l.partition(\" \")\n if ty != \"d\" and ty != \"D\":\n name = f+\".bc\"\n symbol_table[sym] = name\n\nif modified:\n if os.path.isfile(symbol_table_loc):\n with open(symbol_table_loc, 'r') as file:\n print(\"LOADING SYMBOL TABLE\")\n for l in iter(file.readline, ''):\n symbol,_,loc = l[:-1].partition(\" \")\n if not symbol in symbol_table:\n symbol_table[symbol] = loc\n\n symbol_list = symbol_table.items()\n\n print(\"WRITING SYMBOL TABLE\")\n with open(symbol_table_loc, \"w\") as file:\n file.writelines(map(lambda (symbol, loc): symbol + \" \" + loc + \"\\n\", symbol_list))\n print(\"DONE\")"; This script is long enough that it should really live in a standalone file. As it is it's difficult to review. How long does it take to run? Are the dependencies set up correctly so it won't be run when its input hasn't changed? If not and that's hard to arrange, does it at least exit quickly without doing redundant work in that scenario? > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7853 > + 5540756918DA58AD00EFF7F2 /* Update Info.plist with version information */ = { > + isa = PBXShellScriptBuildPhase; This build phase doesn't seem relevant to this target. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7868 > + 5540789918DA58AD00EFF7F2 /* Postprocess Headers */ = { Ditto. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:9116 > + buildSettings = { > + }; I think you may need to set a few settings in the Debug configuration only to get the same defines as we do in JavaScriptCore proper: DEBUG_DEFINES = "$(DEBUG_DEFINES_debug)"; GCC_OPTIMIZATION_LEVEL = "$(GCC_OPTIMIZATION_LEVEL_debug)"; Created attachment 228340 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Have not yet changed name of target - do not approve this patch. Just testing on non local build system.
Attachment 228340 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/build_index.py:8: expected 2 blank lines, found 1 [pep8/E302] [5]
ERROR: Source/JavaScriptCore/build_index.py:11: inline comment should start with '# ' [pep8/E262] [5]
ERROR: Source/JavaScriptCore/build_index.py:13: missing whitespace around operator [pep8/E225] [5]
ERROR: Source/JavaScriptCore/build_index.py:15: missing whitespace around operator [pep8/E225] [5]
ERROR: Source/JavaScriptCore/build_index.py:22: missing whitespace around operator [pep8/E225] [5]
ERROR: Source/JavaScriptCore/build_index.py:24: missing whitespace around operator [pep8/E225] [5]
ERROR: Source/JavaScriptCore/build_index.py:25: missing whitespace around operator [pep8/E225] [5]
ERROR: Source/JavaScriptCore/build_index.py:27: missing whitespace around operator [pep8/E225] [5]
ERROR: Source/JavaScriptCore/build_index.py:29: missing whitespace after ',' [pep8/E231] [5]
ERROR: Source/JavaScriptCore/build_index.py:31: missing whitespace around operator [pep8/E225] [5]
ERROR: Source/JavaScriptCore/build_index.py:39: missing whitespace after ',' [pep8/E231] [5]
Total errors found: 11 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 228350 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Fixed style. Fixed names. Removed some excess things.
(In reply to comment #8) > (From update of attachment 228316 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228316&action=review > > > Source/JavaScriptCore/Configurations/JavaScriptCoreRuntimeToLLVMir.xcconfig:1 > > +// Copyright (C) 2009 Apple Inc. All rights reserved. > > 2014! > > > Source/JavaScriptCore/Configurations/JavaScriptCoreRuntimeToLLVMir.xcconfig:24 > > +#include "JavaScriptCore.xcconfig" > > I don't think you want to pull in JavaScriptCore.xcconfig here. Targets typically #include only FeatureDefines.xcconfig and Version.xcconfig (unless they're following a common template like ToolExecutable.xcconfig). JavaScriptCore.xcconfig contains some build warnings and directives which should be exactly the same as in CompileRuntimeToLLVMir and without the import and the includes (which could get excessively long and redundant) it doesn't compile. > > > Source/JavaScriptCore/Configurations/JavaScriptCoreRuntimeToLLVMir.xcconfig:26 > > +CLANG_MODULES_AUTOLINK = YES; > > This doesn't seem relevant for this target. > > > Source/JavaScriptCore/Configurations/JavaScriptCoreRuntimeToLLVMir.xcconfig:31 > > +INFOPLIST_FILE = Info.plist; > > Nor does this. > > > Source/JavaScriptCore/Configurations/JavaScriptCoreRuntimeToLLVMir.xcconfig:37 > > +GCC_TREAT_WARNINGS_AS_ERRORS = YES; > > This is already set in Base.xcconfig, which is pulled in at the project level. > > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:5909 > > + 5540756C18DA58AD00EFF7F2 /* Headers */ = { > > + isa = PBXHeadersBuildPhase; > > I don't think this target needs a copy headers phase. We wouldn't do anything with the headers anyway. It does, but it didn't need to copy so many headers. This is fixed. > > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7621 > > + 5540756218DA58AD00EFF7F2 /* JavaScriptCoreRuntimeToLLVMir */ = { > > I'm not a huge fan of the name of this target. "Compile runtime to LLVM IR" would be more descriptive. > > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7640 > > + productInstallPath = "${SYSTEM_LIBRARY_DIR}/Frameworks/WebKit.framework/Versions/A/Frameworks"; > > I'm not quite clear on what this specifies, but it doesn't seem right for something related to JavaScriptCore? > > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7756 > > + files = ( > > + 5581AC2A18EB45D200D7CBF1 /* JavaScriptCoreRuntimeToLLVMir.xcconfig in Resources */, > > + 5540756B18DA58AD00EFF7F2 /* framework.sb in Resources */, > > These don't look like they're really resources of this target. > > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7837 > > + 5536CC9518EB53A30093F8DB /* ShellScript */ = { > > This script phase should have a more descriptive name. > > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7843 > > + "$(TARGET_TEMP_DIR)/Objects-normal/$(CURRENT_ARCH)/*.o", > > Does Xcode expand wildcards in the input paths? I'm not sure whether it does or not. > > Does this work when building for multiple architectures? I suspect the script phase only gets run once, meaning you'll only handle a single architecture? > I was under the impression that the FTL is currently only on for x86_64 anyway. > Hardcoding "Objects-normal" doesn't seem great either. I think you can use something like $(OBJECT_FILE_DIR_$(CURRENT_VARIANT))/$(CURRENT_ARCH) to get the path you're after. > > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7850 > > + shellScript = "import os\nimport subprocess\nimport glob\nfrom operator import itemgetter\n\ndef runBash(cmd):\n p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE)\n out = p.stdout.read().strip()\n return out #This is the stdout from the shell command\n\ndir = os.getenv(\"TARGET_TEMP_DIR\")+\"/Objects-normal/\"+os.getenv(\"CURRENT_ARCH\")\n\nsymbol_table_loc = os.getenv(\"BUILT_PRODUCTS_DIR\")+\"/DerivedSources/JavaScriptCoreRuntime.symtbl\"\n\nsymbol_table = {}\n\nmodified = False\n\nfor f in glob.iglob(dir + \"/*.o\"):\n if not os.path.isfile(f+\".bc\") or os.path.getmtime(f) >= os.path.getmtime(f+\".bc\"):\n modified = True\n print(\"ASSEMBLING: \"+f)\n runBash(\"llvm-as \"+f)\n print(\"APPENDING SYMBOLS\")\n lines = runBash(\"llvm-nm --defined-only -P \"+f+\".bc\").splitlines()\n for l in lines:\n sym,_,ty = l.partition(\" \")\n if ty != \"d\" and ty != \"D\":\n name = f+\".bc\"\n symbol_table[sym] = name\n\nif modified:\n if os.path.isfile(symbol_table_loc):\n with open(symbol_table_loc, 'r') as file:\n print(\"LOADING SYMBOL TABLE\")\n for l in iter(file.readline, ''):\n symbol,_,loc = l[:-1].partition(\" \")\n if not symbol in symbol_table:\n symbol_table[symbol] = loc\n\n symbol_list = symbol_table.items()\n\n print(\"WRITING SYMBOL TABLE\")\n with open(symbol_table_loc, \"w\") as file:\n file.writelines(map(lambda (symbol, loc): symbol + \" \" + loc + \"\\n\", symbol_list))\n print(\"DONE\")"; > > This script is long enough that it should really live in a standalone file. As it is it's difficult to review. > > How long does it take to run? Are the dependencies set up correctly so it won't be run when its input hasn't changed? If not and that's hard to arrange, does it at least exit quickly without doing redundant work in that scenario? It takes about 5 minutes to run the first time - I've talked to Geoff and this is acceptable for this release. The script is input file modification time aware, and does not do excessive work. > > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7853 > > + 5540756918DA58AD00EFF7F2 /* Update Info.plist with version information */ = { > > + isa = PBXShellScriptBuildPhase; > > This build phase doesn't seem relevant to this target. > > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:7868 > > + 5540789918DA58AD00EFF7F2 /* Postprocess Headers */ = { > > Ditto. > > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:9116 > > + buildSettings = { > > + }; > > I think you may need to set a few settings in the Debug configuration only to get the same defines as we do in JavaScriptCore proper: > > DEBUG_DEFINES = "$(DEBUG_DEFINES_debug)"; > GCC_OPTIMIZATION_LEVEL = "$(GCC_OPTIMIZATION_LEVEL_debug)"; These appear to cause build failure. Comment on attachment 228350 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. View in context: https://bugs.webkit.org/attachment.cgi?id=228350&action=review > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6471 > + productInstallPath = "${SYSTEM_LIBRARY_DIR}/Frameworks/WebKit.framework/Versions/A/Frameworks"; This still needs to be fixed. Maybe just delete it? Or change it to JS/Resources. > Source/JavaScriptCore/build_index.py:9 > +def runBash(cmd): Let's make sure not to run this script when building 32bit. > Source/JavaScriptCore/build_index.py:14 > +# dir = os.getenv("TARGET_TEMP_DIR") + "/Objects-normal/" + os.getenv("CURRENT_ARCH") Let's nix this. > Source/JavaScriptCore/build_index.py:17 > +symbol_table_loc = os.getenv("BUILT_PRODUCTS_DIR") + "/DerivedSources/JavaScriptCoreRuntime.symtbl" Our table to load at runtime needs to be in the framework's "Resources" folder, so it's available at runtime. DerivedSources is only available at compile time. Created attachment 228629 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Removed many of the C++ files.
Fixed the handling of not 64 bit by the script.
Removed the remnant of WebKit from the install path.
Attachment 228629 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/build_index.py:30: multiple statements on one line (semicolon) [pep8/E702] [5]
ERROR: Source/JavaScriptCore/build_index.py:46: trailing whitespace [pep8/W291] [5]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 228630 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Fixed python style
Comment on attachment 228630 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. Clearing flags on attachment: 228630 Committed r166820: <http://trac.webkit.org/changeset/166820> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 131256 Comment on attachment 228630 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. 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)) Created attachment 228898 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
No longer makes any calls to llvm-nm or llvm-as. Only uses "nm".
Note: the python script only uses "runBash" to capture the output of bash, and only uses it once.
Attachment 228898 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/build_index.py:9: expected 2 blank lines, found 1 [pep8/E302] [5]
ERROR: Source/JavaScriptCore/build_index.py:47: trailing whitespace [pep8/W291] [5]
ERROR: Source/JavaScriptCore/build_index.py:49: trailing whitespace [pep8/W291] [5]
ERROR: Source/JavaScriptCore/build_index.py:52: trailing whitespace [pep8/W291] [5]
ERROR: Source/JavaScriptCore/build_index.py:73: missing whitespace after ',' [pep8/E231] [5]
Total errors found: 5 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 228898 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. 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. Created attachment 228911 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Does not handle stale Runtime.symtbl
The worst thing that can occur is that the symbol table will contain symbols which no longer exist. At the moment this will have no effect on runtime since Runtime.symtbl is not used.
When Runtime.symtbl is being used (patch forthcoming), either the symbols will never be accessed (since they don't exist exposed in runtime, they can't be called by js code), or when they are accessed the FTL code will simply not inline them after catching an error.
Attachment 228911 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/build_symbol_table_index.py:23: whitespace before ',' [pep8/E203] [5]
ERROR: Source/JavaScriptCore/build_symbol_table_index.py:38: whitespace after '(' [pep8/E201] [5]
ERROR: Source/JavaScriptCore/build_symbol_table_index.py:60: trailing whitespace [pep8/W291] [5]
ERROR: Source/JavaScriptCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5]
Total errors found: 4 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 228913 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Fixed style
Created attachment 228916 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Fixed JavaScriptCoreRuntime to Runtime and removed shell=True.
Comment on attachment 228916 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Lgtm, but I want bdash's opinion also.
Comment on attachment 228916 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. 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. (In reply to comment #29) > (From update of attachment 228916 [details]) > 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? I'm using the .o version (which is a binary object file, and not an llvm bitcode file), so that I can get its symbol table using "nm" . The modification time is just a perk. > > > 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. Created attachment 229002 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. > 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? I'm using the .o version (which is a binary object file, and not an llvm bitcode file), so that I can get its symbol table using "nm" . The modification time is just a perk. Created attachment 229003 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Fixed case on LLVMir.xcconfig to LLVMIR.xcconfig
Created attachment 229005 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Added SECTORDER_FLAGS = ;
The patch looks fine and seems to work ok for production builds, but I'm hesitant to r+ it. After building it on my machine I noticed that this increases the size of the JavaScriptCore framework substantially. A single architecture copy of JavaScriptCore.framework went from 30MB to over 280MB. I'm not sure we can land this when it increases the size of the framework on disk by an order of magnitude. Created attachment 229071 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Currently at 70mb. Will do better by reducing the number of files to be included. Just a WIP.
Comment on attachment 229071 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Clearing the review and commit-queue flags since it sounds like Matthew doesn't want this version reviewed.
Created attachment 229075 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Culled the list of files to compile down to only two. Should now only take up an extra 3mb. Ready for review.
Comment on attachment 229075 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. View in context: https://bugs.webkit.org/attachment.cgi?id=229075&action=review > Source/JavaScriptCore/copy-llvm-ir-to-derived-sources.sh:14 > + tar -czvf "$RUNTIME_DERIVED_SOURCES_DIR/$arch/${file_name%.o}.bc.tar.gz" "$file" Using tar to compress a single file is weird, and seems like it'll make it harder to work with the files at runtime. Is there any reason not to use gzip directly? Created attachment 229078 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Fixed tar to gzip
Created attachment 229079 [details]
Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining.
Forgot to change .tar.gz to just .gz in one of the added scripts.
Comment on attachment 229079 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. Clearing flags on attachment: 229079 Committed r167087: <http://trac.webkit.org/changeset/167087> All reviewed patches have been landed. Closing bug. Comment on attachment 229079 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. View in context: https://bugs.webkit.org/attachment.cgi?id=229079&action=review > Source/JavaScriptCore/build-symbol-table-index.py:43 > +for bitcode_file in glob.iglob(os.path.join(framework_directory, "*." + file_suffix)): > + bitcode_basename = os.path.basename(bitcode_file) > + binary_file = os.path.join(binary_file_directory, bitcode_basename[:-file_suffix_length] + "o") > + if os.path.getmtime(binary_file) < symbol_table_modification_time: > + continue Something here is failing every time I compile: Traceback (most recent call last): File "/Users/darin/Safari/OpenSource/Source/JavaScriptCore/build-symbol-table-index.py", line 42, in <module> if os.path.getmtime(binary_file) < symbol_table_modification_time: File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/genericpath.py", line 54, in getmtime return os.stat(filename).st_mtime OSError: [Errno 2] No such file or directory: '/Users/darin/Build/JavaScriptCore.build/Debug/JavaScriptCore.build/Objects-normal/x86_64/*.o' I don’t think that call to glob.iglob is turning "*.o" into actual file names. But are files that match that glob on my computer. That’s the correct path. Comment on attachment 229079 [details] Added target to produce llvm ir for JavaScriptCore. WIP for C++ inlining. View in context: https://bugs.webkit.org/attachment.cgi?id=229079&action=review >> Source/JavaScriptCore/build-symbol-table-index.py:43 >> + continue > > Something here is failing every time I compile: > > Traceback (most recent call last): > File "/Users/darin/Safari/OpenSource/Source/JavaScriptCore/build-symbol-table-index.py", line 42, in <module> > if os.path.getmtime(binary_file) < symbol_table_modification_time: > File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/genericpath.py", line 54, in getmtime > return os.stat(filename).st_mtime > OSError: [Errno 2] No such file or directory: '/Users/darin/Build/JavaScriptCore.build/Debug/JavaScriptCore.build/Objects-normal/x86_64/*.o' > > I don’t think that call to glob.iglob is turning "*.o" into actual file names. > > But are files that match that glob on my computer. That’s the correct path. I see the problem. There are no *.bc.gz files in that directory, which probably makes sense given we aren’t really doing this. Can we do a quick fix this so this does the right thing when there are no files with that extension in the directory? |