Bug 190758 – Re-enable LTO support

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Keith Rollin]

Bug 187785 (r235381) added support for building WebKit with LTO. An infrequent crashing bug in libLTO.dylib required us to back this out in r235412. When a fixed libLTO.dylib is available, re-enable WebKit's support for building with LTO.

[Radar WebKit Bug Importer]

<rdar://problem/45413233>

[Alexey Proskuryakov]

Do we have a consensus that other consequences (like build time impact) are acceptable to the project?

[Keith Rollin]

LTO is being enabled by default only for Production builds. So consensus should be amount the parties that perform those builds, which I guess is your group and Apple's central build center.

Consideration should be based on the actual amount of time added for the build machines used (e.g., an LTO-thin build takes about 2-3 minutes longer on my development system than a non-LTO build (18m27s vs. 16m5s)) as well as the direct (faster, smaller binaries) and indirect (e.g., support for CFI (https://clang.llvm.org/docs/ControlFlowIntegrity.html) and other security measures) benefits of LTO.

Hopefully, the increased build time has been mitigated over the last year through the use of unified-source builds, something which has still to be completed, and will further be mitigated with the enabling of XCBuild.

[Alexey Proskuryakov]

IIRC another objection was that people working on performance weren't OK with having different performance characteristics in release and production builds.

[Keith Rollin]

Created attachment 365078 [details]
Patch

[Keith Rollin]

Note, there are 32-bit artifacts in this patch. As I understand it, those can be removed. I'd like to do that in a follow-up patch. Removing that support will require extensive retesting, which would affect our being able to take advantage of the LTO benefits now.

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

[Alexey Proskuryakov]

Comment on attachment 365078 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365078&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        Enable ThinLTO support in Production builds

Need build performance measurement before I can support this change. Continued an e-mail thread.

[Keith Rollin]

The iOS build is failing.

For ThinLTO:

Ld /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/WebKit.build/WebKit.build/Objects-normal/arm64e/WebKit normal arm64e
    cd /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/Sources/WebKit2
    export IPHONEOS_DEPLOYMENT_TARGET=12.2
    export PATH="/Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/usr/bin:/Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/usr/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"
    /Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/Toolchains/iOS12.2.xctoolchain/usr/bin/clang++ -arch arm64e -dynamiclib -isysroot /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk -L/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/Symbols/BuiltProducts -L/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/Shared/Symbols/ios -L/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/PrivateFrameworks/WebCore.framework/Frameworks -F/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/Symbols/BuiltProducts -F/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/Shared/Symbols/ios -F/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/PrivateFrameworks -iframework /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/PrivateFrameworks -iframework /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/Frameworks -iframework /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/PrivateFrameworks -filelist /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/WebKit.build/WebKit.build/Objects-normal/arm64e/WebKit.LinkFileList -Xlinker -reexport_framework -Xlinker WebKitLegacy -install_name /System/Library/Frameworks/WebKit.framework/WebKit -miphoneos-version-min=12.2 -dead_strip -Xlinker -object_path_lto -Xlinker /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/WebKit.build/WebKit.build/Objects-normal/arm64e/WebKit_lto.o -Xlinker -final_output -Xlinker /System/Library/Frameworks/WebKit.framework/WebKit -stdlib=libc++ -flto=thin -Xlinker -cache_path_lto -Xlinker /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/LTOCache -Wl,-order_file,/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/AppleInternal/OrderFiles/WebKit.order -iframework/Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/SDKs/iPhoneOS12.2.Internal.sdk/System/Library/PrivateFrameworks -Wl,-unexported_symbol -Wl,__ZTISt9bad_alloc -Wl,-unexported_symbol -Wl,__ZTISt9exception -Wl,-unexported_symbol -Wl,__ZTSSt9bad_alloc -Wl,-unexported_symbol -Wl,__ZTSSt9exception -Wl,-unexported_symbol -Wl,__ZdlPvS_ -Wl,-unexported_symbol -Wl,__ZnwmPv -Wl,-unexported_symbol -Wl,__Znwm -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEC2EOS4_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEC1EOS4_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEaSEDn -Wl,-unexported_symbol -Wl,__ZNKSt3__18functionIFvN7WebCore12PolicyActionEEEclES2_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEE4swapERS4_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEC1ERKS4_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEC2ERKS4_ -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEED1Ev -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEED2Ev -Wl,-unexported_symbol -Wl,__ZNSt3__18functionIFvN7WebCore12PolicyActionEEEaSERKS4_ -Wl,-unexported_symbol -Wl,__ZTVNSt3__117bad_function_callE -Wl,-unexported_symbol -Wl,__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_13basic_istreamIcS2_EE -Wl,-unexported_symbol -Wl,__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_14basic_iostreamIcS2_EE -Wl,-unexported_symbol -Wl,__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE16_NS_13basic_ostreamIcS2_EE -Wl,-unexported_symbol -Wl,__ZTTNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE -Wl,-unexported_symbol -Wl,__ZTVNSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEEE -Wl,-unexported_symbol -Wl,__ZTVNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE -Wl,-unexported_symbol -Wl,__ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE8_NS_13basic_ostreamIcS2_EE -lobjc -framework CFNetwork -framework CoreAudio -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework ImageIO -framework IOKit -framework WebKitLegacy -lnetwork -lAccessibility -framework AssertionServices -framework CorePDF -framework CorePrediction -framework DeviceIdentity -framework GraphicsServices -framework IOSurface -weak-lwebrtc -framework MobileCoreServices -lMobileGestalt -framework OpenGLES -framework PDFKit -framework SafariSafeBrowsing -framework UIKit -framework URLFormatting -framework JavaScriptCore -licucore -framework QuartzCore -framework Security -framework WebCore -compatibility_version 1 -current_version 608.1.10 -Xlinker -dependency_info -Xlinker /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/WebKit.build/WebKit.build/Objects-normal/arm64e/WebKit_dependency_info.dat -o /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-thin-WD-WD/WebKit2_WebKit.roots/BuildRecords/WebKit2_install/TempContent/Objects/WebKit.build/WebKit.build/Objects-normal/arm64e/WebKit
invalid llvm.ptrauth global: global doesn't have an initializer
{ i8*, i32, i64, i64 }* @_ZN3WTF8in_placeILm0EEENS_12in_place_tagERNS_18__in_place_private14__value_holderIXT_EEE.ptrauth.llvm.6030729957940808222
LLVM ERROR: Broken module found, compilation aborted!
clang: error: linker command failed with exit code 1 (use -v to see invocation)


For Full LTO, it gets past that but then fails at:

GenerateDSYMFile /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-full-WD-WD/WebCore.roots/BuildRecords/WebCore_install/Symbols/WebCore.framework.dSYM /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-full-WD-WD/WebCore.roots/BuildRecords/WebCore_install/Root/System/Library/PrivateFrameworks/WebCore.framework/WebCore
    cd /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-full-WD-WD/WebCore.roots/Sources/WebCore
    export PATH="/Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/usr/bin:/Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/usr/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"
    /Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/Toolchains/iOS12.2.xctoolchain/usr/bin/dsymutil /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-full-WD-WD/WebCore.roots/BuildRecords/WebCore_install/Root/System/Library/PrivateFrameworks/WebCore.framework/WebCore -o /Volumes/Data/dev/webkit/branches/enable-lto/_archives/ios-production-full-WD-WD/WebCore.roots/BuildRecords/WebCore_install/Symbols/WebCore.framework.dSYM
Command /Volumes/Xcode10P107d_m17A405_m18E221a_i16E227_Boost_19GB/Xcode.app/Contents/Developer/Toolchains/iOS12.2.xctoolchain/usr/bin/dsymutil failed with exit code 10

[Keith Rollin]

It's failing for arm64e, but not arm64.

[Keith Rollin]

Created attachment 365450 [details]
Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later.

[EWS Watchlist]

Comment on attachment 365450 [details]
Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later.

Attachment 365450 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11593723

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 365508 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Keith Rollin]

The bot-reported error is because the following is failing to load the AVFoundation:

SOFT_LINK_CONSTANT(AVFoundation, AVEncoderBitRateKey, NSString *)

Testing locally, both with and without LTO, I get the following error for http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html:

+ CONSOLE MESSAGE: line 2659: Error: assert_approx_equals: Red channel of the pixel at (25, 25) expected 255 +/- 20 but got 0
  
  
- PASS MediaRecorder can successfully record the video for a audio-video stream 
+ Harness Error (FAIL), message = Error: assert_approx_equals: Red channel of the pixel at (25, 25) expected 255 +/- 20 but got 0
  
+ TIMEOUT MediaRecorder can successfully record the video for a audio-video stream Test timed out
+

[Keith Rollin]

Weird. Sometimes that test gives this slightly different error:

+ CONSOLE MESSAGE: line 2659: Error: assert_approx_equals: Red channel of the pixel at (20, 20) expected 0 +/- 20 but got 254


- PASS MediaRecorder can successfully record the video for a audio-video stream 
+ Harness Error (FAIL), message = Error: assert_approx_equals: Red channel of the pixel at (20, 20) expected 0 +/- 20 but got 254

+ TIMEOUT MediaRecorder can successfully record the video for a audio-video stream Test timed out
+

[Keith Rollin]

There should be no way that the LTO changes affected the AVEncoderBitRateKey error. LTO should be disabled for high Sierra because it comes with Xcode 9.3. LTO is enabled only with Xcode 10.2 or later.

[Keith Rollin]

I've checked that, according to the SDK, AVEncoderBitRateKey exists in AVFoundation in 10.13.

[Keith Rollin]

AVFoundation exists on 10.13. AVEncoderBitRateKey exists within it. Though that second part should not be relevant, since the error is with loading the library, not any symbol within the library.

[Keith Rollin]

Eric Carlson tells me that the issue with AVFoundation is a pre-existing known issue: <rdar://problem/47483831>. So the posted patch should be vindicated and OK to review and land.

[Keith Rollin]

Per the email thread Alexey mentioned, the Production build time hit is acceptable.

[Daniel Bates]

Comment on attachment 365450 [details]
Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later.

View in context: https://bugs.webkit.org/attachment.cgi?id=365450&action=review

Ok as-s. No need to address comments, though addressing them would make this patch better in my opinion. cq- for you to contemplate. My comments apply to each file, including ChangeLogs in this patch..

> Source/ThirdParty/ANGLE/ChangeLog:24
> +        - If building with `build-root`, specify --lto={non,thin,full} on the

non

> Source/JavaScriptCore/Configurations/Base.xcconfig:182
> +// Disable on all platforms other than macos, due to rdar://problem/49013399

macos

missing period at the end. <> around URL

> Source/JavaScriptCore/Configurations/Base.xcconfig:192
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1010 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1000 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000 = $(WK_XCODE_VERSION_BEFORE_10_2_1000_$(XCODE_VERSION_MINOR));

Ok as-is. Could be ordered based on increasing major version.

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:84
> +// Disable on all platforms other than macos, due to rdar://problem/49013399

See comments above.

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:93
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1010 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1000 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000 = $(WK_XCODE_VERSION_BEFORE_10_2_1000_$(XCODE_VERSION_MINOR));

Ok as-is. Same ordering suggestion as above.

[Keith Rollin]

Created attachment 365750 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365750 [details]
Patch

Clearing flags on attachment: 365750

Committed r243396: <https://trac.webkit.org/changeset/243396>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.