Bug 122566 – FTL: Soft-link LLVM as a workaround for LLVM's static initializers and exit-time destructors

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 213799 [details]
work in progress

[Filip Pizlo]

Created attachment 213829 [details]
almost done

[Filip Pizlo]

Created attachment 213834 [details]
the patch

[WebKit Commit Bot]

Attachment 213834 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig', u'Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/config.h', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/disassembler/LLVMDisassembler.cpp', u'Source/JavaScriptCore/ftl/FTLAbbreviatedTypes.h', u'Source/JavaScriptCore/ftl/FTLAbbreviations.h', u'Source/JavaScriptCore/ftl/FTLCompile.cpp', u'Source/JavaScriptCore/ftl/FTLFail.cpp', u'Source/JavaScriptCore/ftl/FTLJITCode.h', u'Source/JavaScriptCore/ftl/FTLJITFinalizer.h', u'Source/JavaScriptCore/ftl/FTLLink.cpp', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLState.cpp', u'Source/JavaScriptCore/ftl/WebKitLLVMLibraryAnchor.cpp', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVM.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVM.h', u'Source/JavaScriptCore/llvm/InitializeLLVMMac.mm', u'Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.h', u'Source/JavaScriptCore/llvm/LLVMAPI.cpp', u'Source/JavaScriptCore/llvm/LLVMAPI.h', u'Source/JavaScriptCore/llvm/LLVMAPIFunctions.h', u'Source/JavaScriptCore/llvm/LLVMHeaders.h', u'Source/JavaScriptCore/llvm/library/LLVMAnchor.cpp', u'Source/JavaScriptCore/llvm/library/LLVMExports.cpp', u'Source/JavaScriptCore/llvm/library/LLVMOverrides.cpp', u'Source/JavaScriptCore/runtime/InitializeThreading.cpp', u'Source/JavaScriptCore/runtime/Options.h', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wtf/LLVMHeaders.h', u'Source/WTF/wtf/text/CString.h']" exit_code: 1
Source/JavaScriptCore/llvm/LLVMAPI.h:36:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:125:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:126:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:127:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:128:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:129:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:130:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:131:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:132:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:133:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:134:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:135:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:136:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:137:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:138:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:139:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:140:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:141:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:142:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:143:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:144:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:145:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:146:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:147:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:148:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:149:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:150:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:151:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:152:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:153:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:154:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:155:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:156:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:157:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:158:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:159:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:160:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:161:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:162:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:163:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:164:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:165:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:166:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:167:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:168:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:169:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:170:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:171:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:172:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:173:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:174:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:175:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:176:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:177:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:178:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:179:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:180:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:181:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:182:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:183:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:184:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:185:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:186:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:187:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:188:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:189:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:190:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:191:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:192:  Extra space after ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:491:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:559:  Extra space after (  [whitespace/parens] [2]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:560:  Extra space after (  [whitespace/parens] [2]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:579:  Extra space after (  [whitespace/parens] [2]
Source/JavaScriptCore/disassembler/LLVMDisassembler.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/llvm/LLVMHeaders.h:48:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 75 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 213834 [details]
the patch

Attachment 213834 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3837028

[EFL EWS Bot]

Comment on attachment 213834 [details]
the patch

Attachment 213834 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3836036

[Oliver Hunt]

Comment on attachment 213834 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213834&action=review

> Source/JavaScriptCore/jsc.cpp:849
> -    RefPtr<VM> vm = VM::create(LargeHeap);
> +    VM* vm = VM::create(LargeHeap).leakRef();

Do we really have to leak the vm?

>> Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:125
>> +    macro( LLVMValueRef , IsABasicBlock, (LLVMValueRef Val)) \
> 
> Extra space after ( in function call  [whitespace/parens] [4]

Please remove these extra spaces, also change " , " to ", "

> Source/JavaScriptCore/llvm/library/LLVMOverrides.cpp:33
> +extern "C" int __cxa_atexit() { return 0; }

mmmm, tasty

[Build Bot]

Comment on attachment 213834 [details]
the patch

Attachment 213834 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3769049

[Build Bot]

Comment on attachment 213834 [details]
the patch

Attachment 213834 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3836033

[Build Bot]

Comment on attachment 213834 [details]
the patch

Attachment 213834 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3835039

[Mark Rowe (bdash)]

Comment on attachment 213834 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213834&action=review

> Source/JavaScriptCore/config.h:57
> +#if defined(__cplusplus) && !defined(WTF_SUPPRESS_FAST_MALLOC)

Why is the LLVM stuff using this header at all? Can't it have its own config.h that just pulls in Platform.h if that's all it needs?

> Source/JavaScriptCore/jsc.cpp:849
> -    RefPtr<VM> vm = VM::create(LargeHeap);
> +    VM* vm = VM::create(LargeHeap).leakRef();

This all looks unrelated to the rest of the patch?

> Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig:1
> +// Copyright (C) 2009 Apple Inc. All rights reserved.

You're living in the past!

> Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig:40
> +HEADER_SEARCH_PATHS = "${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore" $(HEADER_SEARCH_PATHS);

Why is it necessary to have DerivedSources/JavaScriptCore on the header search path? If it is truly necessary, BUILT_PRODUCTS_DIR should be referenced using $() rather than ${}.

> Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig:42
> +PRODUCT_NAME = libllvmForJSC;

The product name should be just llvmForJSC. Settting EXECUTABLE_PREFIX set to "lib" will result in the binary having the correct name.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:4853
> +			shellScript = "if [[ $ENABLE_FTL_JIT != \"ENABLE_FTL_JIT\" ]]\nthen\n    exit 0\nfi\n\n# Copy the llvmForJSC library into the framework.\nditto \"${BUILT_PRODUCTS_DIR}/libllvmForJSC.dylib\" \"${BUILT_PRODUCTS_DIR}/JavaScriptCore.framework/Versions/A/Libraries/libllvmForJSC.dylib\"\nln -fs \"${BUILT_PRODUCTS_DIR}/JavaScriptCore.framework/Versions/A/Libraries\" \"${BUILT_PRODUCTS_DIR}/JavaScriptCore.framework/Libraries\"";

The symlink should point to Versions/Current rather than Versions/A.  The destination of the symlink should also be a relative path rather than an absolute one. You can also take advantage of Xcode configuration settings like CONTENTS_FOLDER_PATH and WRAPPER_NAME to avoid hard-coding JavaScriptCore.framework/Versions/A and JavaScriptCore.framework in the script.

It may also be worth skipping the symlink step when SHALLOW_BUNDLE is equal to YES, since in that case the Versions hierarchy isn't used.

> Source/JavaScriptCore/llvm/InitializeLLVM.cpp:31
> +#include <wtf/Threading.h>

It doesn't look like you use anything from wtf/Threading.h.

> Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:34
> +#import <wtf/DataLog.h>
> +#import <wtf/StringPrintStream.h>

These don't seem necessary.

> Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:37
> +@interface JSCLLVMDummy : NSObject

I'd name this something like JavaScriptCoreBundleFinder to follow WebCore's lead.

> Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:41
> +@implementation JSCLLVMDummy {
> +}

These braces are not necessary.

> Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:51
> +    NSBundle* myBundle = [NSBundle bundleForClass:[JSCLLVMDummy class]];
> +    NSString* path = [myBundle bundlePath];
> +    
> +    initializeLLVMPOSIX(toCString([path UTF8String], "/Libraries/libllvmForJSC.dylib").data());

Your *'s are in the wrong place for Objective-C types. It seems weird to bother with toCString here when you could just build up the entire path as an NSString:

NSString *bundle = [NSBundle bundleForClass:[JavaScriptCoreBundleFinder class]];
NSString *path = [[bundle bundlePath] stringByAppendingPathComponent:@"Libraries/libllvmForJSC.dylib"];

initializeLLVMPOSIX([path UTF8String]);

> Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp:44
> +    InitializerFunction initializer = bitwise_cast<InitializerFunction>(
> +        dlsym(library, "initializeAndGetJSCLLVMAPI"));

I'm not sure it's necessary to wrap things like this. I think you've been staring at LLVM code for too long :)

> Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:31
> +#define FOR_EACH_LLVM_API_FUNCTION(macro) \

There are a lot of style issues pointed out by the style checker that it'd be good to address.

> Source/JavaScriptCore/llvm/LLVMHeaders.h:37
> +#undef __STDC_LIMIT_MACROS
> +#undef __STDC_CONSTANT_MACROS
> +#define __STDC_LIMIT_MACROS
> +#define __STDC_CONSTANT_MACROS

Should we be resetting these macros to their original state at the end of the header?

[Filip Pizlo]

(In reply to comment #11)
> (From update of attachment 213834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213834&action=review
> 
> > Source/JavaScriptCore/config.h:57
> > +#if defined(__cplusplus) && !defined(WTF_SUPPRESS_FAST_MALLOC)
> 
> Why is the LLVM stuff using this header at all? Can't it have its own config.h that just pulls in Platform.h if that's all it needs?

I'll create a config_llvm.h.

> 
> > Source/JavaScriptCore/jsc.cpp:849
> > -    RefPtr<VM> vm = VM::create(LargeHeap);
> > +    VM* vm = VM::create(LargeHeap).leakRef();
> 
> This all looks unrelated to the rest of the patch?

It's related.  The jsc command-line normally works the way that this patch makes it work: it leaks a VM.  We changed it to not leak a VM to work around exit-time destructors on LLVM: we would force a tidy VM shutdown to ensure that the FTL compiler thread wasn't running while LLVM's exit-time destructors ran.

This patch fixes exit-time destructors and static initializers in LLVM by doing soft linking.  Hence we can get rid of the hack in jsc.

> 
> > Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig:1
> > +// Copyright (C) 2009 Apple Inc. All rights reserved.
> 
> You're living in the past!

Oops.

> 
> > Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig:40
> > +HEADER_SEARCH_PATHS = "${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore" $(HEADER_SEARCH_PATHS);
> 
> Why is it necessary to have DerivedSources/JavaScriptCore on the header search path? If it is truly necessary, BUILT_PRODUCTS_DIR should be referenced using $() rather than ${}.

It is necessary because JavaScriptCore.framework and llvmForJSC.dylib share the LLVMAPI.h, LLVMAPIFunctions.h, and LLVMHeaders.h files.

As for using $() versus ${}, I was just doing what the other xcconfig's do.

> 
> > Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig:42
> > +PRODUCT_NAME = libllvmForJSC;
> 
> The product name should be just llvmForJSC. Settting EXECUTABLE_PREFIX set to "lib" will result in the binary having the correct name.

Interesting.  Xcode's default build settings used PRODUCT_NAME=libllvmForJSC, hence that's what I did here!  I can change to what you suggest, though.

> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:4853
> > +			shellScript = "if [[ $ENABLE_FTL_JIT != \"ENABLE_FTL_JIT\" ]]\nthen\n    exit 0\nfi\n\n# Copy the llvmForJSC library into the framework.\nditto \"${BUILT_PRODUCTS_DIR}/libllvmForJSC.dylib\" \"${BUILT_PRODUCTS_DIR}/JavaScriptCore.framework/Versions/A/Libraries/libllvmForJSC.dylib\"\nln -fs \"${BUILT_PRODUCTS_DIR}/JavaScriptCore.framework/Versions/A/Libraries\" \"${BUILT_PRODUCTS_DIR}/JavaScriptCore.framework/Libraries\"";
> 
> The symlink should point to Versions/Current rather than Versions/A.  The destination of the symlink should also be a relative path rather than an absolute one. You can also take advantage of Xcode configuration settings like CONTENTS_FOLDER_PATH and WRAPPER_NAME to avoid hard-coding JavaScriptCore.framework/Versions/A and JavaScriptCore.framework in the script.

OK, I'll investigate this.

> 
> It may also be worth skipping the symlink step when SHALLOW_BUNDLE is equal to YES, since in that case the Versions hierarchy isn't used.

Hmmm, that's an iOS only thing, right?  Seems best to skip this until we are building on iOS and can actually test it.

> 
> > Source/JavaScriptCore/llvm/InitializeLLVM.cpp:31
> > +#include <wtf/Threading.h>
> 
> It doesn't look like you use anything from wtf/Threading.h.

pthread_once().

> 
> > Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:34
> > +#import <wtf/DataLog.h>
> > +#import <wtf/StringPrintStream.h>
> 
> These don't seem necessary.

StringPrintStream gives you toCString(), which this uses.  But I'll follow your suggestion and use NSString's APIs.

> 
> > Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:37
> > +@interface JSCLLVMDummy : NSObject
> 
> I'd name this something like JavaScriptCoreBundleFinder to follow WebCore's lead.

OK.

> 
> > Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:41
> > +@implementation JSCLLVMDummy {
> > +}
> 
> These braces are not necessary.

Ok, I'll change it to:

@implementation Blah
@end

For appropriate <Blah>.

> 
> > Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:51
> > +    NSBundle* myBundle = [NSBundle bundleForClass:[JSCLLVMDummy class]];
> > +    NSString* path = [myBundle bundlePath];
> > +    
> > +    initializeLLVMPOSIX(toCString([path UTF8String], "/Libraries/libllvmForJSC.dylib").data());
> 
> Your *'s are in the wrong place for Objective-C types. It seems weird to bother with toCString here when you could just build up the entire path as an NSString:
> 
> NSString *bundle = [NSBundle bundleForClass:[JavaScriptCoreBundleFinder class]];
> NSString *path = [[bundle bundlePath] stringByAppendingPathComponent:@"Libraries/libllvmForJSC.dylib"];
> 
> initializeLLVMPOSIX([path UTF8String]);

Groovy, I'll do that.

> 
> > Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp:44
> > +    InitializerFunction initializer = bitwise_cast<InitializerFunction>(
> > +        dlsym(library, "initializeAndGetJSCLLVMAPI"));
> 
> I'm not sure it's necessary to wrap things like this. I think you've been staring at LLVM code for too long :)

I always wrap things because that's just how I roll.

> 
> > Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:31
> > +#define FOR_EACH_LLVM_API_FUNCTION(macro) \
> 
> There are a lot of style issues pointed out by the style checker that it'd be good to address.

:-/

This part was auto-generated.  I'll address most of the issues but I'm not sure to what extent it's worthwhile to be strict about this.  For example, LLVM uses a different parameter naming convention than we do and for this part it makes sense to just follow what the LLVM headers actually did rather than forcing WebKit convention.

> 
> > Source/JavaScriptCore/llvm/LLVMHeaders.h:37
> > +#undef __STDC_LIMIT_MACROS
> > +#undef __STDC_CONSTANT_MACROS
> > +#define __STDC_LIMIT_MACROS
> > +#define __STDC_CONSTANT_MACROS
> 
> Should we be resetting these macros to their original state at the end of the header?

That's not a bad idea.  OTOH, these macros just mean that you get more C-isms; we don't use those C-isms (like INT64_C(...)) but having them defined doesn't actually hurt.  OTOH, I wouldn't mind having those C-isms available in WebKit; I've definitely written some terrible code in the past just to get the equivalent of the C limit macros.

[Filip Pizlo]

About to upload a new patch, comments below indicating what I fixed or didn't fix.

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 213834 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213834&action=review
> > 
> > > Source/JavaScriptCore/config.h:57
> > > +#if defined(__cplusplus) && !defined(WTF_SUPPRESS_FAST_MALLOC)
> > 
> > Why is the LLVM stuff using this header at all? Can't it have its own config.h that just pulls in Platform.h if that's all it needs?
> 
> I'll create a config_llvm.h.

Fixed.

> 
> > 
> > > Source/JavaScriptCore/jsc.cpp:849
> > > -    RefPtr<VM> vm = VM::create(LargeHeap);
> > > +    VM* vm = VM::create(LargeHeap).leakRef();
> > 
> > This all looks unrelated to the rest of the patch?
> 
> It's related.  The jsc command-line normally works the way that this patch makes it work: it leaks a VM.  We changed it to not leak a VM to work around exit-time destructors on LLVM: we would force a tidy VM shutdown to ensure that the FTL compiler thread wasn't running while LLVM's exit-time destructors ran.
> 
> This patch fixes exit-time destructors and static initializers in LLVM by doing soft linking.  Hence we can get rid of the hack in jsc.

I kept this.  It's related.

> 
> > 
> > > Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig:1
> > > +// Copyright (C) 2009 Apple Inc. All rights reserved.
> > 
> > You're living in the past!
> 
> Oops.

Fixed.

> 
> > 
> > > Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig:40
> > > +HEADER_SEARCH_PATHS = "${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore" $(HEADER_SEARCH_PATHS);
> > 
> > Why is it necessary to have DerivedSources/JavaScriptCore on the header search path? If it is truly necessary, BUILT_PRODUCTS_DIR should be referenced using $() rather than ${}.
> 
> It is necessary because JavaScriptCore.framework and llvmForJSC.dylib share the LLVMAPI.h, LLVMAPIFunctions.h, and LLVMHeaders.h files.
> 
> As for using $() versus ${}, I was just doing what the other xcconfig's do.

Changed to use $().  There are still a bunch of places where our xcconfig's use ${}; I'm not sure I understand what the difference is or which of those we should change.

> 
> > 
> > > Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig:42
> > > +PRODUCT_NAME = libllvmForJSC;
> > 
> > The product name should be just llvmForJSC. Settting EXECUTABLE_PREFIX set to "lib" will result in the binary having the correct name.
> 
> Interesting.  Xcode's default build settings used PRODUCT_NAME=libllvmForJSC, hence that's what I did here!  I can change to what you suggest, though.

Fixed.

> 
> > 
> > > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:4853
> > > +			shellScript = "if [[ $ENABLE_FTL_JIT != \"ENABLE_FTL_JIT\" ]]\nthen\n    exit 0\nfi\n\n# Copy the llvmForJSC library into the framework.\nditto \"${BUILT_PRODUCTS_DIR}/libllvmForJSC.dylib\" \"${BUILT_PRODUCTS_DIR}/JavaScriptCore.framework/Versions/A/Libraries/libllvmForJSC.dylib\"\nln -fs \"${BUILT_PRODUCTS_DIR}/JavaScriptCore.framework/Versions/A/Libraries\" \"${BUILT_PRODUCTS_DIR}/JavaScriptCore.framework/Libraries\"";
> > 
> > The symlink should point to Versions/Current rather than Versions/A.  The destination of the symlink should also be a relative path rather than an absolute one. You can also take advantage of Xcode configuration settings like CONTENTS_FOLDER_PATH and WRAPPER_NAME to avoid hard-coding JavaScriptCore.framework/Versions/A and JavaScriptCore.framework in the script.
> 
> OK, I'll investigate this.

Fixed.

> 
> > 
> > It may also be worth skipping the symlink step when SHALLOW_BUNDLE is equal to YES, since in that case the Versions hierarchy isn't used.
> 
> Hmmm, that's an iOS only thing, right?  Seems best to skip this until we are building on iOS and can actually test it.

I didn't do anything about this because I don't see how to test it right now.

> 
> > 
> > > Source/JavaScriptCore/llvm/InitializeLLVM.cpp:31
> > > +#include <wtf/Threading.h>
> > 
> > It doesn't look like you use anything from wtf/Threading.h.
> 
> pthread_once().

Kept this the same.

> 
> > 
> > > Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:34
> > > +#import <wtf/DataLog.h>
> > > +#import <wtf/StringPrintStream.h>
> > 
> > These don't seem necessary.
> 
> StringPrintStream gives you toCString(), which this uses.  But I'll follow your suggestion and use NSString's APIs.

Fixed.

> 
> > 
> > > Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:37
> > > +@interface JSCLLVMDummy : NSObject
> > 
> > I'd name this something like JavaScriptCoreBundleFinder to follow WebCore's lead.
> 
> OK.

Changed it to JSJavaScriptCoreBundleFinder.  There's some script that checks for the JS prefix in all ObjC classes in JSC.framework, so I decided to appease that script.

> 
> > 
> > > Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:41
> > > +@implementation JSCLLVMDummy {
> > > +}
> > 
> > These braces are not necessary.
> 
> Ok, I'll change it to:
> 
> @implementation Blah
> @end
> 
> For appropriate <Blah>.

Fixed.

> 
> > 
> > > Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:51
> > > +    NSBundle* myBundle = [NSBundle bundleForClass:[JSCLLVMDummy class]];
> > > +    NSString* path = [myBundle bundlePath];
> > > +    
> > > +    initializeLLVMPOSIX(toCString([path UTF8String], "/Libraries/libllvmForJSC.dylib").data());
> > 
> > Your *'s are in the wrong place for Objective-C types. It seems weird to bother with toCString here when you could just build up the entire path as an NSString:

Fixed.

> > 
> > NSString *bundle = [NSBundle bundleForClass:[JavaScriptCoreBundleFinder class]];
> > NSString *path = [[bundle bundlePath] stringByAppendingPathComponent:@"Libraries/libllvmForJSC.dylib"];
> > 
> > initializeLLVMPOSIX([path UTF8String]);
> 
> Groovy, I'll do that.

Fixed.

> 
> > 
> > > Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp:44
> > > +    InitializerFunction initializer = bitwise_cast<InitializerFunction>(
> > > +        dlsym(library, "initializeAndGetJSCLLVMAPI"));
> > 
> > I'm not sure it's necessary to wrap things like this. I think you've been staring at LLVM code for too long :)
> 
> I always wrap things because that's just how I roll.

Kept it as is.

> 
> > 
> > > Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:31
> > > +#define FOR_EACH_LLVM_API_FUNCTION(macro) \
> > 
> > There are a lot of style issues pointed out by the style checker that it'd be good to address.
> 
> :-/
> 
> This part was auto-generated.  I'll address most of the issues but I'm not sure to what extent it's worthwhile to be strict about this.  For example, LLVM uses a different parameter naming convention than we do and for this part it makes sense to just follow what the LLVM headers actually did rather than forcing WebKit convention.

Took care of obvious style faux pas but that list continues to use LLVM style conventions for these LLVM signatures.  I think it's the best way to go.

> 
> > 
> > > Source/JavaScriptCore/llvm/LLVMHeaders.h:37
> > > +#undef __STDC_LIMIT_MACROS
> > > +#undef __STDC_CONSTANT_MACROS
> > > +#define __STDC_LIMIT_MACROS
> > > +#define __STDC_CONSTANT_MACROS
> > 
> > Should we be resetting these macros to their original state at the end of the header?
> 
> That's not a bad idea.  OTOH, these macros just mean that you get more C-isms; we don't use those C-isms (like INT64_C(...)) but having them defined doesn't actually hurt.  OTOH, I wouldn't mind having those C-isms available in WebKit; I've definitely written some terrible code in the past just to get the equivalent of the C limit macros.

Put some #undef's in there.  Also removed the #undef's at the top since it would be a bug for WebKit to have already defined those macros.

[Filip Pizlo]

Created attachment 213842 [details]
the patch

[Filip Pizlo]

Created attachment 213843 [details]
the patch

Fix more style.

[Filip Pizlo]

Comment on attachment 213843 [details]
the patch

Never mind, this still needs some love.

[WebKit Commit Bot]

Attachment 213843 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig', u'Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/config.h', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/disassembler/LLVMDisassembler.cpp', u'Source/JavaScriptCore/ftl/FTLAbbreviatedTypes.h', u'Source/JavaScriptCore/ftl/FTLAbbreviations.h', u'Source/JavaScriptCore/ftl/FTLCompile.cpp', u'Source/JavaScriptCore/ftl/FTLFail.cpp', u'Source/JavaScriptCore/ftl/FTLJITCode.h', u'Source/JavaScriptCore/ftl/FTLJITFinalizer.h', u'Source/JavaScriptCore/ftl/FTLLink.cpp', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLState.cpp', u'Source/JavaScriptCore/ftl/WebKitLLVMLibraryAnchor.cpp', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVM.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVM.h', u'Source/JavaScriptCore/llvm/InitializeLLVMMac.mm', u'Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.h', u'Source/JavaScriptCore/llvm/LLVMAPI.cpp', u'Source/JavaScriptCore/llvm/LLVMAPI.h', u'Source/JavaScriptCore/llvm/LLVMAPIFunctions.h', u'Source/JavaScriptCore/llvm/LLVMHeaders.h', u'Source/JavaScriptCore/llvm/library/LLVMAnchor.cpp', u'Source/JavaScriptCore/llvm/library/LLVMExports.cpp', u'Source/JavaScriptCore/llvm/library/LLVMOverrides.cpp', u'Source/JavaScriptCore/llvm/library/config_llvm.h', u'Source/JavaScriptCore/runtime/InitializeThreading.cpp', u'Source/JavaScriptCore/runtime/Options.h', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wtf/LLVMHeaders.h', u'Source/WTF/wtf/text/CString.h']" exit_code: 1
Source/JavaScriptCore/llvm/LLVMAPI.h:36:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/library/config_llvm.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:491:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #7)
> (From update of attachment 213834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213834&action=review
> 
> > Source/JavaScriptCore/jsc.cpp:849
> > -    RefPtr<VM> vm = VM::create(LargeHeap);
> > +    VM* vm = VM::create(LargeHeap).leakRef();
> 
> Do we really have to leak the vm?

That's what the jsc shell used to do before I added a hack to not leak it.  I prefer for the jsc shell to leak the VM since that's what the browser - and probably most JSC clients - will do.

> 
> >> Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:125
> >> +    macro( LLVMValueRef , IsABasicBlock, (LLVMValueRef Val)) \
> > 
> > Extra space after ( in function call  [whitespace/parens] [4]
> 
> Please remove these extra spaces, also change " , " to ", "
> 
> > Source/JavaScriptCore/llvm/library/LLVMOverrides.cpp:33
> > +extern "C" int __cxa_atexit() { return 0; }
> 
> mmmm, tasty

[Filip Pizlo]

Created attachment 213844 [details]
the patch

OK, now I think it's ready.

[WebKit Commit Bot]

Attachment 213844 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig', u'Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/disassembler/LLVMDisassembler.cpp', u'Source/JavaScriptCore/ftl/FTLAbbreviatedTypes.h', u'Source/JavaScriptCore/ftl/FTLAbbreviations.h', u'Source/JavaScriptCore/ftl/FTLCompile.cpp', u'Source/JavaScriptCore/ftl/FTLFail.cpp', u'Source/JavaScriptCore/ftl/FTLJITCode.h', u'Source/JavaScriptCore/ftl/FTLJITFinalizer.h', u'Source/JavaScriptCore/ftl/FTLLink.cpp', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLState.cpp', u'Source/JavaScriptCore/ftl/WebKitLLVMLibraryAnchor.cpp', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVM.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVM.h', u'Source/JavaScriptCore/llvm/InitializeLLVMMac.mm', u'Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.h', u'Source/JavaScriptCore/llvm/LLVMAPI.cpp', u'Source/JavaScriptCore/llvm/LLVMAPI.h', u'Source/JavaScriptCore/llvm/LLVMAPIFunctions.h', u'Source/JavaScriptCore/llvm/LLVMHeaders.h', u'Source/JavaScriptCore/llvm/library/LLVMAnchor.cpp', u'Source/JavaScriptCore/llvm/library/LLVMExports.cpp', u'Source/JavaScriptCore/llvm/library/LLVMOverrides.cpp', u'Source/JavaScriptCore/llvm/library/config_llvm.h', u'Source/JavaScriptCore/runtime/InitializeThreading.cpp', u'Source/JavaScriptCore/runtime/Options.h', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wtf/LLVMHeaders.h', u'Source/WTF/wtf/text/CString.h']" exit_code: 1
Source/JavaScriptCore/llvm/LLVMAPI.h:40:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/library/config_llvm.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 213845 [details]
the patch

Fixed a goof in my use of ln -fs.  We were getting a Libraries/Libraries symlink.  This patch won't create a symlink if Libraries already exists.

[WebKit Commit Bot]

Attachment 213845 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig', u'Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/disassembler/LLVMDisassembler.cpp', u'Source/JavaScriptCore/ftl/FTLAbbreviatedTypes.h', u'Source/JavaScriptCore/ftl/FTLAbbreviations.h', u'Source/JavaScriptCore/ftl/FTLCompile.cpp', u'Source/JavaScriptCore/ftl/FTLFail.cpp', u'Source/JavaScriptCore/ftl/FTLJITCode.h', u'Source/JavaScriptCore/ftl/FTLJITFinalizer.h', u'Source/JavaScriptCore/ftl/FTLLink.cpp', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLState.cpp', u'Source/JavaScriptCore/ftl/WebKitLLVMLibraryAnchor.cpp', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVM.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVM.h', u'Source/JavaScriptCore/llvm/InitializeLLVMMac.mm', u'Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.h', u'Source/JavaScriptCore/llvm/LLVMAPI.cpp', u'Source/JavaScriptCore/llvm/LLVMAPI.h', u'Source/JavaScriptCore/llvm/LLVMAPIFunctions.h', u'Source/JavaScriptCore/llvm/LLVMHeaders.h', u'Source/JavaScriptCore/llvm/library/LLVMAnchor.cpp', u'Source/JavaScriptCore/llvm/library/LLVMExports.cpp', u'Source/JavaScriptCore/llvm/library/LLVMOverrides.cpp', u'Source/JavaScriptCore/llvm/library/config_llvm.h', u'Source/JavaScriptCore/runtime/InitializeThreading.cpp', u'Source/JavaScriptCore/runtime/Options.h', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wtf/LLVMHeaders.h', u'Source/WTF/wtf/text/CString.h']" exit_code: 1
Source/JavaScriptCore/llvm/LLVMAPI.h:40:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/library/config_llvm.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 213845 [details]
the patch

Attachment 213845 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3580046

[Filip Pizlo]

Created attachment 213887 [details]
the patch

Hopefully this breaks less things.

[Nadav Rotem]

Looks good to me.

[Mark Rowe (bdash)]

(In reply to comment #12)
> This patch fixes exit-time destructors and static initializers in LLVM by doing soft linking.  Hence we can get rid of the hack in jsc.

Ah, cool!

> As for using $() versus ${}, I was just doing what the other xcconfig's do.

It looks like this mistake crept in to one place in one .xcconfig file long ago and has since spread to others. The fact that ${} works at all isn't something that's documented AFAICT.

> Hmmm, that's an iOS only thing, right?  Seems best to skip this until we are building on iOS and can actually test it.

Ok, sounds reasonable.

> > > Source/JavaScriptCore/llvm/InitializeLLVM.cpp:31
> > > +#include <wtf/Threading.h>
> > 
> > It doesn't look like you use anything from wtf/Threading.h.
> 
> pthread_once().

That comes from pthread.h. You should just include it directly.

> > 
> > > Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:31
> > > +#define FOR_EACH_LLVM_API_FUNCTION(macro) \
> > 
> > There are a lot of style issues pointed out by the style checker that it'd be good to address.
> 
> :-/
> 
> This part was auto-generated.  I'll address most of the issues but I'm not sure to what extent it's worthwhile to be strict about this.  For example, LLVM uses a different parameter naming convention than we do and for this part it makes sense to just follow what the LLVM headers actually did rather than forcing WebKit convention.

I'm not too concerned about whether you address the naming issues, since as you mention it makes sense for this to be consistent with LLVM. A lot of warnings were about inconsistent whitespace. I think you've fixed that in your latest patch though.

[Mark Rowe (bdash)]

Comment on attachment 213887 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213887&action=review

> Source/JavaScriptCore/llvm/InitializeLLVM.cpp:31
> +#include <wtf/Threading.h>

This should be #include <pthread.h>

[Filip Pizlo]

(In reply to comment #27)
> (From update of attachment 213887 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213887&action=review
> 
> > Source/JavaScriptCore/llvm/InitializeLLVM.cpp:31
> > +#include <wtf/Threading.h>
> 
> This should be #include <pthread.h>

Yup, I'll fix.  Thanks for the reviews!

[Filip Pizlo]

Created attachment 213912 [details]
patch for ews

Should be all done, but I want to see what the bots say.

[WebKit Commit Bot]

Attachment 213912 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Configurations/JavaScriptCore.xcconfig', u'Source/JavaScriptCore/Configurations/LLVMForJSC.xcconfig', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/dfg/DFGPlan.cpp', u'Source/JavaScriptCore/disassembler/LLVMDisassembler.cpp', u'Source/JavaScriptCore/ftl/FTLAbbreviatedTypes.h', u'Source/JavaScriptCore/ftl/FTLAbbreviations.h', u'Source/JavaScriptCore/ftl/FTLCompile.cpp', u'Source/JavaScriptCore/ftl/FTLFail.cpp', u'Source/JavaScriptCore/ftl/FTLJITCode.h', u'Source/JavaScriptCore/ftl/FTLJITFinalizer.h', u'Source/JavaScriptCore/ftl/FTLLink.cpp', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.cpp', u'Source/JavaScriptCore/ftl/FTLOutput.h', u'Source/JavaScriptCore/ftl/FTLState.cpp', u'Source/JavaScriptCore/ftl/WebKitLLVMLibraryAnchor.cpp', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVM.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVM.h', u'Source/JavaScriptCore/llvm/InitializeLLVMMac.mm', u'Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp', u'Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.h', u'Source/JavaScriptCore/llvm/LLVMAPI.cpp', u'Source/JavaScriptCore/llvm/LLVMAPI.h', u'Source/JavaScriptCore/llvm/LLVMAPIFunctions.h', u'Source/JavaScriptCore/llvm/LLVMHeaders.h', u'Source/JavaScriptCore/llvm/library/LLVMAnchor.cpp', u'Source/JavaScriptCore/llvm/library/LLVMExports.cpp', u'Source/JavaScriptCore/llvm/library/LLVMOverrides.cpp', u'Source/JavaScriptCore/llvm/library/config_llvm.h', u'Source/JavaScriptCore/runtime/InitializeThreading.cpp', u'Source/JavaScriptCore/runtime/Options.h', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wtf/LLVMHeaders.h', u'Source/WTF/wtf/text/CString.h', u'Tools/ChangeLog', u'Tools/Scripts/configure-llvm']" exit_code: 1
Source/JavaScriptCore/llvm/LLVMAPI.h:40:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/llvm/library/config_llvm.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/157260