Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

P0627R6: Function to mark unreachable code #2526

Merged
merged 13 commits into from
Feb 12, 2022
Merged

P0627R6: Function to mark unreachable code #2526

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ab32fa7
unreachable code
AlexGuteniev Feb 7, 2022
7b03bb9
(c)
AlexGuteniev Feb 7, 2022
27d7a8c
inline
AlexGuteniev Feb 7, 2022
6317604
Apply suggestions from code review
AlexGuteniev Feb 7, 2022
b234d9b
[[noreturn]] , noexcept /* strengthened */
AlexGuteniev Feb 7, 2022
d183d52
__builtin_unreachable
AlexGuteniev Feb 8, 2022
04e36c8
debugging experience
AlexGuteniev Feb 8, 2022
6d9de6f
it wouldn't hurt
AlexGuteniev Feb 8, 2022
b564b0d
Use _STL_UNREACHABLE and don't call abort()
AlexGuteniev Feb 9, 2022
e9f57df
unreachable()
AlexGuteniev Feb 9, 2022
ea8eb26
char
AlexGuteniev Feb 9, 2022
68dc5a7
hail undefined behavior!
AlexGuteniev Feb 9, 2022
e64e511
to be (or not to be)
AlexGuteniev Feb 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions stl/inc/utility
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,18 @@ template <class _Ty>
_NODISCARD constexpr underlying_type_t<_Ty> to_underlying(_Ty _Value) noexcept {
return static_cast<underlying_type_t<_Ty>>(_Value);
}

[[noreturn]] inline void unreachable() noexcept /* strengthened */ {
#ifdef _DEBUG
_CSTD abort();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is a great idea. abort is well-defined, and therefore doesn't poison the code path leading to it with undefined behavior. That seems like a very subtle difference that folks may not be expecting when they flip the debug/release switch. (I'm going to call this "Comment" rather than "Request change" to get more reviewers' opinions.)

Copy link
Contributor Author

@AlexGuteniev AlexGuteniev Feb 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I observed that __assume(false) does provide UB in debug. Up to this

int main()
{
   __assume(false);
   // does not necessarily return zero, even in /Od
}

Normally I'd except debug mode to catch any UBs that can be caught.

Also look into ATLASSUME for a precedent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexGuteniev I really like your idea of using _STL_UNREACHABLE here as we're already using it in product code for this exact purpose (in <format> and <variant>).

@CaseyCarter I share your concern about well-defined behavior. How would you feel about:

_STL_UNREACHABLE;
#ifdef _DEBUG
_CSTD abort();
#endif // _DEBUG

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the best of both worlds. +1 from me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

_STL_UNREACHABLE;
#ifdef _DEBUG
_CSTD abort();
#endif // _DEBUG

Is not really a good option.
By having abort(); after _STL_UNREACHABLE; you ask for UB, and compiler may implement the UB by not actually calling abort, and in /O2 it really does! _DEBUG may be used with /O2, and I don't think #pragma optimize is good here.

If you want to enjoy UB even in _DEBUG here, I'd rather leave just __assume(false)/__builtin_unreachable(), wrapped in _STL_UNREACHABLE;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible way to be able to debug the unreachable is to guard the original _STL_UNREACHABLE in #ifndef _STL_UNREACHABLE. This way a user may do #define _STL_UNREACHABLE abort(), #define _STL_UNREACHABLE __debugbreak() or #define _STL_UNREACHABLE __ud2(). Still it is arguable place for a customization; if we decide to embrace the UB, let's have it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By having abort(); after _STL_UNREACHABLE; you ask for UB, and compiler may implement the UB by not actually calling abort, and in /O2 it really does!

I'm fine with this, it is working as intended. The goal of putting abort here is not to define the behavior, it's to increase the likelihood that the outcome of the UB is to crash near to the occurrence of unreachable in the user's code. That outcome isn't deterministic and can't be made deterministic without breaking the intended functionality. I don't agree that the lack of determinism makes the abort call useless.

A possible way to be able to debug the unreachable is to guard the original _STL_UNREACHABLE in #ifndef _STL_UNREACHABLE.

If people sometimes want to not use unreachable they can define their own macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review the comment I added next to abort().
(I don't think we can leave the code with deliberate UB without a comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also what exactly the semantic of _STL_UNREACHABLE?
I mean if this is precondition and not internal invariant, maybe the abort() should rather be embedded there rather than here?

#else // ^^^ _DEBUG ^^^ / vvv not _DEBUG vvv
#ifdef __clang__
__builtin_unreachable();
#else // ^^^ __clang__ ^^^ // vvv not __clang__ vvv
__assume(false);
AlexGuteniev marked this conversation as resolved.
Show resolved Hide resolved
#endif // ^^^ not __clang__ ^^^
#endif // ^^^ not _DEBUG ^^^
}
#endif // _HAS_CXX23

#if _HAS_TR1_NAMESPACE
Expand Down
2 changes: 2 additions & 0 deletions stl/inc/yvals_core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@
// P0288R9 move_only_function
// P0401R6 Providing Size Feedback In The Allocator Interface
// P0448R4 <spanstream>
// P0627R6 Function To Mark Unreachable Code
AlexGuteniev marked this conversation as resolved.
Show resolved Hide resolved
// P0798R8 Monadic Operations For optional
// P0943R6 Supporting C Atomics In C++
// P1048R1 is_scoped_enum
Expand Down Expand Up @@ -1418,6 +1419,7 @@
#define __cpp_lib_string_contains 202011L
#define __cpp_lib_string_resize_and_overwrite 202110L
#define __cpp_lib_to_underlying 202102L
#define __cpp_lib_unreachable 202202L
#endif // _HAS_CXX23

#define __cpp_lib_experimental_erase_if 201411L
Expand Down
1 change: 1 addition & 0 deletions tests/std/test.lst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ tests\P0595R2_is_constant_evaluated
tests\P0607R0_inline_variables
tests\P0608R3_improved_variant_converting_constructor
tests\P0616R0_using_move_in_numeric
tests\P0627R6_unreachable
tests\P0631R8_numbers_math_constants
tests\P0645R10_text_formatting_args
tests\P0645R10_text_formatting_custom_formatting
Expand Down
4 changes: 4 additions & 0 deletions tests/std/tests/P0627R6_unreachable/env.lst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Copyright (c) Microsoft Corporation.
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

RUNALL_INCLUDE ..\usual_latest_matrix.lst
30 changes: 30 additions & 0 deletions tests/std/tests/P0627R6_unreachable/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright (c) Microsoft Corporation.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#include <cassert>
#include <utility>

constexpr int test_impl(int arg) {
AlexGuteniev marked this conversation as resolved.
Show resolved Hide resolved
switch (arg) {
case 1:
return 'x';

case 2:
return 'y';

default:
std::unreachable();
}
}

constexpr bool test() {
assert(test_impl(1) == 'x');
assert(test_impl(2) == 'y');
return true;
AlexGuteniev marked this conversation as resolved.
Show resolved Hide resolved
}

int main() {
test();
static_assert(test());
static_assert(noexcept(std::unreachable())); // strengthened
}
14 changes: 14 additions & 0 deletions tests/std/tests/VSO_0157762_feature_test_macros/test.compile.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1818,6 +1818,20 @@ STATIC_ASSERT(__cpp_lib_uncaught_exceptions == 201411L);
STATIC_ASSERT(__cpp_lib_unordered_map_try_emplace == 201411L);
#endif

#if _HAS_CXX23
#ifndef __cpp_lib_unreachable
#error __cpp_lib_unreachable is not defined
#elif __cpp_lib_unreachable != 202202L
#error __cpp_lib_unreachable is not 202202L
#else
STATIC_ASSERT(__cpp_lib_unreachable == 202202L);
#endif
#else
#ifdef __cpp_lib_unreachable
#error __cpp_lib_unreachable is defined
#endif
#endif

#if _HAS_CXX20
#ifndef __cpp_lib_unwrap_ref
#error __cpp_lib_unwrap_ref is not defined
Expand Down