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.
Created attachment 232611 [details] Patch
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.
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?
(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?
Created attachment 233798 [details] Patch
(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.
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
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
Can someone review this? This is quite important patch for me.
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.
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?
(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.
(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.
(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.
Created attachment 237301 [details] Proposed patch based on previous comments and update to ToT.
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.
Created attachment 237339 [details] Proposed patch
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)
Created attachment 237940 [details] Patch for landing
Comment on attachment 237940 [details] Patch for landing Clearing flags on attachment: 237940 Committed r173509: <http://trac.webkit.org/changeset/173509>
All reviewed patches have been landed. Closing bug.