[webkit-reviews] review denied: [Bug 122566] FTL: Soft-link LLVM as a workaround for LLVM's static initializers and exit-time destructors : [Attachment 213834] the patch

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied	review:
Bug 122566: FTL: Soft-link LLVM as a workaround for LLVM's static initializers
and exit-time destructors
https://bugs.webkit.org/show_bug.cgi?id=122566

Attachment 213834: the patch
https://bugs.webkit.org/attachment.cgi?id=213834&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
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/libllvmFo
rJSC.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
> + at interface JSCLLVMDummy : NSObject

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

> Source/JavaScriptCore/llvm/InitializeLLVMMac.mm:41
> + at 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?