Bug 189708 – Separate configuration extraction from offset extraction

• Component: New Bugs
• Status: RESOLVED FIXED

[Tadeu Zagallo]

Separate configuration extraction from offset extraction

[Tadeu Zagallo]

Created attachment 350043 [details]
Patch

[EWS Watchlist]

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.

[Tadeu Zagallo]

Created attachment 350244 [details]
Patch

[EWS Watchlist]

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.

[Tadeu Zagallo]

Created attachment 350273 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Mark Lam]

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.

[Tadeu Zagallo]

Created attachment 350734 [details]
Patch

[EWS Watchlist]

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.

[Tadeu Zagallo]

Created attachment 350735 [details]
LLIntDesiredSettings.h

[Tadeu Zagallo]

Thanks for the review. I addressed the comments, rebased and attached a sample LLIntDesiredSettings.h from ToT.

[Keith Miller]

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."?

[Tadeu Zagallo]

Created attachment 352211 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 352211 [details]
Patch for landing

Clearing flags on attachment: 352211

Committed r237084: <https://trac.webkit.org/changeset/237084>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45239818>

[Dawei Fenton (:realdawei)]

(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'?

[Tadeu Zagallo]

Committed r237088: <https://trac.webkit.org/changeset/237088>

[Tadeu Zagallo]

Build fix landed in r237088 <https://trac.webkit.org/r237088>

[Mark Lam]

Another build fix landed in r237098: <http://trac.webkit.org/r237098>.

[David Kilzer (:ddkilzer)]

Third build fix landed in r237114:  <http://trac.webkit.org/r237114>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 190602

[Keith Miller]

Committed r237188: <https://trac.webkit.org/changeset/237188>

[Keith Miller]

Forgot to add files. follow-up in https://trac.webkit.org/changeset/237189.

[Fujii Hironori]

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?

[Fujii Hironori]

(In reply to Fujii Hironori from comment #27)
> WinCairo Debug builds are failing. But Release builds are fine.

WinCairo Release are failing LayoutTests.

[Fujii Hironori]

Reverted r237188, r237189, and r237197 for reason:

It breaks WinCairo Debug builds and Release LayoutTests

Committed r237214: <https://trac.webkit.org/changeset/237214>

[Fujii Hironori]

getLSBSet(sizeof(void*)) in offsetExtractorTable is the problem.
It becomes a function call in MSVC Debug builds, and splits the table.

[Fujii Hironori]

Created attachment 352560 [details]
Fixed the WinCairo build and crash issues by preserving constexpr variables.

[EWS Watchlist]

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.

[Fujii Hironori]

Relanded: https://trac.webkit.org/r237219