Details to come in ChangeLog.
Created attachment 379371 [details] proposed patch. Let's get some EWS testing.
Created attachment 379372 [details] proposed patch.
Comment on attachment 379372 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=379372&action=review r=me > Source/JavaScriptCore/runtime/JSCConfig.h:42 > #if CPU(ARM64) || PLATFORM(WATCHOS) > constexpr size_t PageSize = 16 * KB; > +constexpr size_t ConfigSizeToProtect = 1 * PageSize; > #else > constexpr size_t PageSize = 4 * KB; > +constexpr size_t ConfigSizeToProtect = 1 * PageSize; > #endif Add `PageSize = 64 * KB` for `CPU(PPC64) || CPU(PPC64LE) || CPU(PPC) || CPU(UNKNOWN)`.
Thanks for the review. (In reply to Yusuke Suzuki from comment #3) > > Source/JavaScriptCore/runtime/JSCConfig.h:42 > > #if CPU(ARM64) || PLATFORM(WATCHOS) > > constexpr size_t PageSize = 16 * KB; > > +constexpr size_t ConfigSizeToProtect = 1 * PageSize; > > #else > > constexpr size_t PageSize = 4 * KB; > > +constexpr size_t ConfigSizeToProtect = 1 * PageSize; > > #endif > > Add `PageSize = 64 * KB` for `CPU(PPC64) || CPU(PPC64LE) || CPU(PPC) || > CPU(UNKNOWN)`. I've added this case. Landed in r250262: <http://trac.webkit.org/r250262>.
<rdar://problem/55641041>
Windows port can't compile: https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/builds/6980 > c:\cygwin\worker\win10-release\build\webkitbuild\release\derivedsources\forwardingheaders\javascriptcore\optionslist.h(590): fatal error C1060: compiler is out of heap space (compiling source file C:\cygwin\worker\win10-release\build\WebKitBuild\Release\DerivedSources\WebCore\JSInternals.cpp) [C:\cygwin\worker\win10-release\build\WebKitBuild\Release\Source\WebCore\WebCoreTestSupport.vcxproj] > c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.15.26726\include\utility(405): fatal error C1060: compiler is out of heap space (compiling source file C:\cygwin\worker\win10-release\build\Source\WebKitLegacy\win\Plugins\PluginPackage.cpp) [C:\cygwin\worker\win10-release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]
>fatal error C1060: compiler is out of heap space I guess it uses x86_amd64 toolchain (i.e., 32-bit compiler and other tools) instead of amd64
Yes, AppleWin has only 32bit Buildbots. However, WinCairo support only 64bit. And, its buildbots are reporting same errors: WinCairo 64-bit WKL Debug (Build) WinCairo 64-bit WKL Release (Build) https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Build%29/builds/12188 > FAILED: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f2e18ffc-26.cpp.obj > C:\MSVS\VC\Tools\MSVC\14.20.27508\bin\Hostx64\x64\cl.exe /nologo /TP -DBUILDING_JavaScriptCore -DBUILDING_WITH_CMAKE=1 -DHAVE_CONFIG_H=1 -DJavaScriptCore_EXPORTS -DNOCRYPT -DNOMINMAX -DUNICODE -DWINVER=0x601 -DWTF_PLATFORM_WIN_CAIRO=1 -D_CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES=1 -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -D_WIN32_WINNT=0x601 -D_WINDOWS -D_WINSOCKAPI_="" -IJavaScriptCore\Headers -I. -I..\..\Source\JavaScriptCore -I..\..\Source\JavaScriptCore\API -I..\..\Source\JavaScriptCore\assembler -I..\..\Source\JavaScriptCore\b3 -I..\..\Source\JavaScriptCore\b3\air -I..\..\Source\JavaScriptCore\bindings -I..\..\Source\JavaScriptCore\builtins -I..\..\Source\JavaScriptCore\bytecode -I..\..\Source\JavaScriptCore\bytecompiler -I..\..\Source\JavaScriptCore\dfg -I..\..\Source\JavaScriptCore\disassembler -I..\..\Source\JavaScriptCore\disassembler\ARM64 -I..\..\Source\JavaScriptCore\disassembler\udis86 -I..\..\Source\JavaScriptCore\domjit -I..\..\Source\JavaScriptCore\ftl -I..\..\Source\JavaScriptCore\heap -I..\..\Source\JavaScriptCore\debugger -I..\..\Source\JavaScriptCore\inspector -I..\..\Source\JavaScriptCore\inspector\agents -I..\..\Source\JavaScriptCore\inspector\augmentable -I..\..\Source\JavaScriptCore\inspector\remote -I..\..\Source\JavaScriptCore\interpreter -I..\..\Source\JavaScriptCore\jit -I..\..\Source\JavaScriptCore\llint -I..\..\Source\JavaScriptCore\parser -I..\..\Source\JavaScriptCore\profiler -I..\..\Source\JavaScriptCore\runtime -I..\..\Source\JavaScriptCore\tools -I..\..\Source\JavaScriptCore\wasm -I..\..\Source\JavaScriptCore\wasm\js -I..\..\Source\JavaScriptCore\yarr -IJavaScriptCore\DerivedSources -IJavaScriptCore\DerivedSources\inspector -IJavaScriptCore\DerivedSources\runtime -IJavaScriptCore\DerivedSources\yarr -I..\include\private -I..\..\Source\JavaScriptCore\inspector\remote\socket -IWTF\Headers -IDerivedSources -I..\..\Source\ThirdParty -I..\..\WebKitLibraries\win\include /W4 /DWIN32 /D_WINDOWS /GR- /EHs- /EHc- /MD /O2 /Ob2 /DNDEBUG /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180 /wd4189 /wd4201 /wd4206 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291 /wd4305 /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458 /wd4459 /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611 /wd4646 /wd4702 /wd4706 /wd4722 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996 /wd6011 /wd6031 /wd6211 /wd6246 /wd6255 /wd6387 /Zi /GS /EHa- /EHc- /EHs- /fp:except- /analyze- /bigobj /utf-8 /validate-charset /Oy- -std:c++17 /showIncludes /FoSource\JavaScriptCore\CMakeFiles\JavaScriptCore.dir\__\__\JavaScriptCore\DerivedSources\unified-sources\UnifiedSource-f2e18ffc-26.cpp.obj /FdSource\JavaScriptCore\CMakeFiles\JavaScriptCore.dir\ /FS -c JavaScriptCore\DerivedSources\unified-sources\UnifiedSource-f2e18ffc-26.cpp > C:\WebKit-BuildWorker\wincairo-wkl-release\build\Source\JavaScriptCore\runtime\PropertyMapHashTable.h(289) : fatal error C1002: compiler is out of heap space in pass 2 > C:\WebKit-BuildWorker\wincairo-wkl-release\build\Source\JavaScriptCore\runtime\Uint16WithFraction.h(140) : fatal error C1002: compiler is out of heap space in pass 2 https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Debug%20%28Build%29/builds/10577 > FAILED: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f2e18ffc-31.cpp.obj > C:\MSVS\VC\Tools\MSVC\14.21.27702\bin\Hostx64\x64\cl.exe /nologo /TP -DBUILDING_JavaScriptCore -DBUILDING_WITH_CMAKE=1 -DHAVE_CONFIG_H=1 -DJavaScriptCore_EXPORTS -DNOCRYPT -DNOMINMAX -DUNICODE -DWINVER=0x601 -DWTF_PLATFORM_WIN_CAIRO=1 -D_CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES=1 -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -D_WIN32_WINNT=0x601 -D_WINDOWS -D_WINSOCKAPI_="" -IJavaScriptCore\Headers -I. -I..\..\Source\JavaScriptCore -I..\..\Source\JavaScriptCore\API -I..\..\Source\JavaScriptCore\assembler -I..\..\Source\JavaScriptCore\b3 -I..\..\Source\JavaScriptCore\b3\air -I..\..\Source\JavaScriptCore\bindings -I..\..\Source\JavaScriptCore\builtins -I..\..\Source\JavaScriptCore\bytecode -I..\..\Source\JavaScriptCore\bytecompiler -I..\..\Source\JavaScriptCore\dfg -I..\..\Source\JavaScriptCore\disassembler -I..\..\Source\JavaScriptCore\disassembler\ARM64 -I..\..\Source\JavaScriptCore\disassembler\udis86 -I..\..\Source\JavaScriptCore\domjit -I..\..\Source\JavaScriptCore\ftl -I..\..\Source\JavaScriptCore\heap -I..\..\Source\JavaScriptCore\debugger -I..\..\Source\JavaScriptCore\inspector -I..\..\Source\JavaScriptCore\inspector\agents -I..\..\Source\JavaScriptCore\inspector\augmentable -I..\..\Source\JavaScriptCore\inspector\remote -I..\..\Source\JavaScriptCore\interpreter -I..\..\Source\JavaScriptCore\jit -I..\..\Source\JavaScriptCore\llint -I..\..\Source\JavaScriptCore\parser -I..\..\Source\JavaScriptCore\profiler -I..\..\Source\JavaScriptCore\runtime -I..\..\Source\JavaScriptCore\tools -I..\..\Source\JavaScriptCore\wasm -I..\..\Source\JavaScriptCore\wasm\js -I..\..\Source\JavaScriptCore\yarr -IJavaScriptCore\DerivedSources -IJavaScriptCore\DerivedSources\inspector -IJavaScriptCore\DerivedSources\runtime -IJavaScriptCore\DerivedSources\yarr -I..\include\private -I..\..\Source\JavaScriptCore\inspector\remote\socket -IWTF\Headers -IDerivedSources -I..\..\Source\ThirdParty -I..\..\WebKitLibraries\win\include /W4 /DWIN32 /D_WINDOWS /GR- /EHs- /EHc- /MD /Zi /Ob0 /Od /RTC1 /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180 /wd4189 /wd4201 /wd4206 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291 /wd4305 /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458 /wd4459 /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611 /wd4646 /wd4702 /wd4706 /wd4722 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996 /wd6011 /wd6031 /wd6211 /wd6246 /wd6255 /wd6387 /Zi /GS /EHa- /EHc- /EHs- /fp:except- /analyze- /bigobj /utf-8 /validate-charset -std:c++17 /showIncludes /FoSource\JavaScriptCore\CMakeFiles\JavaScriptCore.dir\__\__\JavaScriptCore\DerivedSources\unified-sources\UnifiedSource-f2e18ffc-31.cpp.obj /FdSource\JavaScriptCore\CMakeFiles\JavaScriptCore.dir\ /FS -c JavaScriptCore\DerivedSources\unified-sources\UnifiedSource-f2e18ffc-31.cpp > C:\WebKit-BuildWorker\wincairo-wkl-debug\build\WebKitBuild\Debug\WTF\Headers\wtf/SetForScope.h(43): fatal error C1060: compiler is out of heap space
>Yes, AppleWin has only 32bit Buildbots. They could switch to 64-bit toolchain to produce 32-bit binaries. It's probably called amd64_x86 in vcvarsall
(In reply to Konstantin Tokarev from comment #9) > >Yes, AppleWin has only 32bit Buildbots. > > They could switch to 64-bit toolchain to produce 32-bit binaries. It's > probably called amd64_x86 in vcvarsall Looks like a MSVC bug: https://developercommunity.visualstudio.com/content/problem/653301/fatal-error-c1002-compiler-is-out-of-heap-space-in.html Posted on Sep 6: "A fix for this issue has been internally implemented and is being prepared for release. We’ll update you once it becomes available for download".
(In reply to Konstantin Tokarev from comment #9) > They could switch to 64-bit toolchain to produce 32-bit binaries. It's > probably called amd64_x86 in vcvarsall According the compilation log of comment 6, AppleWin is using 'HostX64\x86\CL.exe'. > INTERNAL COMPILER ERROR in 'C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.15.26726\bin\HostX64\x86\CL.exe' > Please choose the Technical Support command on the Visual C++ > Help menu, or open the Technical Support help file for more information > cl : Command line error D8040: error creating or communicating with child process [C:\cygwin\worker\win10-release\build\WebKitBuild\Release\Source\WebCore\WebCoreTestSupport.vcxproj]
(In reply to Mark Lam from comment #10) > Looks like a MSVC bug: > https://developercommunity.visualstudio.com/content/problem/653301/fatal- > error-c1002-compiler-is-out-of-heap-space-in.html Nice to find it out. > To workaround this issue, please use the x64 hosted toolset or use /cgthreads1 to disable multithreaded compilation. It must be unacceptable for Windows devs to use /cgthreads1. Let's revert the patch until the upcoming MSVC release.
(In reply to Fujii Hironori from comment #12) > (In reply to Mark Lam from comment #10) > > Looks like a MSVC bug: > > https://developercommunity.visualstudio.com/content/problem/653301/fatal- > > error-c1002-compiler-is-out-of-heap-space-in.html > > Nice to find it out. > > > To workaround this issue, please use the x64 hosted toolset or use /cgthreads1 to disable multithreaded compilation. > > It must be unacceptable for Windows devs to use /cgthreads1. > Let's revert the patch until the upcoming MSVC release. It's sad for progress to be gated by MSVC. I'll roll out and see if I can workaround this for MSVC.
Re-opened since this is blocked by bug 202126
Created attachment 381082 [details] Rebased patch for landing.
Created attachment 381108 [details] refactored test patch for checking Windows build.
Created attachment 381133 [details] another test patch for MSVC.
Created attachment 381134 [details] test patch for MSVC.
Created attachment 381135 [details] test patch for MSVC.
Created attachment 381142 [details] patch for landing.
Created attachment 381148 [details] patch for landing.
Comment on attachment 381148 [details] patch for landing. Attachment 381148 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13141860 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla
(In reply to Build Bot from comment #22) > New failing tests: > mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla I can reproduce this without my patch. The test is inherently flaky.
Fix landed in r251226: <http://trac.webkit.org/r251226>.
Filed: Bug 203142 – [Clang][Windows] Options.cpp(317,25): error: no matching function for call to 'optionTypeSpecificIndex'
This was rolled out in r251400: <http://trac.webkit.org/r251400>.
Created attachment 382356 [details] work in progress using type specific index from Options::s_optionsInfo.
Created attachment 383711 [details] proposed patch. Without template magic this time. In fact, I reduced the amount of stuff defined in Options.h. So, compile time should be the same or marginally less.
Created attachment 383712 [details] proposed patch.
Created attachment 383713 [details] proposed patch.
Comment on attachment 383713 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=383713&action=review r=me, I love a patch that makes things easier to read and more efficient at the same time. > Source/JavaScriptCore/runtime/JSCConfig.h:42 > +constexpr size_t ConfigSizeToProtect = PageSize; Is it possible to have some static_assert that the size of the options fit in that ? I've seen one that checks for 16*KB, are we guaranteed that PageSize is always that on every platform? > Source/JavaScriptCore/runtime/Options.cpp:917 > return; // Illegal option. If it is illegal, can we make it a release assert instead? > Source/JavaScriptCore/runtime/Options.cpp:1038 > { nitpicking, but I think an ASSERT(type() == other.type()) might be valuable here. > Source/JavaScriptCore/runtime/OptionsList.h:74 > +// Any modifications to options must be done before the first VM is instantiate. nit: instantiate => instantiated. > Source/JavaScriptCore/runtime/OptionsList.h:555 > + { // Only needed for initialization I don't understand this at all, this function appears to do nothing when passed a non-zero number. I realize it's not a change from your patch, but can you explain what this is for?
Comment on attachment 383713 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=383713&action=review Thanks for the review. >> Source/JavaScriptCore/runtime/JSCConfig.h:42 >> +constexpr size_t ConfigSizeToProtect = PageSize; > > Is it possible to have some static_assert that the size of the options fit in that ? > I've seen one that checks for 16*KB, are we guaranteed that PageSize is always that on every platform? We already have a static_assert(sizeof(Config) == ConfigSizeToProtect, "") below (in JSCConfig.h) that covers this. >> Source/JavaScriptCore/runtime/Options.cpp:917 >> return; // Illegal option. > > If it is illegal, can we make it a release assert instead? Done. >> Source/JavaScriptCore/runtime/Options.cpp:1038 >> { > > nitpicking, but I think an ASSERT(type() == other.type()) might be valuable here. Done. >> Source/JavaScriptCore/runtime/OptionsList.h:74 >> +// Any modifications to options must be done before the first VM is instantiate. > > nit: instantiate => instantiated. Fixed. >> Source/JavaScriptCore/runtime/OptionsList.h:555 >> + { // Only needed for initialization > > I don't understand this at all, this function appears to do nothing when passed a non-zero number. I realize it's not a change from your patch, but can you explain what this is for? I think the reason for this is because options in OptionsList.h of type OptionRange has a default value of 0. The 0 means no range specified, so please set to Uninitialized. However, it's meaningless to pass in any other int value. We can RELEASE_ASSERT that rhs is never non-zero, or provide a better way to specify the initial value. I'll leave this as is for now. We can improve it in a follow up patch.
Landed in r252557: <http://trac.webkit.org/r252557>.