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.
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.
Created attachment 283643 [details] Patch
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`)
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.
Let's backup with sizeof then.
Created attachment 283660 [details] Patch
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?
Didn't have the time to investigate further. I added a fixme in the generator to do follow on this.
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]; ^ ~
(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.
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.
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))
(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.
(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
(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.
(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.
(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.
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.
Created attachment 283914 [details] Patch
Comment on attachment 283914 [details] Patch r=me as long as the bots are happy.
Comment on attachment 283914 [details] Patch LGTM too. A test case for a builtin with multiple conditionals would be good to add.
(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.
<rdar://problem/27405849>
Comment on attachment 283914 [details] Patch Clearing flags on attachment: 283914 Committed r203353: <http://trac.webkit.org/changeset/203353>
All reviewed patches have been landed. Closing bug.