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.
<rdar://problem/45413233>
Do we have a consensus that other consequences (like build time impact) are acceptable to the project?
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.
IIRC another objection was that people working on performance weren't OK with having different performance characteristics in release and production builds.
Created attachment 365078 [details] Patch
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.
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
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.
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
It's failing for arm64e, but not arm64.
Created attachment 365450 [details] Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later.
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
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
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 +
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 +
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.
I've checked that, according to the SDK, AVEncoderBitRateKey exists in AVFoundation in 10.13.
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.
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.
Per the email thread Alexey mentioned, the Production build time hit is acceptable.
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.
Created attachment 365750 [details] Patch
Comment on attachment 365750 [details] Patch Clearing flags on attachment: 365750 Committed r243396: <https://trac.webkit.org/changeset/243396>
All reviewed patches have been landed. Closing bug.