Make it possible to implement JS builtins in JS

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Make it possible to implement JS builtins in JS

[Oliver Hunt]

Created attachment 222627 [details]
Patch

[Oliver Hunt]

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

[Oliver Hunt]

Created attachment 222669 [details]
Patch

[Csaba Osztrogonác]

Let me check the EFL build. ;)

[Oliver Hunt]

(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

[Oliver Hunt]

Created attachment 222670 [details]
Patch

[Csaba Osztrogonác]

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

[Oliver Hunt]

Created attachment 222671 [details]
Patch

[Csaba Osztrogonác]

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)

[Oliver Hunt]

Created attachment 222674 [details]
Patch

more efl fixing

[Oliver Hunt]

Created attachment 222675 [details]
Patch

more efl fixing

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

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?

[Oliver Hunt]

EFL really needs to fix the file mime type, and not strip the build failures

[Oliver Hunt]

Created attachment 222678 [details]
Patch

more efl fixing

[Oliver Hunt]

Created attachment 222679 [details]
Patch

more efl fixing

[Oliver Hunt]

Created attachment 222684 [details]
Patch

[Oliver Hunt]

Created attachment 222685 [details]
Patch

[Brent Fulgham]

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.

[Oliver Hunt]

Created attachment 222686 [details]
Patch

[Brent Fulgham]

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!

[Brent Fulgham]

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.

[Brent Fulgham]

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).

[Oliver Hunt]

Created attachment 222694 [details]
Patch

Fix derived sources makefile properly

[Oliver Hunt]

Created attachment 222704 [details]
Patch

hopefully get all the includes right

[Oliver Hunt]

Created attachment 222722 [details]
Patch

[Oliver Hunt]

Created attachment 222744 [details]
Patch

[Oliver Hunt]

Created attachment 222750 [details]
Patch

[Oliver Hunt]

Created attachment 222757 [details]
Patch

Lets just try and make the compiler stop complaining about this warning

[Oliver Hunt]

Created attachment 222762 [details]
Patch

Lets not include random characters

[Michael Saboff]

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.

[Oliver Hunt]

Created attachment 222771 [details]
Patch

[Oliver Hunt]

> > 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.

[Oliver Hunt]

Created attachment 222774 [details]
Patch

[Oliver Hunt]

Created attachment 222777 [details]
Patch

[Oliver Hunt]

Created attachment 222784 [details]
Patch

[Oliver Hunt]

Created attachment 222785 [details]
Patch

i can haz fix windows\?

[Sergio Correia (qrwteyrutiyoup)]

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

[Zan Dobersek]

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.

[Oliver Hunt]

Created attachment 222816 [details]
Patch

and now to not break literally every build

[Oliver Hunt]

(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

[Oliver Hunt]

Created attachment 222817 [details]
Patch

[Oliver Hunt]

Created attachment 222819 [details]
Patch

[Oliver Hunt]

Created attachment 222820 [details]
Patch

i hate this

[Oliver Hunt]

Created attachment 222824 [details]
Patch

lets see if efl and gtk builds now

[Oliver Hunt]

Created attachment 222828 [details]
Patch

now lets try efl

[Oliver Hunt]

Created attachment 222832 [details]
Patch

now lets try windows

[Oliver Hunt]

Created attachment 222833 [details]
Patch

dammit efl build

[Oliver Hunt]

Created attachment 222834 [details]
Patch

[Sergio Correia (qrwteyrutiyoup)]

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 :)

[Oliver Hunt]

Created attachment 222835 [details]
Patch

[Oliver Hunt]

Created attachment 222838 [details]
Patch

Rebase to convince windows to apply the patch properly

[Oliver Hunt]

Committed r163195: <http://trac.webkit.org/changeset/163195>

[Oliver Hunt]

Reopening to attach new patch.

[Oliver Hunt]

Created attachment 223913 [details]
Patch

[WebKit Commit Bot]

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.

[Oliver Hunt]

Created attachment 223919 [details]
fix release builds

[WebKit Commit Bot]

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.

[Michael Saboff]

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).

[Oliver Hunt]

(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).

[Ryosuke Niwa]

Windows build fix attempted in http://trac.webkit.org/changeset/163967.

[Ryosuke Niwa]

(In reply to comment #61)
> Windows build fix attempted in http://trac.webkit.org/changeset/163967.

That worked.