Bug 189708 - Separate configuration extraction from offset extraction
Summary: Separate configuration extraction from offset extraction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on: 190602
Blocks:
  Show dependency treegraph
 
Reported: 2018-09-18 13:32 PDT by Tadeu Zagallo
Modified: 2018-10-17 21:10 PDT (History)
11 users (show)

See Also:


Attachments
Patch (42.47 KB, patch)
2018-09-18 13:32 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (42.47 KB, patch)
2018-09-20 12:01 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (45.80 KB, patch)
2018-09-20 15:33 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.91 MB, application/zip)
2018-09-20 17:17 PDT, EWS Watchlist
no flags Details
Patch (45.39 KB, patch)
2018-09-24 20:51 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
LLIntDesiredSettings.h (831.31 KB, text/x-csrc)
2018-09-24 21:05 PDT, Tadeu Zagallo
no flags Details
Patch for landing (39.71 KB, patch)
2018-10-12 15:03 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Fixed the WinCairo build and crash issues by preserving constexpr variables. (41.39 KB, patch)
2018-10-17 03:06 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2018-09-18 13:32:10 PDT
Separate configuration extraction from offset extraction
Comment 1 Tadeu Zagallo 2018-09-18 13:32:44 PDT
Created attachment 350043 [details]
Patch
Comment 2 EWS Watchlist 2018-09-18 13:34:09 PDT
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.
Comment 3 Tadeu Zagallo 2018-09-20 12:01:30 PDT
Created attachment 350244 [details]
Patch
Comment 4 EWS Watchlist 2018-09-20 12:04:48 PDT
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.
Comment 5 Tadeu Zagallo 2018-09-20 15:33:06 PDT
Created attachment 350273 [details]
Patch
Comment 6 EWS Watchlist 2018-09-20 15:35:59 PDT
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 7 EWS Watchlist 2018-09-20 17:17:19 PDT
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
Comment 8 EWS Watchlist 2018-09-20 17:17:23 PDT
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 9 Mark Lam 2018-09-24 16:43:56 PDT
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.
Comment 10 Tadeu Zagallo 2018-09-24 20:51:42 PDT
Created attachment 350734 [details]
Patch
Comment 11 EWS Watchlist 2018-09-24 20:54:25 PDT
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.
Comment 12 Tadeu Zagallo 2018-09-24 21:05:20 PDT
Created attachment 350735 [details]
LLIntDesiredSettings.h
Comment 13 Tadeu Zagallo 2018-09-24 21:10:48 PDT
Thanks for the review. I addressed the comments, rebased and attached a sample LLIntDesiredSettings.h from ToT.
Comment 14 Keith Miller 2018-10-02 11:54:27 PDT
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."?
Comment 15 Tadeu Zagallo 2018-10-12 15:03:55 PDT
Created attachment 352211 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2018-10-12 15:39:53 PDT
Comment on attachment 352211 [details]
Patch for landing

Clearing flags on attachment: 352211

Committed r237084: <https://trac.webkit.org/changeset/237084>
Comment 17 WebKit Commit Bot 2018-10-12 15:39:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2018-10-12 15:40:28 PDT
<rdar://problem/45239818>
Comment 19 Dawei Fenton (:realdawei) 2018-10-12 15:45:38 PDT
(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'?
Comment 20 Tadeu Zagallo 2018-10-12 16:17:07 PDT
Committed r237088: <https://trac.webkit.org/changeset/237088>
Comment 21 Tadeu Zagallo 2018-10-12 16:19:07 PDT
Build fix landed in r237088 <https://trac.webkit.org/r237088>
Comment 22 Mark Lam 2018-10-15 07:02:58 PDT
Another build fix landed in r237098: <http://trac.webkit.org/r237098>.
Comment 23 David Kilzer (:ddkilzer) 2018-10-15 08:56:46 PDT
Third build fix landed in r237114:  <http://trac.webkit.org/r237114>
Comment 24 WebKit Commit Bot 2018-10-15 14:35:07 PDT
Re-opened since this is blocked by bug 190602
Comment 25 Keith Miller 2018-10-16 10:44:34 PDT
Committed r237188: <https://trac.webkit.org/changeset/237188>
Comment 26 Keith Miller 2018-10-16 10:50:02 PDT
Forgot to add files. follow-up in https://trac.webkit.org/changeset/237189.
Comment 27 Fujii Hironori 2018-10-16 19:06:12 PDT
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?
Comment 28 Fujii Hironori 2018-10-16 19:41:21 PDT
(In reply to Fujii Hironori from comment #27)
> WinCairo Debug builds are failing. But Release builds are fine.

WinCairo Release are failing LayoutTests.
Comment 29 Fujii Hironori 2018-10-16 20:02:14 PDT
Reverted r237188, r237189, and r237197 for reason:

It breaks WinCairo Debug builds and Release LayoutTests

Committed r237214: <https://trac.webkit.org/changeset/237214>
Comment 30 Fujii Hironori 2018-10-17 00:27:43 PDT
getLSBSet(sizeof(void*)) in offsetExtractorTable is the problem.
It becomes a function call in MSVC Debug builds, and splits the table.
Comment 31 Fujii Hironori 2018-10-17 03:06:46 PDT
Created attachment 352560 [details]
Fixed the WinCairo build and crash issues by preserving constexpr variables.
Comment 32 EWS Watchlist 2018-10-17 03:08:00 PDT
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.
Comment 33 Fujii Hironori 2018-10-17 21:10:11 PDT
Relanded: https://trac.webkit.org/r237219