Bug 133571 – [JavaScriptCore] Fix FTL on platform EFL.

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

[László Langó]

We can enable FTL on EFL port. There are no compact_unwind sections on Linux systems so FTL crashes. We have to parse eh_frame in FTLUnwindInfo instead of compact_unwind and get the information for stack unwinding from there.

[László Langó]

Created attachment 232611 [details]
Patch

[Darin Adler]

Comment on attachment 232611 [details]
Patch

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

I didn’t review the whole patch, but I looked it over briefly and spotted a few things.

> Source/JavaScriptCore/ftl/FTLJITCode.h:40
> +#if PLATFORM(COCOA)

I don’t think PLATFORM(COCOA) is right here. OS(DARWIN) perhaps?

> Source/JavaScriptCore/ftl/FTLState.cpp:48
> +    , ehFrame(0)

Please use nullptr here.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:247
> +/* Read a ULEB128 into a 64-bit word.   */

Comment does not seem helpful. Says the same thing as the function name.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:270
> +/* Read a SLEB128 into a 64-bit word.  */

Comment does not seem helpful. Says the same thing as the function name.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:488
> +//
> +// Scan an eh_frame section to find an FDE for a pc
> +//

Comment does not seem useful. If we can rename the function to be clearer, that might help.

FDE_Info and CIE_Info arguments should be references rather than pointers.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:559
> +//    RELEASE_ASSERT(reg <= MaxRegisterNumber); // reg too big

Why is this commented out? Please either omit it or include it, but don’t include it commented out.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:579
> +static void parseInstructions(intPtr instructions, intPtr instructionsEnd, const CIE_Info& cieInfo, PrologInfo* results)

I suggest a reference rather than pointer for the result argument.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:583
> +    // see Dwarf Spec, section 6.4.2 for details on unwind opcodes

WebKit comment style is to capitalize and use a period.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:647
> +//
> +// Parse the dwarf instructions and create the abstact PrologInfo for an FDE
> +//
> +static void parseFDEInstructions(const FDE_Info& fdeInfo, const CIE_Info& cieInfo, PrologInfo* results)

This comment formatting is not typical for our project. Also seems that the out argument should be a reference rather than a pointer.

Also, I would suggest giving the function a clear enough name that you don’t need a comment.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:649
> +    // clear results

This comment should be removed. Says the exact same thing the code below does.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:652
> +    // parse CIE then FDE instructions

This comment should be removed. Says the exact same thing the code below does.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.h:42
> +    bool parse(void*, size_t, void*, size_t, GeneratedFunction);

Unclear what these arguments mean. One void* plus size_t is pretty clear. But two pairs makes this unclear.

[Filip Pizlo]

Comment on attachment 232611 [details]
Patch

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

You appear to have lifted this code, without attribution, from https://llvm.org/svn/llvm-project/libcxxabi/trunk/src/Unwind/AddressSpace.hpp.  That's not cool, man.  In particular, the LLVM license clearly states that LLVM's copyright notice must be preserved, and you have failed to do that.

> Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:234
> +typedef uintptr_t intPtr;

Why?

[Filip Pizlo]

(In reply to comment #3)
> (From update of attachment 232611 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232611&action=review
> 
> You appear to have lifted this code, without attribution, from https://llvm.org/svn/llvm-project/libcxxabi/trunk/src/Unwind/AddressSpace.hpp.  That's not cool, man.  In particular, the LLVM license clearly states that LLVM's copyright notice must be preserved, and you have failed to do that.

That file is dual-licensed under MIT as well, but that license also requires preservation of the copyright notice.

I suspect that the best way to proceed is to carefully update your patch so that:

- The code lifted from LLVM is in a separate file, so that it can retain the original LLVM style, which will make it easier to pull upstream LLVM changes in the future.

- LLVM's copyright notice is preserved.

> 
> > Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:234
> > +typedef uintptr_t intPtr;
> 
> Why?

[László Langó]

Created attachment 233798 [details]
Patch

[László Langó]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 232611 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=232611&action=review
> > 
> > You appear to have lifted this code, without attribution, from https://llvm.org/svn/llvm-project/libcxxabi/trunk/src/Unwind/AddressSpace.hpp.  That's not cool, man.  In particular, the LLVM license clearly states that LLVM's copyright notice must be preserved, and you have failed to do that.
> 
> That file is dual-licensed under MIT as well, but that license also requires preservation of the copyright notice.
> 
> I suspect that the best way to proceed is to carefully update your patch so that:
> 
> - The code lifted from LLVM is in a separate file, so that it can retain the original LLVM style, which will make it easier to pull upstream LLVM changes in the future.
> 
> - LLVM's copyright notice is preserved.
> 
> > 
> > > Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:234
> > > +typedef uintptr_t intPtr;
> > 
> > Why?

Sorry for the late update. Thanks for the review. I updated the path based on your suggestions.

[Build Bot]

Comment on attachment 233798 [details]
Patch

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

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 233802 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[László Langó]

Can someone review this? This is quite important patch for me.

[Filip Pizlo]

Comment on attachment 233798 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLEhFrameParser.h:21
> +/*
> +* University of Illinois/NCSA
> +* Open Source License
> +*
> +* Copyright (c) 2009-2014 by the contributors of LLVM/libc++abi project.
> +*
> +* All rights reserved.
> +*
> +* Developed by:
> +*
> +*    LLVM Team
> +*
> +*    University of Illinois at Urbana-Champaign
> +*
> +*    http://llvm.org
> +*
> +* Permission is hereby granted, free of charge, to any person obtaining a copy of
> +* this software and associated documentation files (the "Software"), to deal with
> +* the Software without restriction, including without limitation the rights to
> +* use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
> +* of the Software, and to permit persons to whom the Software is furnished to do

This should not be a header file.  It's very strange to have headers that define functions in the global namespace with the intention of only including them in one .cpp file.

I would find a way to factor this out into a .cpp file, with a declared API in a corresponding .h file.

[Yong Li]

Anything forgot to include to the patch?

I tried the patch, and re-ran update-webkitefl-libs, but got a lot of errors like the following when building jsc (build-jsc --efl --ftl-jit)

/home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp: In function ‘JSC::LLVMAPI* initializeAndGetJSCLLVMAPI(void (*)(const char*, ...))’:
/home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp:102:20: error: ‘LLVMBuildFence’ was not declared in this scope
     result->name = LLVM##name;
                    ^
/home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:500:5: note: in expansion of macro ‘LLVM_API_FUNCTION_ASSIGNMENT’
     macro(LLVMValueRef, BuildFence, (LLVMBuilderRef B, LLVMAtomicOrdering Ordering, LLVMBool isSingleThread, const char *Name)) \
     ^

Any idea?

[Filip Pizlo]

(In reply to comment #11)
> Anything forgot to include to the patch?
> 
> I tried the patch, and re-ran update-webkitefl-libs, but got a lot of errors like the following when building jsc (build-jsc --efl --ftl-jit)
> 
> /home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp: In function ‘JSC::LLVMAPI* initializeAndGetJSCLLVMAPI(void (*)(const char*, ...))’:
> /home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp:102:20: error: ‘LLVMBuildFence’ was not declared in this scope
>      result->name = LLVM##name;
>                     ^
> /home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:500:5: note: in expansion of macro ‘LLVM_API_FUNCTION_ASSIGNMENT’
>      macro(LLVMValueRef, BuildFence, (LLVMBuilderRef B, LLVMAtomicOrdering Ordering, LLVMBool isSingleThread, const char *Name)) \
>      ^
> 
> Any idea?

You need a newer LLVM.

[Csaba Osztrogonác]

(In reply to comment #11)
> Anything forgot to include to the patch?
> 
> I tried the patch, and re-ran update-webkitefl-libs, but got a lot of errors like the following when building jsc (build-jsc --efl --ftl-jit)
> 
> /home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp: In function ‘JSC::LLVMAPI* initializeAndGetJSCLLVMAPI(void (*)(const char*, ...))’:
> /home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp:102:20: error: ‘LLVMBuildFence’ was not declared in this scope
>      result->name = LLVM##name;
>                     ^
> /home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:500:5: note: in expansion of macro ‘LLVM_API_FUNCTION_ASSIGNMENT’
>      macro(LLVMValueRef, BuildFence, (LLVMBuilderRef B, LLVMAtomicOrdering Ordering, LLVMBool isSingleThread, const char *Name)) \
>      ^
> 
> Any idea?

Have you set WEBKIT_EXTRA_MODULES=llvm environment variable? jhbuild 
doesn't build llvm by default, because FTL is still experimental. I
assume you haven't built llvm and cmake found your old system llvm.

[Yong Li]

(In reply to comment #13)
> (In reply to comment #11)
> > Anything forgot to include to the patch?
> > 
> > I tried the patch, and re-ran update-webkitefl-libs, but got a lot of errors like the following when building jsc (build-jsc --efl --ftl-jit)
> > 
> > /home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp: In function ‘JSC::LLVMAPI* initializeAndGetJSCLLVMAPI(void (*)(const char*, ...))’:
> > /home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/library/LLVMExports.cpp:102:20: error: ‘LLVMBuildFence’ was not declared in this scope
> >      result->name = LLVM##name;
> >                     ^
> > /home/newuser/dev/WebKit/Source/JavaScriptCore/llvm/LLVMAPIFunctions.h:500:5: note: in expansion of macro ‘LLVM_API_FUNCTION_ASSIGNMENT’
> >      macro(LLVMValueRef, BuildFence, (LLVMBuilderRef B, LLVMAtomicOrdering Ordering, LLVMBool isSingleThread, const char *Name)) \
> >      ^
> > 
> > Any idea?
> 
> Have you set WEBKIT_EXTRA_MODULES=llvm environment variable? jhbuild 
> doesn't build llvm by default, because FTL is still experimental. I
> assume you haven't built llvm and cmake found your old system llvm.

Thanks. You are right. I worked around it by manually checking out LLVM at the revision and building it with the flags.

[László Langó]

Created attachment 237301 [details]
Proposed patch based on previous comments and update to ToT.

[Filip Pizlo]

Comment on attachment 237301 [details]
Proposed patch based on previous comments and update to ToT.

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

> Source/JavaScriptCore/ftl/FTLState.h:80
> +    void* section;

It's wrong to change the name of this field to just "section". That could mean any kind if section. You're using it for the section that has unwind data. You should pick a name that reflects this.

[László Langó]

Created attachment 237339 [details]
Proposed patch

[Filip Pizlo]

Comment on attachment 237339 [details]
Proposed patch

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

Please fix the OS(DARWIN) issue before landing.

> Source/JavaScriptCore/ftl/FTLJITCode.h:40
> +#if PLATFORM(COCOA)

I think you mean OS(DARWIN)

[László Langó]

Created attachment 237940 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 237940 [details]
Patch for landing

Clearing flags on attachment: 237940

Committed r173509: <http://trac.webkit.org/changeset/173509>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.