Bug 190758

Summary: Re-enable LTO support
Product: WebKit Reporter: Keith Rollin <krollin>
Component: Tools / TestsAssignee: Keith Rollin <krollin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, commit-queue, dbates, dino, eric.carlson, ews-watchlist, graouts, keith_miller, kondapallykalyan, lforschler, mark.lam, msaboff, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later.
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Patch none

Keith Rollin
Reported 2018-10-19 12:26:32 PDT
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.
Attachments
Patch (24.25 KB, patch)
2019-03-18 15:41 PDT, Keith Rollin
no flags
Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later. (35.81 KB, patch)
2019-03-20 17:54 PDT, Keith Rollin
no flags
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.63 MB, application/zip)
2019-03-21 00:11 PDT, EWS Watchlist
no flags
Patch (35.83 KB, patch)
2019-03-22 11:29 PDT, Keith Rollin
no flags
Radar WebKit Bug Importer
Comment 1 2018-10-19 12:26:47 PDT
Alexey Proskuryakov
Comment 2 2018-10-19 12:37:08 PDT
Do we have a consensus that other consequences (like build time impact) are acceptable to the project?
Keith Rollin
Comment 3 2018-10-19 13:22:39 PDT
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
Comment 4 2018-10-25 10:19:04 PDT
IIRC another objection was that people working on performance weren't OK with having different performance characteristics in release and production builds.
Keith Rollin
Comment 5 2019-03-18 15:41:46 PDT
Keith Rollin
Comment 6 2019-03-18 15:45:18 PDT
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
Comment 7 2019-03-18 15:45:48 PDT
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Alexey Proskuryakov
Comment 8 2019-03-18 16:02:23 PDT
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
Comment 9 2019-03-19 00:55:18 PDT
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
Comment 10 2019-03-19 02:45:32 PDT
It's failing for arm64e, but not arm64.
Keith Rollin
Comment 11 2019-03-20 17:54:25 PDT
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 12 2019-03-21 00:11:13 PDT
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
Comment 13 2019-03-21 00:11:15 PDT
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
Comment 14 2019-03-21 10:56:10 PDT
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
Comment 15 2019-03-21 12:12:37 PDT
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
Comment 16 2019-03-21 13:39:02 PDT
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
Comment 17 2019-03-21 13:40:27 PDT
I've checked that, according to the SDK, AVEncoderBitRateKey exists in AVFoundation in 10.13.
Keith Rollin
Comment 18 2019-03-21 13:50:00 PDT
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
Comment 19 2019-03-21 14:17:08 PDT
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
Comment 20 2019-03-21 14:43:01 PDT
Per the email thread Alexey mentioned, the Production build time hit is acceptable.
Daniel Bates
Comment 21 2019-03-21 20:36:40 PDT
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
Comment 22 2019-03-22 11:29:46 PDT
WebKit Commit Bot
Comment 23 2019-03-22 11:56:47 PDT
Comment on attachment 365750 [details] Patch Clearing flags on attachment: 365750 Committed r243396: <https://trac.webkit.org/changeset/243396>
WebKit Commit Bot
Comment 24 2019-03-22 11:56:49 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.