Bug 186536 – WTF's internal std::optional implementation should abort() on bad optional access

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Michael Catanzaro]

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;
  }

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 342475 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Michael Catanzaro]

Looks like we'll definitely need to fix bug #186189 first. No doubt there will be more issues.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Michael Catanzaro]

Created attachment 343716 [details]
Patch

Resubmitting the same patch for another EWS run

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 343812 [details]
Patch (defines a new ASSERT macro without asm statement)

[Frédéric Wang (:fredw)]

Created attachment 343813 [details]
Patch (defines a new ASSERT macro without asm statement)

[Michael Catanzaro]

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.

[Frédéric Wang (:fredw)]

(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.

[Michael Catanzaro]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 343898 [details]
Patch

[Frédéric Wang (:fredw)]

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?

[Michael Catanzaro]

(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.

[Michael Catanzaro]

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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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).

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 344000 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 344000 [details]
Patch for landing

Clearing flags on attachment: 344000

Committed r233391: <https://trac.webkit.org/changeset/233391>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41669560>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 187217

[Frédéric Wang (:fredw)]

Created attachment 344007 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 344081 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 344081 [details]
Patch for landing

Clearing flags on attachment: 344081

Committed r233417: <https://trac.webkit.org/changeset/233417>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

Committed r233418: <https://trac.webkit.org/changeset/233418>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 187364

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Frédéric Wang (:fredw)]

(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.

[Ryosuke Niwa]

(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.

[Antoine Quint]

(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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 344412 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

Committed r233577: <https://trac.webkit.org/changeset/233577>

[Frédéric Wang (:fredw)]

(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.

[Saam Barati]

(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

[Michael Catanzaro]

(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. ;)

[Michael Catanzaro]

Reopening.

[Michael Catanzaro]

(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.

[Saam Barati]

(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.

[Frédéric Wang (:fredw)]

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?

[JF Bastien]

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.

[Saam Barati]

(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

[JF Bastien]

(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.

[Ryosuke Niwa]

I still don't think we should be adding a release assertion.

[Frédéric Wang (:fredw)]

(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.