WTF's internal std::optional implementation should abort() on bad optional access
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 2 Jul 2018 07:56:47 +0000 (07:56 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 2 Jul 2018 07:56:47 +0000 (07:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186536

Patch by Frederic Wang <fwang@igalia.com> on 2018-07-02
Reviewed by Michael Catanzaro.

Source/WTF:

Currently, some ports built with recent compilers will cause the program to abort when one
tries to access the value of an unset std:optional (i.e. std::nullopt) as specified by C++17.
However, most ports still use WTF's internal std::optional implementation, which does not
verify illegal access. Hence it's not possible for developers working on these ports to
detect issues like bugs #186189, #186535, #186752, #186753, #187139, #187145 or #187243.
WTF's version of std::optional was introduced in bug #164199 but it was not possible to
verify the availability of the value inside constexpr member functions because the assert
might involve asm declarations. This commit introduces a new
RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT macro (a simplified version of RELEASE_ASSERT that can
be used in constexpr context) and uses it in WTF's implementation of std::optional.

* wtf/Assertions.h: Define RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT as a version of
RELEASE_ASSERT that can be used in constexpr context (in particular avoids asm declarations).
* wtf/Optional.h:
(std::optional::operator ->): Add an assert to ensure the optional value is available.
(std::optional::operator *): Ditto.
(std::optional::value const): Ditto.
(std::optional::value): Ditto.
(std::optional<T::value const): Ditto.

LayoutTests:

* TestExpectations: Mark two tests as crashing (bug #187145 and bug #187243).

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233417 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
Source/WTF/ChangeLog
Source/WTF/wtf/Assertions.h
Source/WTF/wtf/Optional.h

index 6365c4a..68f6f2d 100644 (file)
@@ -1,3 +1,12 @@
+2018-07-02  Frederic Wang  <fwang@igalia.com>
+
+        WTF's internal std::optional implementation should abort() on bad optional access
+        https://bugs.webkit.org/show_bug.cgi?id=186536
+
+        Reviewed by Michael Catanzaro.
+
+        * TestExpectations: Mark two tests as crashing (bug #187145 and bug #187243).
+
 2018-07-01  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         [WK2] fast/parser/document-open-in-unload.html makes the following test crash
index e4fd0cf..591cb2c 100644 (file)
@@ -386,6 +386,9 @@ http/wpt/cross-origin-resource-policy/ [ Skip ]
 # End platform-specific tests.
 #//////////////////////////////////////////////////////////////////////////////////////////
 
+webkit.org/b/187145 imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html [ Crash ]
+webkit.org/b/187243 http/tests/cache-storage/cache-persistency.https.html [ Crash ]
+
 # media/video-seek-after-end.html is flaky
 webkit.org/b/116293 media/video-seek-after-end.html [ Pass Failure ]
 
index 64a7d1e..f98d603 100644 (file)
@@ -1,3 +1,30 @@
+2018-07-02  Frederic Wang  <fwang@igalia.com>
+
+        WTF's internal std::optional implementation should abort() on bad optional access
+        https://bugs.webkit.org/show_bug.cgi?id=186536
+
+        Reviewed by Michael Catanzaro.
+
+        Currently, some ports built with recent compilers will cause the program to abort when one
+        tries to access the value of an unset std:optional (i.e. std::nullopt) as specified by C++17.
+        However, most ports still use WTF's internal std::optional implementation, which does not
+        verify illegal access. Hence it's not possible for developers working on these ports to
+        detect issues like bugs #186189, #186535, #186752, #186753, #187139, #187145 or #187243.
+        WTF's version of std::optional was introduced in bug #164199 but it was not possible to
+        verify the availability of the value inside constexpr member functions because the assert
+        might involve asm declarations. This commit introduces a new
+        RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT macro (a simplified version of RELEASE_ASSERT that can
+        be used in constexpr context) and uses it in WTF's implementation of std::optional.
+
+        * wtf/Assertions.h: Define RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT as a version of
+        RELEASE_ASSERT that can be used in constexpr context (in particular avoids asm declarations).
+        * wtf/Optional.h:
+        (std::optional::operator ->): Add an assert to ensure the optional value is available.
+        (std::optional::operator *): Ditto.
+        (std::optional::value const): Ditto.
+        (std::optional::value): Ditto.
+        (std::optional<T::value const): Ditto.
+
 2018-07-01  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [WTF] RandomDevice should be initialized inside std::call_once
index 5e06eaa..99811cb 100644 (file)
@@ -220,6 +220,12 @@ WTF_EXPORT_PRIVATE bool WTFIsDebuggerAttached(void);
 #define WTFBreakpointTrap() WTFCrash() // Not implemented.
 #endif
 
+#if COMPILER(MSVC)
+#define WTFBreakpointTrapUnderConstexprContext() __debugbreak()
+#else
+#define WTFBreakpointTrapUnderConstexprContext() __builtin_trap()
+#endif
+
 #ifndef CRASH
 
 #if defined(NDEBUG) && OS(DARWIN)
@@ -230,8 +236,13 @@ WTF_EXPORT_PRIVATE bool WTFIsDebuggerAttached(void);
     WTFBreakpointTrap(); \
     __builtin_unreachable(); \
 } while (0)
+#define CRASH_UNDER_CONSTEXPR_CONTEXT() do { \
+    WTFBreakpointTrapUnderConstexprContext(); \
+    __builtin_unreachable(); \
+} while (0)
 #else
 #define CRASH() WTFCrash()
+#define CRASH_UNDER_CONSTEXPR_CONTEXT() WTFCrash()
 #endif
 
 #endif // !defined(CRASH)
@@ -505,6 +516,25 @@ WTF_EXPORT_PRIVATE NO_RETURN_DUE_TO_CRASH void WTFCrashWithSecurityImplication(v
 #define RELEASE_ASSERT_NOT_REACHED() ASSERT_NOT_REACHED()
 #endif
 
+/* RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT
+
+   This is a special version of RELEASE_ASSERT(assertion) that can be used in constexpr contexts.
+*/
+#if ASSERT_DISABLED
+#define RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion) do { \
+    if (UNLIKELY(!(assertion))) { \
+        CRASH_UNDER_CONSTEXPR_CONTEXT(); \
+    } \
+} while (0)
+#else
+#define RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion) do { \
+    if (!(assertion)) { \
+        WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \
+        CRASH_UNDER_CONSTEXPR_CONTEXT(); \
+    } \
+} while (0)
+#endif
+
 /* UNREACHABLE_FOR_PLATFORM */
 
 #if COMPILER(CLANG)
index 67ee277..c573dd9 100644 (file)
@@ -530,8 +530,7 @@ public:
   }
 
   OPTIONAL_MUTABLE_CONSTEXPR T* operator ->() {
-    // FIXME: We need to offer special assert function that can be used under the contexpr context.
-    // CONSTEXPR_ASSERT(initialized());
+    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
     return dataptr();
   }
 
@@ -540,32 +539,27 @@ public:
   }
 
   OPTIONAL_MUTABLE_CONSTEXPR T& operator *() & {
-    // FIXME: We need to offer special assert function that can be used under the contexpr context.
-    // CONSTEXPR_ASSERT(initialized());
+    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(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());
+    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(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());
+    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
     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());
+    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
     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"));
+    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
     return std::move(contained_val());
   }
 
@@ -683,8 +677,7 @@ public:
   }
 
   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);
+    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(ref());
     return *ref;
   }