P0627R6: Function to mark unreachable code

[AlexGuteniev]

Resolves #2531

[CaseyCarter]

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

[AlexGuteniev]

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.

[StephanTLavavej]

@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

[CaseyCarter]

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

[AlexGuteniev]

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

[AlexGuteniev]

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.

[CaseyCarter]

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.

[AlexGuteniev]

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

[AlexGuteniev]

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?

[AlexGuteniev]

Maybe should reuse:

STL/stl/inc/yvals_core.h

Lines 1500 to 1504 in ee6a79e

	#ifdef __clang__
	#define _STL_UNREACHABLE __builtin_unreachable()
	#else // ^^^ clang ^^^ / vvv other vvv
	#define _STL_UNREACHABLE __assume(false)
	#endif // __clang__

[StephanTLavavej]

@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

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[StephanTLavavej]

Thanks for reaching this unreachable feature so quickly! 😹 🚀 🎉