Separate configuration extraction from offset extraction
Created attachment 350043 [details] Patch
Attachment 350043 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/llint/LLIntSettingsExtractor.cpp:33: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 350244 [details] Patch
Attachment 350244 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/llint/LLIntSettingsExtractor.cpp:33: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 350273 [details] Patch
Attachment 350273 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/llint/LLIntSettingsExtractor.cpp:33: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 350273 [details] Patch Attachment 350273 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9288391 New failing tests: accessibility/smart-invert-reference.html
Created attachment 350283 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 350273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350273&action=review Some comments for now, and some requests: 1. Please rebase to ToT to green out the EWS. 2. Have you tested this on building a fat binary (with multiple ARCH targets) yet? If not, please do. 3. Please attach an example of the generated LLIntDesiredSettings.h so that we can have an idea of how the output will look like. It will also help the review go easier. Thanks. > Source/JavaScriptCore/ChangeLog:16 > + * JavaScriptCore.xcodeproj/project.pbxproj: > + * offlineasm/generate_offset_extractor.rb: > + * offlineasm/offsets.rb: > + * offlineasm/settings.rb: This appears to be out of date. For one thing, it's missing the new LLIntSettingsExtractor.cpp. > Source/JavaScriptCore/llint/LLIntSettingsExtractor.cpp:33 > + // Out of an abundance of caution, make sure that LLIntSettingsExtractor::dummy() is live, > + // and the extractorTable is live, too. It was copied over from LLIntOffsetExtractor but is meaningless here because you don't have a dummy() function/. You embedded "LLIntDesiredSettings.h" in main() itself instead. Please remove. > Source/JavaScriptCore/llint/LLIntSettingsExtractor.cpp:35 > + printf("%p\n", extractorTable); nit: lets name this settingsExtractorTable now that we have 2 types of extractors. > Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:43 > +$stderr.puts settingsFlnm Do we need this? Or is this just leftover debugging code? > Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:51 > + $stderr.puts "offlineasm: No magic values found. Skipping assembly file generation." The error message should say "OffsetExtractor: ..." instead. > Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:63 > +inputHash = "// offlineasm input hash: #{parseHash(inputFlnm)} #{configurationHash} #{selfHash}" Ditto. /offlineasm/OffsetExtractor/. > Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:84 > + outp.puts "static const int64_t extractorTable[] = {" nit: lets name this offsetsExtractorTable now that we have 2 types of extractors, and change it in LLIntOffsetsExtractor.cpp as well. > Source/JavaScriptCore/offlineasm/generate_settings_extractor.rb:3 > +# Copyright (C) 2011 Apple Inc. All rights reserved. 2018, or 2011-2018 since it was refactored out of generate_offset_extractor.rb. > Source/JavaScriptCore/offlineasm/generate_settings_extractor.rb:45 > +inputHash = "// offlineasm input hash: #{parseHash(inputFlnm)} #{selfHash}" /offline/SettingsExtractor/. > Source/JavaScriptCore/offlineasm/generate_settings_extractor.rb:52 > + $stderr.puts "offlineasm: Nothing changed." /offline/SettingsExtractor/. > Source/JavaScriptCore/offlineasm/generate_settings_extractor.rb:69 > + outp.puts "static const int64_t extractorTable[#{length}] = {" nit: lets name this settingsExtractorTable now that we have 2 types of extractors.
Created attachment 350734 [details] Patch
Attachment 350734 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/llint/LLIntSettingsExtractor.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 350735 [details] LLIntDesiredSettings.h
Thanks for the review. I addressed the comments, rebased and attached a sample LLIntDesiredSettings.h from ToT.
Comment on attachment 350734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350734&action=review r=me with comments. > Source/JavaScriptCore/CMakeLists.txt:250 > DEPENDS ${LLINT_ASM} ${OFFLINE_ASM} ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/InitBytecodes.asm I think this needs to depend on the result of the add_executable for LLIntSettingsExtractor? > Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:49 > + $stderr.puts "OffsetExtractor: No magic values found. Skipping assembly file generation." Nit: Shouldn't this be "OffsetExtractor: No magic values found. Skipping offsets extractor file generation."?
Created attachment 352211 [details] Patch for landing
Comment on attachment 352211 [details] Patch for landing Clearing flags on attachment: 352211 Committed r237084: <https://trac.webkit.org/changeset/237084>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45239818>
(In reply to WebKit Commit Bot from comment #16) > Comment on attachment 352211 [details] > Patch for landing > > Clearing flags on attachment: 352211 > > Committed r237084: <https://trac.webkit.org/changeset/237084> This revision has broken some builds: https://build.webkit.org/builders/Apple%20Sierra%20Release%20%2832-bit%20Build%29/builds/14654/steps/compile-webkit/logs/errors /Volumes/Data/slave/sierra-32bit-release/build/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:83:12: error: use of undeclared identifier 'offsetExtractorTable'; did you mean 'extractorTable'? https://build.webkit.org/builders/Apple%20Sierra%20Debug%20%28Build%29/builds/14016/steps/compile-webkit/logs/stdio /Volumes/Data/slave/sierra-debug/build/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:83:12: error: use of undeclared identifier 'offsetExtractorTable'; did you mean 'extractorTable'?
Committed r237088: <https://trac.webkit.org/changeset/237088>
Build fix landed in r237088 <https://trac.webkit.org/r237088>
Another build fix landed in r237098: <http://trac.webkit.org/r237098>.
Third build fix landed in r237114: <http://trac.webkit.org/r237114>
Re-opened since this is blocked by bug 190602
Committed r237188: <https://trac.webkit.org/changeset/237188>
Forgot to add files. follow-up in https://trac.webkit.org/changeset/237189.
WinCairo Debug builds are failing. But Release builds are fine. https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Debug%20%28Build%29/builds/3628 > [1197/6199] Generating ../../DerivedSources/JavaScriptCore/LowLevelInterpreterWin.asm > FAILED: DerivedSources/JavaScriptCore/LowLevelInterpreterWin.asm > cmd.exe /C "cd /D C:\WebKit-BuildWorker\wincairo-wkl-debug\build\WebKitBuild\Debug\DerivedSources\JavaScriptCore && C:\tools\ruby\bin\ruby.exe C:/WebKit-BuildWorker/wincairo-wkl-debug/build/Source/JavaScriptCore/offlineasm/asm.rb -IC:/WebKit-BuildWorker/wincairo-wkl-debug/build/WebKitBuild/Debug/DerivedSources/JavaScriptCore/ C:/WebKit-BuildWorker/wincairo-wkl-debug/build/Source/JavaScriptCore/llint/LowLevelInterpreter.asm C:/WebKit-BuildWorker/wincairo-wkl-debug/build/WebKitBuild/Debug/bin64/LLIntOffsetsExtractor.exe C:/WebKit-BuildWorker/wincairo-wkl-debug/build/WebKitBuild/Debug/DerivedSources/JavaScriptCore/LowLevelInterpreterWin.asm --assembler=MASM && C:\tools\cmake\bin\cmake.exe -E touch_nocreate C:/WebKit-BuildWorker/wincairo-wkl-debug/build/WebKitBuild/Debug/DerivedSources/JavaScriptCore/LowLevelInterpreterWin.asm" > C:/WebKit-BuildWorker/wincairo-wkl-debug/build/Source/JavaScriptCore/offlineasm/offsets.rb:226:in `buildOffsetsMap': unhandled exception > from C:/WebKit-BuildWorker/wincairo-wkl-debug/build/Source/JavaScriptCore/offlineasm/asm.rb:389:in `block (3 levels) in <main>' > from C:/WebKit-BuildWorker/wincairo-wkl-debug/build/Source/JavaScriptCore/offlineasm/settings.rb:107:in `forSettings' > from C:/WebKit-BuildWorker/wincairo-wkl-debug/build/Source/JavaScriptCore/offlineasm/asm.rb:379:in `block (2 levels) in <main>' > from C:/WebKit-BuildWorker/wincairo-wkl-debug/build/Source/JavaScriptCore/offlineasm/asm.rb:375:in `each' > from C:/WebKit-BuildWorker/wincairo-wkl-debug/build/Source/JavaScriptCore/offlineasm/asm.rb:375:in `block in <main>' > from C:/WebKit-BuildWorker/wincairo-wkl-debug/build/Source/JavaScriptCore/offlineasm/asm.rb:366:in `open' > from C:/WebKit-BuildWorker/wincairo-wkl-debug/build/Source/JavaScriptCore/offlineasm/asm.rb:366:in `<main>' This is the condition failing. > raise unless astOffsetsList.size + astSizesList.size + astConstsList.size == extractedConstants.size What does this check mean?
(In reply to Fujii Hironori from comment #27) > WinCairo Debug builds are failing. But Release builds are fine. WinCairo Release are failing LayoutTests.
Reverted r237188, r237189, and r237197 for reason: It breaks WinCairo Debug builds and Release LayoutTests Committed r237214: <https://trac.webkit.org/changeset/237214>
getLSBSet(sizeof(void*)) in offsetExtractorTable is the problem. It becomes a function call in MSVC Debug builds, and splits the table.
Created attachment 352560 [details] Fixed the WinCairo build and crash issues by preserving constexpr variables.
Attachment 352560 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/llint/LLIntSettingsExtractor.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Relanded: https://trac.webkit.org/r237219