Currently GTK/WPE when built with GCC 7 and higher are suffering from issues like bug #186535 and bug #186189. It's impossible for Apple developers to notice the problems because Apple is still using WTF's internal std::optional, which allows accessing the value of std::optional when it is unset (std::nullopt), which is illegal and causes the program to abort using the standard std::optional. i.e., we need to fix these FIXMEs in wtf/Optional.h: OPTIONAL_MUTABLE_CONSTEXPR T& operator *() & { // FIXME: We need to offer special assert function that can be used under the contexpr context. // CONSTEXPR_ASSERT(initialized()); return contained_val(); } OPTIONAL_MUTABLE_CONSTEXPR T&& operator *() && { // FIXME: We need to offer special assert function that can be used under the contexpr context. // CONSTEXPR_ASSERT(initialized()); return detail_::constexpr_move(contained_val()); } constexpr T const& value() const& { // FIXME: We need to offer special assert function that can be used under the contexpr context. // return initialized() ? contained_val() : (throw bad_optional_access("bad optional access"), contained_val()); return contained_val(); } OPTIONAL_MUTABLE_CONSTEXPR T& value() & { // FIXME: We need to offer special assert function that can be used under the contexpr context. // return initialized() ? contained_val() : (throw bad_optional_access("bad optional access"), contained_val()); return contained_val(); } OPTIONAL_MUTABLE_CONSTEXPR T&& value() && { // FIXME: We need to offer special assert function that can be used under the contexpr context. // if (!initialized()) __THROW_EXCEPTION(bad_optional_access("bad optional access")); return std::move(contained_val()); } constexpr T& value() const { // FIXME: We need to offer special assert function that can be used under the contexpr context. // return ref ? *ref : (throw bad_optional_access("bad optional access"), *ref); return *ref; }
I'm going to attach a speculative patch, which will probably neither build nor work. I can try setting up a build environment to test it in a VM another day.
Created attachment 342475 [details] Patch
Comment on attachment 342475 [details] Patch Attachment 342475 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/8137841 Number of test failures exceeded the failure limit.
Created attachment 342491 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Looks like we'll definitely need to fix bug #186189 first. No doubt there will be more issues.
Comment on attachment 342475 [details] Patch Attachment 342475 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8140364 New failing tests: http/tests/security/contentSecurityPolicy/script-src-redirect.html http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number.html
Created attachment 342505 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 343716 [details] Patch Resubmitting the same patch for another EWS run
Comment on attachment 343716 [details] Patch Attachment 343716 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8362922 New failing tests: imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html fast/css-grid-layout/flex-sizing-rows-min-max-height.html fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html
Created attachment 343763 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 343812 [details] Patch (defines a new ASSERT macro without asm statement)
Created attachment 343813 [details] Patch (defines a new ASSERT macro without asm statement)
Comment on attachment 343812 [details] Patch (defines a new ASSERT macro without asm statement) View in context: https://bugs.webkit.org/attachment.cgi?id=343812&action=review > Source/WTF/wtf/Assertions.h:350 > + __builtin_trap(); \ > + __builtin_unreachable(); \ This won't work for MSVCC, though.
(In reply to Michael Catanzaro from comment #13) > Comment on attachment 343812 [details] > Patch (defines a new ASSERT macro without asm statement) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343812&action=review > > > Source/WTF/wtf/Assertions.h:350 > > + __builtin_trap(); \ > > + __builtin_unreachable(); \ > > This won't work for MSVCC, though. Good point, I think we can use __debugbreak() instead. Let's see if that version builds on other platforms. I think we will still need to figure out what to do with the 4 crashes you got in our initial patch.
(In reply to Frédéric Wang (:fredw) from comment #14) > Good point, I think we can use __debugbreak() instead. Let's see if that > version builds on other platforms. I think we will still need to figure out > what to do with the 4 crashes you got in our initial patch. Let's just add Crash expectations for them, in the global TestExpectations file, pointing to other bugs that are not closed. We already have bug #186752 for the CSS grid crashes. We can simply leave a comment there that the expectations should be removed when the crash is fixed. That leaves only imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html. I've reported bug #187145 for it now.
Comment on attachment 343813 [details] Patch (defines a new ASSERT macro without asm statement) Attachment 343813 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8372067 New failing tests: imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html fast/css-grid-layout/flex-sizing-rows-min-max-height.html fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html
Created attachment 343840 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
The crash with http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html seems flacky on Windows. From a quick bugzilla search, it also happened in older bugs.
Created attachment 343898 [details] Patch
Comment on attachment 343898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343898&action=review > Source/WTF/ChangeLog:17 > + WTF's implementation of std::optional. Like other ASSERT_* macros, it's not used in release mode. Should we instead introduce a RELEASE_ASSERT_*? > Source/WTF/ChangeLog:22 > + (std::optional::operator ->): Add an assert to ensure the optional value is available. It seems the bad access check is actually not done for operator-> and operator* according to https://en.cppreference.com/w/cpp/utility/optional/operator*#Notes What do we prefer between making implementation consistent with C++17 and making check stricter?
(In reply to Frédéric Wang (:fredw) from comment #20) > Comment on attachment 343898 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343898&action=review > > > Source/WTF/ChangeLog:17 > > + WTF's implementation of std::optional. > > Like other ASSERT_* macros, it's not used in release mode. Should we instead > introduce a RELEASE_ASSERT_*? Yes, this should be a RELEASE_ASSERT() because the goal is for our std::optional to match the behavior of the real std::optional. > > Source/WTF/ChangeLog:22 > > + (std::optional::operator ->): Add an assert to ensure the optional value is available. > > It seems the bad access check is actually not done for operator-> and > operator* according to > https://en.cppreference.com/w/cpp/utility/optional/operator*#Notes > What do we prefer between making implementation consistent with C++17 and > making check stricter? Good catch. Sigh, it says the behavior is undefined in this case. :S We could go either way. We are allowed to keep the assert, because it says undefined, but I checked libstdc++ and it does not do so... probably best to match the behavior of the standard library.
Comment on attachment 343898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343898&action=review > LayoutTests/TestExpectations:394 > +webkit.org/b/186752 fast/css-grid-layout/flex-sizing-rows-min-max-height.html [ Pass Crash ] > +webkit.org/b/186752 fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html [ Pass Crash ] > +webkit.org/b/186752 fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html [ Pass Crash ] These tests are fixed in bug #186752, so remember to remove them from your patch.
Comment on attachment 343898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343898&action=review >>> Source/WTF/ChangeLog:17 >>> + WTF's implementation of std::optional. >> >> Like other ASSERT_* macros, it's not used in release mode. Should we instead introduce a RELEASE_ASSERT_*? > > Yes, this should be a RELEASE_ASSERT() because the goal is for our std::optional to match the behavior of the real std::optional. OK. I fear this is going to affect more people, but maybe not a bad thing... >>> Source/WTF/ChangeLog:22 >>> + (std::optional::operator ->): Add an assert to ensure the optional value is available. >> >> It seems the bad access check is actually not done for operator-> and operator* according to https://en.cppreference.com/w/cpp/utility/optional/operator*#Notes >> What do we prefer between making implementation consistent with C++17 and making check stricter? > > Good catch. Sigh, it says the behavior is undefined in this case. :S We could go either way. We are allowed to keep the assert, because it says undefined, but I checked libstdc++ and it does not do so... probably best to match the behavior of the standard library. OK, let's remove the assert for these cases.
Comment on attachment 343898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343898&action=review >> LayoutTests/TestExpectations:394 >> +webkit.org/b/186752 fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html [ Pass Crash ] > > These tests are fixed in bug #186752, so remember to remove them from your patch. Yes, I'm waiting for rego to land it (and maybe Antoine will fix webkit.org/b/187145 soon too).
Comment on attachment 343898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343898&action=review >>>> Source/WTF/ChangeLog:22 >>>> + (std::optional::operator ->): Add an assert to ensure the optional value is available. >>> >>> It seems the bad access check is actually not done for operator-> and operator* according to https://en.cppreference.com/w/cpp/utility/optional/operator*#Notes >>> What do we prefer between making implementation consistent with C++17 and making check stricter? >> >> Good catch. Sigh, it says the behavior is undefined in this case. :S We could go either way. We are allowed to keep the assert, because it says undefined, but I checked libstdc++ and it does not do so... probably best to match the behavior of the standard library. > > OK, let's remove the assert for these cases. On further thoughts, I see that we already assert in some cases and so does https://github.com/akrzemi1/Optional/blob/master/optional.hpp where the code was taken from. So let's keep the assert for consistency. If we want to match the behavior of the standard library, maybe we should do that in a follow-up bug.
Created attachment 344000 [details] Patch for landing
Comment on attachment 344000 [details] Patch for landing Clearing flags on attachment: 344000 Committed r233391: <https://trac.webkit.org/changeset/233391>
All reviewed patches have been landed. Closing bug.
<rdar://problem/41669560>
Re-opened since this is blocked by bug 187217
Created attachment 344007 [details] Patch
Comment on attachment 344007 [details] Patch Attachment 344007 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8393859 New failing tests: http/tests/cache-storage/cache-persistency.https.html
Created attachment 344009 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 344007 [details] Patch Attachment 344007 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8393899 New failing tests: http/tests/cache-storage/cache-persistency.https.html
Created attachment 344010 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 344081 [details] Patch for landing
Comment on attachment 344081 [details] Patch for landing Clearing flags on attachment: 344081 Committed r233417: <https://trac.webkit.org/changeset/233417>
Committed r233418: <https://trac.webkit.org/changeset/233418>
Re-opened since this is blocked by bug 187364
This patch introduced an occasional crash on Google Drive. When you navigate to drive.google.com with Safari, open sheets, and refresh drive.google.com 5-6 times, WebContent processes crash with the following trace. 0 com.apple.WebCore 0x00007fff53c378dd WebCore::AnimationBase::updateStateMachine(WebCore::AnimationBase::AnimationStateInput, double) + 3261 1 com.apple.WebCore 0x00007fff52c2cb3c WebCore::CompositeAnimation::resumeAnimations() + 460 2 com.apple.WebCore 0x00007fff53c3a1ae WebCore::CSSAnimationControllerPrivate::resumeAnimationsForDocument(WebCore::Document*) + 398 3 com.apple.WebCore 0x00007fff53c39fda WebCore::CSSAnimationControllerPrivate::resumeAnimations() + 42 4 com.apple.WebCore 0x00007fff53c05ad0 WebCore::Page::setIsVisibleInternal(bool) + 432 5 com.apple.WebCore 0x00007fff53c04835 WebCore::Page::setActivityState(unsigned int) + 85 6 com.apple.WebKit 0x00007fff547f6f55 WebKit::WebPage::setActivityState(unsigned int, bool, WTF::Vector<WebKit::CallbackID, 0ul, WTF::CrashOnOverflow, 16ul> const&) + 83 7 com.apple.WebKit 0x00007fff54813d95 WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&) + 4601 8 com.apple.WebKit 0x00007fff546275ff IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) + 127 9 com.apple.WebKit 0x00007fff54881f6e WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 28 10 com.apple.WebKit 0x00007fff545f488d IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 119 11 com.apple.WebKit 0x00007fff545f79d7 IPC::Connection::dispatchOneIncomingMessage() + 177 12 com.apple.JavaScriptCore 0x00007fff48bb8db9 WTF::RunLoop::performWork() + 313 13 com.apple.JavaScriptCore 0x00007fff48bb8ff2 WTF::RunLoop::performWork(void*) + 34 14 com.apple.CoreFoundation 0x7fff45548fcb __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1543.10/CoreFoundation/RunLoop.subproj/CFRunLoop.c:1980) 15 com.apple.CoreFoundation 0x7fff455e543f __CFRunLoopDoSource0 + 108 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1543.10/CoreFoundation/RunLoop.subproj/CFRunLoop.c:2015) 16 com.apple.CoreFoundation 0x7fff4553067f __CFRunLoopDoSources0 + 283 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1543.10/CoreFoundation/RunLoop.subproj/CFRunLoop.c:2059) 17 com.apple.CoreFoundation 0x7fff4552fbcc __CFRunLoopRun + 1219 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1543.10/CoreFoundation/RunLoop.subproj/CFRunLoop.c:2922) 18 com.apple.CoreFoundation 0x7fff4552f4f1 CFRunLoopRunSpecific + 463 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1543.10/CoreFoundation/RunLoop.subproj/CFRunLoop.c:3247) 19 com.apple.Foundation 0x7fff47924e85 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 280 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/Foundation/Foundation-1543.10/Foundation/Soil.subproj/NSRunLoop.m:367) 20 com.apple.Foundation 0x7fff47924d5a -[NSRunLoop(NSRunLoop) run] + 76 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/Foundation/Foundation-1543.10/Foundation/Soil.subproj/NSRunLoop.m:389) 21 libxpc.dylib 0x7fff72a53f8a _xpc_objc_main + 567 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libxpc/libxpc-1336.200.83/src/main.m:170) 22 libxpc.dylib 0x7fff72a52bde xpc_main + 443 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libxpc/libxpc-1336.200.83/src/init.c:1471) 23 com.apple.WebKit.WebContent 0x00000001022a763f 0x1022a6000 + 5695 24 libdyld.dylib 0x00007fff7280a36d start + 1
Also, adding a release assertion has a significant cost in terms of the binary size as well as runtime branch check. I don't think we should be adding these checks everywhere in release code in optional.h like this patch is doing.
(In reply to Ryosuke Niwa from comment #42) > Also, adding a release assertion has a significant cost in terms of the > binary size as well as runtime branch check. I don't think we should be > adding these checks everywhere in release code in optional.h like this patch > is doing. OK, that's what I was wondering in comment 20. I guess a debug release like in the initial patch will be enough... (In reply to Ryosuke Niwa from comment #41) > This patch introduced an occasional crash on Google Drive. > When you navigate to drive.google.com with Safari, open sheets, and refresh > drive.google.com 5-6 times, WebContent processes crash with the following > trace. > I'll take a look. The purpose of this patch is to verify illegal use of std::optional so this is probably an actual bug in the WebAnimation code that should be handled separately.
(In reply to Frédéric Wang (:fredw) from comment #43) > (In reply to Ryosuke Niwa from comment #42) > > Also, adding a release assertion has a significant cost in terms of the > > binary size as well as runtime branch check. I don't think we should be > > adding these checks everywhere in release code in optional.h like this patch > > is doing. > > OK, that's what I was wondering in comment 20. I guess a debug release like > in the initial patch will be enough... You mean debug assert? Yes, that would be the right thing to do here. > (In reply to Ryosuke Niwa from comment #41) > > This patch introduced an occasional crash on Google Drive. > > When you navigate to drive.google.com with Safari, open sheets, and refresh > > drive.google.com 5-6 times, WebContent processes crash with the following > > trace. > > > > I'll take a look. The purpose of this patch is to verify illegal use of > std::optional so this is probably an actual bug in the WebAnimation code > that should be handled separately. Indeed.
(In reply to Ryosuke Niwa from comment #44) > (In reply to Frédéric Wang (:fredw) from comment #43) > > (In reply to Ryosuke Niwa from comment #41) > > > This patch introduced an occasional crash on Google Drive. > > > When you navigate to drive.google.com with Safari, open sheets, and refresh > > > drive.google.com 5-6 times, WebContent processes crash with the following > > > trace. > > > > > > > I'll take a look. The purpose of this patch is to verify illegal use of > > std::optional so this is probably an actual bug in the WebAnimation code > > that should be handled separately. > > Indeed. This is the old animation code, not the Web Animations code. It’s going to be removed soon.
(In reply to Ryosuke Niwa from comment #44) > You mean debug assert? Yes, that would be the right thing to do here. I'll work on a new patch. (In reply to Antoine Quint from comment #45) > This is the old animation code, not the Web Animations code. It’s going to > be removed soon. Ah, my bad indeed it's AnimationBase. Do you mean it's not worth patching that code in the short term?
Created attachment 344412 [details] Patch
Comment on attachment 344412 [details] Patch Attachment 344412 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8454879 New failing tests: animations/needs-layout.html
Created attachment 344420 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
(In reply to Build Bot from comment #48) > New failing tests: > animations/needs-layout.html This flacky failure is bug 185659. The Windows bot win-ews is in release mode so the tests failures are unrelated.
Committed r233577: <https://trac.webkit.org/changeset/233577>
(In reply to Frédéric Wang (:fredw) from comment #51) > Committed r233577: <https://trac.webkit.org/changeset/233577> For future reference: If you detect new crashes after r233577 then it's likely to be an error in the code (illegal use of uninitialized std::optional value). Please fix them in separate bugs rather than reverting this commit. Thank you.
(In reply to Frédéric Wang (:fredw) from comment #52) > (In reply to Frédéric Wang (:fredw) from comment #51) > > Committed r233577: <https://trac.webkit.org/changeset/233577> > > For future reference: If you detect new crashes after r233577 then it's > likely to be an error in the code (illegal use of uninitialized > std::optional value). Please fix them in separate bugs rather than reverting > this commit. Thank you. Agreed
(In reply to Ryosuke Niwa from comment #42) > Also, adding a release assertion has a significant cost in terms of the > binary size as well as runtime branch check. I don't think we should be > adding these checks everywhere in release code in optional.h like this patch > is doing. The spec requires it to throw std::bad_optional_access. Since we compile with -fno-exceptions, that will be replaced with a call to std::abort(). A RELEASE_ASSERT() seems like a good equivalent to me. We do need to have the same behavior for all ports here. This std::optional behavior difference has been so exceptionally problematic that IMO we *really* need to switch back to C++ 14 if you're not OK with a release assert. CC: JF, please help us with this remaining fallout from the switch to C++ 17. ;)
Reopening.
(In reply to Ryosuke Niwa from comment #42) > Also, adding a release assertion has a significant cost in terms of the > binary size as well as runtime branch check. I don't think we should be > adding these checks everywhere in release code in optional.h like this patch > is doing. Please keep in mind that we don't control the C++ standard... you're going to get the runtime branch check no matter what when you enable C++ 17.
(In reply to Michael Catanzaro from comment #56) > (In reply to Ryosuke Niwa from comment #42) > > Also, adding a release assertion has a significant cost in terms of the > > binary size as well as runtime branch check. I don't think we should be > > adding these checks everywhere in release code in optional.h like this patch > > is doing. > > Please keep in mind that we don't control the C++ standard... you're going > to get the runtime branch check no matter what when you enable C++ 17. We could always deviate away from using optional from the stdlib if we think this runtime check is prohibitive. However, I agree that we should add a RELEASE_ASSERT to align with C++17 behavior if we decide we want to use the stdlib's optional.
Personally, I think the main issue here was that we were not catching illegal use of uninitalized std::optional (and in particular it was crashing on some platforms). So a debug assert seems good enough, assuming some developers & bots run debug builds. Deviating from the stdlib behavior (or using * or -> which don't assert) would avoid the crashes but also prevent us from detecting the bad usage, so I'm not sure it's a good idea :-) I don't have strong preference for what to do with release builds, but maybe this could be handled/discussed in a new separate bug?
Accessing the "wrong" optional field with * and -> is UB, so we can do whatever we want, including navigating to combo.com if we're so inclined. We're not deviating from the standard in doing so. I believe Darin was surprised a while ago when Dan made a change around this. Can't find the patch now, but it was either with optional or expected (which follow the same pattern). We had discussed, IIRC, making the UB always trap. Realistically if there's a cost that the compiler can't eliminate then it's either a compiler bug (please file a bug), or it's hard enough to prove that your code likely has or will have a bug in the future. In this case think it's an invalid argument to worry about code bloat without proof.
(In reply to JF Bastien from comment #59) > Accessing the "wrong" optional field with * and -> is UB, so we can do > whatever we want, including navigating to combo.com if we're so inclined. > We're not deviating from the standard in doing so. > > I believe Darin was surprised a while ago when Dan made a change around > this. Can't find the patch now, but it was either with optional or expected > (which follow the same pattern). We had discussed, IIRC, making the UB > always trap. Realistically if there's a cost that the compiler can't > eliminate then it's either a compiler bug (please file a bug), or it's hard > enough to prove that your code likely has or will have a bug in the future. > In this case think it's an invalid argument to worry about code bloat > without proof. Almost all optional code I’ve read already branches around the optional being set or not. I’d be surprised if the compiler didn’t eliminate the redundant check as long as things are properly inlined
(In reply to Saam Barati from comment #60) > (In reply to JF Bastien from comment #59) > > Accessing the "wrong" optional field with * and -> is UB, so we can do > > whatever we want, including navigating to combo.com if we're so inclined. > > We're not deviating from the standard in doing so. > > > > I believe Darin was surprised a while ago when Dan made a change around > > this. Can't find the patch now, but it was either with optional or expected > > (which follow the same pattern). We had discussed, IIRC, making the UB > > always trap. Realistically if there's a cost that the compiler can't > > eliminate then it's either a compiler bug (please file a bug), or it's hard > > enough to prove that your code likely has or will have a bug in the future. > > In this case think it's an invalid argument to worry about code bloat > > without proof. > > Almost all optional code I’ve read already branches around the optional > being set or not. I’d be surprised if the compiler didn’t eliminate the > redundant check as long as things are properly inlined Exactly. If the compiler can't eliminate the extra check then file a bug on the compiler. That being said, maybe don't call abort() and use CRASH() instead, so that in release builds it's a single instruction.
I still don't think we should be adding a release assertion.
(In reply to Frédéric Wang (:fredw) from comment #58) > I don't have strong preference for what to do with release builds, but maybe > this could be handled/discussed in a new separate bug? Let's move discussion for release in bug 187669.