Make it possible to implement JS builtins in JS
Created attachment 222627 [details] Patch
Waiting for all the bots to turn as this will no doubt hose the lot while i get the various build systems going again. Help would be appreciated for anyone not using DerivedSources.make
Created attachment 222669 [details] Patch
Let me check the EFL build. ;)
(In reply to comment #4) > Let me check the EFL build. ;) I have probably got the CMake build commands completely wrong. Would <3 cake calling Derivedsources.make
Created attachment 222670 [details] Patch
Comment on attachment 222669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222669&action=review I still have build failures, but I'm on fixing them. > Source/JavaScriptCore/CMakeLists.txt:820 > + # JSCBuiltins Please don't indent this. > Source/JavaScriptCore/CMakeLists.txt:825 > + COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/generate-js-builtins JSCBuiltins.h JSCBuiltins.cpp JSCBuiltins.h -> ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.h JSCBuiltins.cpp -> ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.cpp > Source/JavaScriptCore/CMakeLists.txt:830 > list(APPEND JavaScriptCore_SOURCES > ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorJSBackendDispatchers.cpp > ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InspectorJSFrontendDispatchers.cpp Add ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.cpp here and ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.h to JavaScriptCore_HEADERS
Created attachment 222671 [details] Patch
Comment on attachment 222671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222671&action=review I still get same build failures as the EFL EWS notices, but still debugging. > Source/JavaScriptCore/CMakeLists.txt:827 > +# JSCBuiltins > +add_custom_command( > + OUTPUT ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.cpp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.h > + MAIN_DEPENDENCY ${CMAKE_CURRENT_SOURCE_DIR}/generate-js-builtins > + DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/builtins/*.js > + COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/generate-js-builtins ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.h ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.cpp > + VERBATIM) > + I played a little bit with it, but it still needs fine tuning, because the input wasn't passed to the generator command. The following code generated the cpp and the h for me: # JSCBuiltins file(GLOB JSCBuiltins_js_files "${CMAKE_CURRENT_SOURCE_DIR}/builtins/*.js") add_custom_command( OUTPUT ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.cpp ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.h MAIN_DEPENDENCY ${CMAKE_CURRENT_SOURCE_DIR}/generate-js-builtins DEPENDS ${JSCBuiltins_js_files} COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/generate-js-builtins ${JSCBuiltins_js_files} ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.h ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/JSCBuiltins.cpp VERBATIM)
Created attachment 222674 [details] Patch more efl fixing
Created attachment 222675 [details] Patch more efl fixing
(In reply to comment #8) > Created an attachment (id=222671) [details] > Patch I managed to reach linking (but still fail) with it and the fix I suggested in comment #9 with some more fixes: - added #include "Operations.h" back to Source/JavaScriptCore/bytecode/CodeBlock.h - added #include "JSFunctionInlines.h" to Source/JavaScriptCore/runtime/JSCellInlines.h - added "${JAVASCRIPTCORE_DIR}/builtins" to set(JavaScriptCore_INCLUDE_DIRECTORIES in Source/JavaScriptCore/CMakeLists.txt
I got the following: Linking CXX shared library ../../lib/libjavascriptcore_efl.so CMakeFiles/JavaScriptCore.dir/runtime/JSPromiseConstructor.cpp.o: In function `JSC::JSPromiseConstructorFuncAll(JSC::ExecState*)': JSPromiseConstructor.cpp:(.text._ZN3JSCL27JSPromiseConstructorFuncAllEPNS_9ExecStateE+0x315): undefined reference to `JSC::JSString::getStringPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)' JSPromiseConstructor.cpp:(.text._ZN3JSCL27JSPromiseConstructorFuncAllEPNS_9ExecStateE+0x86d): undefined reference to `JSC::JSString::getStringPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)' JSPromiseConstructor.cpp:(.text._ZN3JSCL27JSPromiseConstructorFuncAllEPNS_9ExecStateE+0xa4d): undefined reference to `JSC::JSString::getStringPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)' JSPromiseConstructor.cpp:(.text._ZN3JSCL27JSPromiseConstructorFuncAllEPNS_9ExecStateE+0x1282): undefined reference to `JSC::JSString::getStringPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)' CMakeFiles/JavaScriptCore.dir/runtime/JSPromiseConstructor.cpp.o: In function `JSC::JSPromiseConstructorFuncRace(JSC::ExecState*)': JSPromiseConstructor.cpp:(.text._ZN3JSCL28JSPromiseConstructorFuncRaceEPNS_9ExecStateE+0x2da): undefined reference to `JSC::JSString::getStringPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)' CMakeFiles/JavaScriptCore.dir/runtime/JSPromiseConstructor.cpp.o:JSPromiseConstructor.cpp:(.text._ZN3JSCL28JSPromiseConstructorFuncRaceEPNS_9ExecStateE+0x637): more undefined references to `JSC::JSString::getStringPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)' follow CMakeFiles/JavaScriptCore.dir/runtime/RegExpConstructor.cpp.o: In function `JSC::RegExpConstructor::getBackref(JSC::ExecState*, unsigned int)': RegExpConstructor.cpp:(.text._ZN3JSC17RegExpConstructor10getBackrefEPNS_9ExecStateEj+0x131): undefined reference to `JSC::JSString::getStringPropertySlot(JSC::ExecState*, unsigned int, JSC::PropertySlot&)' CMakeFiles/JavaScriptCore.dir/runtime/RegExpConstructor.cpp.o: In function `JSC::RegExpConstructor::getLastParen(JSC::ExecState*)': RegExpConstructor.cpp:(.text._ZN3JSC17RegExpConstructor12getLastParenEPNS_9ExecStateE+0x131): undefined reference to `JSC::JSString::getStringPropertySlot(JSC::ExecState*, unsigned int, JSC::PropertySlot&)' collect2: error: ld returned 1 exit status Any idea?
EFL really needs to fix the file mime type, and not strip the build failures
Created attachment 222678 [details] Patch more efl fixing
Created attachment 222679 [details] Patch more efl fixing
Created attachment 222684 [details] Patch
Created attachment 222685 [details] Patch
Windows is failing to build because it can't find the new BuiltinExecutables.h file: >C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\JavaScriptCore\DerivedSources\JSCBuiltins.cpp(34): fatal error C1083: Cannot open include file: 'BuiltinExecutables.h': No such file or directory We control the include search path in our Visual Studio property sheets. You need to add "..\bindings\;" to the <AdditionalIncludeDirectories> in Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCoreCommon.props That should fix the build.
Created attachment 222686 [details] Patch
Comment on attachment 222686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222686&action=review > Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCoreCommon.props:8 > + <AdditionalIncludeDirectories>..\;..\tools\;..\runtime\;..\llint\;..\jit\;..\disassembler\;..\heap\;..\debugger\;..\assembler\;..\profiler\;..\yarr\;..\interpreter\;..\bytecode\;..\builtins\;..\dfg\;..\bytecompiler\;..\parser\;..\API\;..\ftl\;..\bindings\;..\inspector\;$(ConfigurationBuildDir)\obj$(PlatformArchitecture)\JavaScriptCore\DerivedSources\;$(ConfigurationBuildDir)\include\;$(ConfigurationBuildDir)\include\JavaScriptCore\;$(ConfigurationBuildDir)\include\private\;$(WebKit_Libraries)\include;$(WebKit_Libraries)\include\private;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> Yay!
I'm seeing a few errors that look like: EFL and GTK fail because of: make: *** No rule to make target `DerivedSources/JavaScriptCore/JSCBuiltins.cpp', needed by `DerivedSources/JavaScriptCore/LLIntDesiredOffsets.h'. Stop. Windows: make: *** No rule to make target `/home/buildbot/WebKit/Source/JavaScriptCore/JSCBuiltins.h', needed by `JSCBuiltins'. Stop.
Comment on attachment 222686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222686&action=review > Source/JavaScriptCore/DerivedSources.make:86 > +JSCBuiltins: $(JavaScriptCore)/JSCBuiltins.h $(JavaScriptCore)/JSCBuiltins.cpp I think this needs to be the build target; It thinks it should be finding "Source/JavaScriptCore/JSCBuiltins.{h,cpp}", but they are actually build in "${WebKit_Output}/${Configuration}/DerivedSources/JavaScriptCore/" (or something like that).
Created attachment 222694 [details] Patch Fix derived sources makefile properly
Created attachment 222704 [details] Patch hopefully get all the includes right
Created attachment 222722 [details] Patch
Created attachment 222744 [details] Patch
Created attachment 222750 [details] Patch
Created attachment 222757 [details] Patch Lets just try and make the compiler stop complaining about this warning
Created attachment 222762 [details] Patch Lets not include random characters
Comment on attachment 222762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222762&action=review r=me. Make sure all EWS go green! > Source/JavaScriptCore/ChangeLog:15 > + without breaking the offset extractor. This result of this refactoring "This result" => "The result" > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:549 > + Eliminate whitespace > Source/JavaScriptCore/generate-js-builtins:2 > +# Copyright (C) 2013 Apple Inc. All rights reserved. Update year. > Source/JavaScriptCore/runtime/CommonIdentifiers.h:137 > + macro(promise) \ Shouldn't this be in a different patch? > Source/JavaScriptCore/runtime/CommonIdentifiers.h:140 > + macro(resolve) \ Ditto > Source/JavaScriptCore/runtime/CommonIdentifiers.h:168 > + macro(writable) \ > + macro(iterator) \ > + macro(iteratorNext) \ > + macro(reject) \ > + macro(fulfillmentHandler) \ > + macro(rejectionHandler) \ > + macro(deferred) \ > + macro(countdownHolder) \ Ditto > Source/JavaScriptCore/runtime/JSStringInlines.h:2 > + * Copyright (C) 2012, 2013 Apple Inc. All rights reserved. Update copyright year.
Created attachment 222771 [details] Patch
> > Source/JavaScriptCore/runtime/JSStringInlines.h:2 > > + * Copyright (C) 2012, 2013 Apple Inc. All rights reserved. > > Update copyright year. No new code here, this is code motion, so i'm keeping the existing copyright dates.
Created attachment 222774 [details] Patch
Created attachment 222777 [details] Patch
Created attachment 222784 [details] Patch
Created attachment 222785 [details] Patch i can haz fix windows\?
Trying to build it on EFL, it gives this: JSFunction.h:102:21: error: inline function 'bool JSC::JSFunction::isHostFunction() const' used but never defined [-Werror] I removed the inlne keyword from there and the build went on, breaking on WK2, due to not finding JSCBuiltins.h, which was solved by adding "${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}" to the WebKit2_INCLUDE_DIRECTORIES list of WebKit2/CMakeLists.txt
The GTK port builds with an additional JSCJSValueInlines.h inclusion added in Source/JavaScriptCore/dfg/DFGJITCode.cpp, but there are WebProcess crashes when running layout tests.
Created attachment 222816 [details] Patch and now to not break literally every build
(In reply to comment #38) > Trying to build it on EFL, it gives this: > JSFunction.h:102:21: error: inline function 'bool JSC::JSFunction::isHostFunction() const' used but never defined [-Werror] > > I removed the inlne keyword from there and the build went on, breaking on WK2, due to not finding JSCBuiltins.h, which was solved by adding "${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}" to the WebKit2_INCLUDE_DIRECTORIES list of WebKit2/CMakeLists.txt Yeah, i realised that the inline keyword there was causing the inline foo warning. I may update the style bot to disallow inline directives inside class/struct definitions
Created attachment 222817 [details] Patch
Created attachment 222819 [details] Patch
Created attachment 222820 [details] Patch i hate this
Created attachment 222824 [details] Patch lets see if efl and gtk builds now
Created attachment 222828 [details] Patch now lets try efl
Created attachment 222832 [details] Patch now lets try windows
Created attachment 222833 [details] Patch dammit efl build
Created attachment 222834 [details] Patch
Comment on attachment 222834 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222834&action=review > Tools/DumpRenderTree/CMakeLists.txt:61 > + "{DERIVED_SOURCES_JAVASCRIPTCORE_DIR} ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR} Hopefully the last thing needed :)
Created attachment 222835 [details] Patch
Created attachment 222838 [details] Patch Rebase to convince windows to apply the patch properly
Committed r163195: <http://trac.webkit.org/changeset/163195>
Reopening to attach new patch.
Created attachment 223913 [details] Patch
Attachment 223913 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/CommonIdentifiers.h:229: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 69 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 223919 [details] fix release builds
Attachment 223919 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/CommonIdentifiers.h:229: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 69 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 223919 [details] fix release builds View in context: https://bugs.webkit.org/attachment.cgi?id=223919&action=review r=me. Get the EWS bots green before checkin. > Source/JavaScriptCore/builtins/Array.prototype.js:46 > + if (!(i in array)) > + continue; > + if (!callback.@call(thisArg, array[i], i, array)) > + return false; Is there any concerns that the array will be mutated by the callback? Is the "i in array" check considered enough? > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:548 > + Extra space. > Source/JavaScriptCore/generate-js-builtins:89 > + > + > + One one blank line between function definitions. > Source/JavaScriptCore/generate-js-builtins:127 > + > + One one blank line between function definitions. > Source/JavaScriptCore/parser/Lexer.cpp:761 > +bool isSafeIdentifier(VM& vm, const Identifier* ident) This should be called isSafeBuiltinIdentifier(). > Source/JavaScriptCore/runtime/CommonIdentifiers.h:237 > + Extra space > Source/JavaScriptCore/runtime/PropertySlot.h:47 > + BuiltinOrFunction = Builtin | Function, // helper only used by static hashtables Indent the '=' the same as most other Attribute values (e.g. Accessor).
(In reply to comment #59) > (From update of attachment 223919 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223919&action=review > > r=me. Get the EWS bots green before checkin. > > > Source/JavaScriptCore/builtins/Array.prototype.js:46 > > + if (!(i in array)) > > + continue; > > + if (!callback.@call(thisArg, array[i], i, array)) > > + return false; > > Is there any concerns that the array will be mutated by the callback? Is the "i in array" check considered enough? This is the spec behaviour, and happily this is in JS so it's all memory safe :) > > > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:548 > > + > > Extra space. > > > Source/JavaScriptCore/generate-js-builtins:89 > > + > > + > > + > > One one blank line between function definitions. > > > Source/JavaScriptCore/generate-js-builtins:127 > > + > > + > > One one blank line between function definitions. > > > Source/JavaScriptCore/parser/Lexer.cpp:761 > > +bool isSafeIdentifier(VM& vm, const Identifier* ident) > > This should be called isSafeBuiltinIdentifier(). > > > Source/JavaScriptCore/runtime/CommonIdentifiers.h:237 > > + > > Extra space > > > Source/JavaScriptCore/runtime/PropertySlot.h:47 > > + BuiltinOrFunction = Builtin | Function, // helper only used by static hashtables > > Indent the '=' the same as most other Attribute values (e.g. Accessor).
Windows build fix attempted in http://trac.webkit.org/changeset/163967.
(In reply to comment #61) > Windows build fix attempted in http://trac.webkit.org/changeset/163967. That worked.