Bug 159765 – REGRESSION(r202975): --minimal build is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Csaba Osztrogonác]

DerivedSources/WebCore/WebCoreJSBuiltinInternals.cpp:125:103: error: no matching function for call to 'ArrayLengthHelperFunction(JSC::JSGlobalObject::GlobalPropertyInfo [0])'
In file included from ../../Source/WTF/wtf/FastMalloc.h:26:0,
                 from ../../Source/WebCore/config.h:75,
                 from DerivedSources/WebCore/WebCoreJSBuiltinInternals.cpp:36:
../../Source/WTF/wtf/StdLibExtras.h:172:42: note: candidate: template<class T, long unsigned int Size> char (& WTF::ArrayLengthHelperFunction(T (&)[Size]))[Size]
../../Source/WTF/wtf/StdLibExtras.h:172:42: note:   template argument deduction/substitution failed:
../../Source/WTF/wtf/StdLibExtras.h:175:29: note: candidate: template<class T> char (& WTF::ArrayLengthHelperFunction(T (&)[0]))[0]
../../Source/WTF/wtf/StdLibExtras.h:175:29: note:   template argument deduction/substitution failed:
DerivedSources/WebCore/WebCoreJSBuiltinInternals.cpp:125:103: note:   template argument '-1' does not match '#'integer_cst' not supported by dump_decl#<declaration error>'


preprocessed DerivedSources/WebCore/WebCoreJSBuiltinInternals.cpp:
-------------------------------------------------------------------
....
    JSDOMGlobalObject::GlobalPropertyInfo staticGlobals[] = {
    };
    globalObject.addStaticGlobals(staticGlobals, sizeof(::WTF::ArrayLengthHelperFunction(staticGlobals)));
....

It seems the new script doesn't handle empty staticGlobals[] properly.
I don't know anything about this code path and don't plan to fix it
myself, I just reported to let you know this build break.

[youenn fablet]

Thanks I'll check that today or tomorrow.

(In reply to comment #0)
> DerivedSources/WebCore/WebCoreJSBuiltinInternals.cpp:125:103: error: no
> matching function for call to
> 'ArrayLengthHelperFunction(JSC::JSGlobalObject::GlobalPropertyInfo [0])'
> In file included from ../../Source/WTF/wtf/FastMalloc.h:26:0,
>                  from ../../Source/WebCore/config.h:75,
>                  from
> DerivedSources/WebCore/WebCoreJSBuiltinInternals.cpp:36:
> ../../Source/WTF/wtf/StdLibExtras.h:172:42: note: candidate: template<class
> T, long unsigned int Size> char (& WTF::ArrayLengthHelperFunction(T
> (&)[Size]))[Size]
> ../../Source/WTF/wtf/StdLibExtras.h:172:42: note:   template argument
> deduction/substitution failed:
> ../../Source/WTF/wtf/StdLibExtras.h:175:29: note: candidate: template<class
> T> char (& WTF::ArrayLengthHelperFunction(T (&)[0]))[0]
> ../../Source/WTF/wtf/StdLibExtras.h:175:29: note:   template argument
> deduction/substitution failed:
> DerivedSources/WebCore/WebCoreJSBuiltinInternals.cpp:125:103: note:  
> template argument '-1' does not match '#'integer_cst' not supported by
> dump_decl#<declaration error>'
> 
> 
> preprocessed DerivedSources/WebCore/WebCoreJSBuiltinInternals.cpp:
> -------------------------------------------------------------------
> ....
>     JSDOMGlobalObject::GlobalPropertyInfo staticGlobals[] = {
>     };
>     globalObject.addStaticGlobals(staticGlobals,
> sizeof(::WTF::ArrayLengthHelperFunction(staticGlobals)));
> ....
> 
> It seems the new script doesn't handle empty staticGlobals[] properly.
> I don't know anything about this code path and don't plan to fix it
> myself, I just reported to let you know this build break.

[youenn fablet]

Created attachment 283643 [details]
Patch

[WebKit Commit Bot]

This patch modifies the JS builtins code generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-builtins-generator-tests --reset-results`)

[Csaba Osztrogonác]

Comment on attachment 283643 [details]
Patch

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

> Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_implementation.py:136
> -        lines.append("    globalObject.addStaticGlobals(staticGlobals, WTF_ARRAY_LENGTH(staticGlobals));")
> +        lines.append("    if (sizeof(staticGlobals))")
> +        lines.append("        globalObject.addStaticGlobals(staticGlobals, WTF_ARRAY_LENGTH(staticGlobals));")

It doesn't fix the build, error is error inside if too.

[youenn fablet]

Let's backup with sizeof then.

[youenn fablet]

Created attachment 283660 [details]
Patch

[Alex Christensen]

There's a comment in the definition of WTF_ARRAY_LENGTH saying it supports 0-length arrays.  Why doesn't it work in this case?

[youenn fablet]

Didn't have the time to investigate further. I added a fixme in the generator to do follow on this.

[Csaba Osztrogonác]

just a note, I got the same error with clang 3.8: (without the fix)

DerivedSources/WebCore/WebCoreJSBuiltinInternals.cpp:125:50: error: no matching function for call to 'ArrayLengthHelperFunction'
    globalObject.addStaticGlobals(staticGlobals, WTF_ARRAY_LENGTH(staticGlobals));
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/WTF/wtf/StdLibExtras.h:177:40: note: expanded from macro 'WTF_ARRAY_LENGTH'
#define WTF_ARRAY_LENGTH(array) sizeof(::WTF::ArrayLengthHelperFunction(array))
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/WTF/wtf/StdLibExtras.h:172:42: note: candidate template ignored: substitution failure [with T = JSC::JSGlobalObject::GlobalPropertyInfo, Size = 0]: zero-length arrays are not permitted in C++
template<typename T, size_t Size> char (&ArrayLengthHelperFunction(T (&)[Size]))[Size];
                                         ^                                       ~~~~
../../Source/WTF/wtf/StdLibExtras.h:175:29: note: candidate template ignored: substitution failure [with T = JSC::JSGlobalObject::GlobalPropertyInfo]: zero-length arrays are not permitted in C++
template<typename T> char (&ArrayLengthHelperFunction(T (&)[0]))[0];
                            ^                              ~

[Csaba Osztrogonác]

(In reply to comment #7)
> There's a comment in the definition of WTF_ARRAY_LENGTH saying it supports
> 0-length arrays.  Why doesn't it work in this case?

It is a 4 years old comment - https://trac.webkit.org/changeset/114042/trunk/Source/WTF/wtf/StdLibExtras.h

Maybe GCC/Clang change a little bit since then.

[Chris Dumez]

Comment on attachment 283660 [details]
Patch

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

> Source/JavaScriptCore/Scripts/tests/builtins/expected/WebCore-ArbitraryConditionalGuard-Separate.js-result:403
>      JSDOMGlobalObject::GlobalPropertyInfo staticGlobals[] = {

Why do we generate such code at all? Seems to me we should not generate empty arrays.

[Alexey Proskuryakov]

Comment on attachment 283660 [details]
Patch

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

> Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_implementation.py:135
> +        # FIXME: We should use WTF_ARRAY_LENGTH here but it fails on zero-length arrays.

Would the below work?

// Macro that returns a compile time constant with the length of an array, but gives an error if passed a non-array.
template<typename T, size_t Size> char (&ArrayLengthHelperFunction(T (&)[Size]))[Size];
// GCC needs some help to deduce a 0 length array.
-#if COMPILER(GCC_OR_CLANG)
+#if COMPILER(GCC)
template<typename T> char (&ArrayLengthHelperFunction(T (&)[0]))[0];
#endif
#define WTF_ARRAY_LENGTH(array) sizeof(::WTF::ArrayLengthHelperFunction(array))

[youenn fablet]

(In reply to comment #11)
> Comment on attachment 283660 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283660&action=review
> 
> > Source/JavaScriptCore/Scripts/tests/builtins/expected/WebCore-ArbitraryConditionalGuard-Separate.js-result:403
> >      JSDOMGlobalObject::GlobalPropertyInfo staticGlobals[] = {
> 
> Why do we generate such code at all? Seems to me we should not generate
> empty arrays.

The issue is that arrays are not empty except they have entries that are #ifdefed.
In that case, the generator cannot know beforehand whether array is empty or not.

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #11)
> > Comment on attachment 283660 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=283660&action=review
> > 
> > > Source/JavaScriptCore/Scripts/tests/builtins/expected/WebCore-ArbitraryConditionalGuard-Separate.js-result:403
> > >      JSDOMGlobalObject::GlobalPropertyInfo staticGlobals[] = {
> > 
> > Why do we generate such code at all? Seems to me we should not generate
> > empty arrays.
> 
> The issue is that arrays are not empty except they have entries that are
> #ifdefed.
> In that case, the generator cannot know beforehand whether array is empty or
> not.

It can if you add the right #ifdefs outside the array declaration.
e.g.
#if ENABLE(...) || ENABLE(...)
//declare array
// add globals
#endif

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Comment on attachment 283660 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=283660&action=review
> > > 
> > > > Source/JavaScriptCore/Scripts/tests/builtins/expected/WebCore-ArbitraryConditionalGuard-Separate.js-result:403
> > > >      JSDOMGlobalObject::GlobalPropertyInfo staticGlobals[] = {
> > > 
> > > Why do we generate such code at all? Seems to me we should not generate
> > > empty arrays.
> > 
> > The issue is that arrays are not empty except they have entries that are
> > #ifdefed.
> > In that case, the generator cannot know beforehand whether array is empty or
> > not.
> 
> It can if you add the right #ifdefs outside the array declaration.
> e.g.
> #if ENABLE(...) || ENABLE(...)
> //declare array
> // add globals
> #endif

JSGlobalObject::addStaticGlobals() is not inlined and therefore we generate a function call for no reason in such cases.

[youenn fablet]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Comment on attachment 283660 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=283660&action=review
> > > 
> > > > Source/JavaScriptCore/Scripts/tests/builtins/expected/WebCore-ArbitraryConditionalGuard-Separate.js-result:403
> > > >      JSDOMGlobalObject::GlobalPropertyInfo staticGlobals[] = {
> > > 
> > > Why do we generate such code at all? Seems to me we should not generate
> > > empty arrays.
> > 
> > The issue is that arrays are not empty except they have entries that are
> > #ifdefed.
> > In that case, the generator cannot know beforehand whether array is empty or
> > not.
> 
> It can if you add the right #ifdefs outside the array declaration.
> e.g.
> #if ENABLE(...) || ENABLE(...)
> //declare array
> // add globals
> #endif

When shipping the fix, we thought that it was not worth the effort to do that.
Maybe we should...
I will prepare a patch accordingly.

> template<typename T, size_t Size> char (&ArrayLengthHelperFunction(T
> (&)[Size]))[Size];
> // GCC needs some help to deduce a 0 length array.
> -#if COMPILER(GCC_OR_CLANG)
> +#if COMPILER(GCC)
> template<typename T> char (&ArrayLengthHelperFunction(T (&)[0]))[0];
> #endif
> #define WTF_ARRAY_LENGTH(array)
> sizeof(::WTF::ArrayLengthHelperFunction(array))

I think I tried and I don't think it was working.

[Csaba Osztrogonác]

(In reply to comment #12)
> Comment on attachment 283660 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283660&action=review
> 
> > Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_implementation.py:135
> > +        # FIXME: We should use WTF_ARRAY_LENGTH here but it fails on zero-length arrays.
> 
> Would the below work?
> 
> // Macro that returns a compile time constant with the length of an array,
> but gives an error if passed a non-array.
> template<typename T, size_t Size> char (&ArrayLengthHelperFunction(T
> (&)[Size]))[Size];
> // GCC needs some help to deduce a 0 length array.
> -#if COMPILER(GCC_OR_CLANG)
> +#if COMPILER(GCC)
> template<typename T> char (&ArrayLengthHelperFunction(T (&)[0]))[0];
> #endif
> #define WTF_ARRAY_LENGTH(array)
> sizeof(::WTF::ArrayLengthHelperFunction(array))

No, zero sized arrays are broken with Clang and GCC too
with and without ifdef-ed workaround.

[Alexey Proskuryakov]

Given that, the broken workaround should be removed. I agree that fixing WTF_ARRAY_LENGTH is not the best fix for this bug, as we'd have an unneeded function call.

[youenn fablet]

Created attachment 283914 [details]
Patch

[Chris Dumez]

Comment on attachment 283914 [details]
Patch

r=me as long as the bots are happy.

[Blaze Burg]

Comment on attachment 283914 [details]
Patch

LGTM too. A test case for a builtin with multiple conditionals would be good to add.

[youenn fablet]

(In reply to comment #21)
> Comment on attachment 283914 [details]
> Patch
> 
> LGTM too. A test case for a builtin with multiple conditionals would be good
> to add.

Yes, we should extend the builtin generator test script to take multiple files at once and concatenate the results, or just output the wrapper files.

[Radar WebKit Bug Importer]

<rdar://problem/27405849>

[WebKit Commit Bot]

Comment on attachment 283914 [details]
Patch

Clearing flags on attachment: 283914

Committed r203353: <http://trac.webkit.org/changeset/203353>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.