Changeset 233391 in webkit


Ignore:
Timestamp:
Jun 29, 2018 11:42:04 PM (6 years ago)
Author:
commit-queue@webkit.org
Message:

WTF's internal std::optional implementation should abort() on bad optional access
https://bugs.webkit.org/show_bug.cgi?id=186536

Patch by Frederic Wang <fwang@igalia.com> on 2018-06-29
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 or #187145. 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:

Location:
trunk
Files:
5 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r233390 r233391  
     12018-06-29  Frederic Wang  <fwang@igalia.com>
     2
     3        WTF's internal std::optional implementation should abort() on bad optional access
     4        https://bugs.webkit.org/show_bug.cgi?id=186536
     5
     6        Reviewed by Michael Catanzaro.
     7
     8        * TestExpectations: Mark one WebAnimations test as crashing (bug #187145).
     9
    1102018-06-29  Nan Wang  <n_wang@apple.com>
    211
  • trunk/LayoutTests/TestExpectations

    r233379 r233391  
    386386# End platform-specific tests.
    387387#//////////////////////////////////////////////////////////////////////////////////////////
     388
     389webkit.org/b/187145 imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html [ Crash ]
    388390
    389391# media/video-seek-after-end.html is flaky
  • trunk/Source/WTF/ChangeLog

    r233380 r233391  
     12018-06-29  Frederic Wang  <fwang@igalia.com>
     2
     3        WTF's internal std::optional implementation should abort() on bad optional access
     4        https://bugs.webkit.org/show_bug.cgi?id=186536
     5
     6        Reviewed by Michael Catanzaro.
     7
     8        Currently, some ports built with recent compilers will cause the program to abort when one
     9        tries to access the value of an unset std:optional (i.e. std::nullopt) as specified by C++17.
     10        However, most ports still use WTF's internal std::optional implementation, which does not
     11        verify illegal access. Hence it's not possible for developers working on these ports to
     12        detect issues like bugs #186189, #186535, #186752, #186753, #187139 or #187145. WTF's version
     13        of std::optional was introduced in bug #164199 but it was not possible to verify the
     14        availability of the value inside constexpr member functions because the assert might involve
     15        asm declarations. This commit introduces a new RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT macro
     16        (a simplified version of RELEASE_ASSERT that can be used in constexpr context) and uses it in
     17        WTF's implementation of std::optional.
     18
     19        * wtf/Assertions.h: Define RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT as a version of
     20        RELEASE_ASSERT that can be used in constexpr context (in particular avoids asm declarations).
     21        * wtf/Optional.h:
     22        (std::optional::operator ->): Add an assert to ensure the optional value is available.
     23        (std::optional::operator *): Ditto.
     24        (std::optional::value const): Ditto.
     25        (std::optional::value): Ditto.
     26        (std::optional<T::value const): Ditto.
     27
    1282018-06-29  Darin Adler  <darin@apple.com>
    229
  • trunk/Source/WTF/wtf/Assertions.h

    r232734 r233391  
    221221#endif
    222222
     223#if COMPILER(MSVC)
     224#define WTFBreakpointTrapUnderConstexprContext() __debugbreak()
     225#else
     226#define WTFBreakpointTrapUnderConstexprContext() __builtin_trap()
     227#endif
     228
    223229#ifndef CRASH
    224230
     
    506512#endif
    507513
     514/* RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT
     515
     516   This is a special version of RELEASE_ASSERT(assertion) that can be used in constexpr contexts.
     517*/
     518#if ASSERT_DISABLED
     519#define RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion) do { \
     520    if (UNLIKELY(!(assertion))) { \
     521        WTFBreakpointTrapUnderConstexprContext(); \
     522        __builtin_unreachable(); \
     523    } \
     524} while (0)
     525#else
     526#define RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion) do { \
     527    if (!(assertion)) { \
     528        WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \
     529        WTFBreakpointTrapUnderConstexprContext(); \
     530        __builtin_unreachable(); \
     531    } \
     532} while (0)
     533#endif
     534
    508535/* UNREACHABLE_FOR_PLATFORM */
    509536
  • trunk/Source/WTF/wtf/Optional.h

    r231342 r233391  
    531531
    532532  OPTIONAL_MUTABLE_CONSTEXPR T* operator ->() {
    533     // FIXME: We need to offer special assert function that can be used under the contexpr context.
    534     // CONSTEXPR_ASSERT(initialized());
     533    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
    535534    return dataptr();
    536535  }
     
    541540
    542541  OPTIONAL_MUTABLE_CONSTEXPR T& operator *() & {
    543     // FIXME: We need to offer special assert function that can be used under the contexpr context.
    544     // CONSTEXPR_ASSERT(initialized());
     542    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
    545543    return contained_val();
    546544  }
    547545
    548546  OPTIONAL_MUTABLE_CONSTEXPR T&& operator *() && {
    549     // FIXME: We need to offer special assert function that can be used under the contexpr context.
    550     // CONSTEXPR_ASSERT(initialized());
     547    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
    551548    return detail_::constexpr_move(contained_val());
    552549  }
    553550
    554551  constexpr T const& value() const& {
    555     // FIXME: We need to offer special assert function that can be used under the contexpr context.
    556     // return initialized() ? contained_val() : (throw bad_optional_access("bad optional access"), contained_val());
     552    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
    557553    return contained_val();
    558554  }
    559555
    560556  OPTIONAL_MUTABLE_CONSTEXPR T& value() & {
    561     // FIXME: We need to offer special assert function that can be used under the contexpr context.
    562     // return initialized() ? contained_val() : (throw bad_optional_access("bad optional access"), contained_val());
     557    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
    563558    return contained_val();
    564559  }
    565560
    566561  OPTIONAL_MUTABLE_CONSTEXPR T&& value() && {
    567     // FIXME: We need to offer special assert function that can be used under the contexpr context.
    568     // if (!initialized()) __THROW_EXCEPTION(bad_optional_access("bad optional access"));
     562    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
    569563    return std::move(contained_val());
    570564  }
     
    684678
    685679  constexpr T& value() const {
    686     // FIXME: We need to offer special assert function that can be used under the contexpr context.
    687     // return ref ? *ref : (throw bad_optional_access("bad optional access"), *ref);
     680    RELEASE_ASSERT_UNDER_CONSTEXPR_CONTEXT(ref());
    688681    return *ref;
    689682  }
Note: See TracChangeset for help on using the changeset viewer.