[issue38249] Optimize out Py_UNREACHABLE in the release mode

[STINNER Victor]

STINNER Victor <vstinner at python.org> added the comment:

What does __builtin_unreachable()? Does it nothing? Call abort()?


> Py_UNREACHABLE is used to indicate that a specific point in the program cannot be reached, even if the compiler might otherwise think it can.

I disagree here.
https://docs.python.org/dev/c-api/intro.html#c.Py_UNREACHABLE

For me, it's not an issue with the compiler, but more about handling corner cases. Py_UNREACHABLE() is used in cases which "should not occur" but can technically occur if you pass invalid data, or in "very unlikely case".

I looked at the Python code base.

tracemalloc_realloc():

        if (ADD_TRACE(ptr2, new_size) < 0) {
            /* Memory allocation failed. The error cannot be reported to
               the caller, because realloc() may already have shrunk the
               memory block and so removed bytes.

               This case is very unlikely: a hash entry has just been
               released, so the hash table should have at least one free entry.

               The GIL and the table lock ensures that only one thread is
               allocating memory. */
            Py_UNREACHABLE();
        }

Technically, Py_UNREACHABLE() can be reached if there is very low available memory, but it "should not".

dictobject.c:

static Py_ssize_t
lookdict_index(PyDictKeysObject *k, Py_hash_t hash, Py_ssize_t index)
{
    size_t mask = DK_MASK(k);
    size_t perturb = (size_t)hash;
    size_t i = (size_t)hash & mask;

    for (;;) {
        Py_ssize_t ix = dictkeys_get_index(k, i);
        if (ix == index) {
            return i;
        }
        if (ix == DKIX_EMPTY) {
            return DKIX_EMPTY;
        }
        perturb >>= PERTURB_SHIFT;
        i = mask & (i*5 + perturb + 1);
    }
    Py_UNREACHABLE();
}

Here it's unclear to me if this case can be reached or not.

_PyLong_Format() calls Py_UNREACHABLE() if base is not in (2, 8, 16).

long_bitwise() calls Py_UNREACHABLE() if op is not in ('&', '^', '|'). Technically, this case cannot occur, since it's static function which has exactly 3 call sites all with valid 'op'. This *very specific* case could use __builtin_unreachable().

pytime.c:

_PyTime_t
_PyTime_GetSystemClock(void)
{
    _PyTime_t t;
    if (pygettimeofday(&t, NULL, 0) < 0) {
        /* should not happen, _PyTime_Init() checked the clock at startup */
        Py_UNREACHABLE();
    }
    return t;
}

_PyTime_t
_PyTime_GetPerfCounter(void)
{
    _PyTime_t t;
    if (_PyTime_GetPerfCounterWithInfo(&t, NULL)) {
        Py_UNREACHABLE();
    }
    return t;
}

Clocks should not fail, but if they fail I would prefer to call Py_FatalError() to collect the traceback where the bug occurred.


> This is exact the case for __builtin_unreachable in GCC and Clang. I propose to extend Py_UNREACHABLE() to __builtin_unreachable() in the release mode. This will allow the compiler to generate more efficient code.
>
> If there are circumstances in which Py_UNREACHABLE() is reachable, then it is improper use of Py_UNREACHABLE(). It should be replaced with raising an appropriate exception (like TypeError, ValueError, RuntimeError or SystemError) or, in extreme cases, with explicit Py_FatalError()
 
If you consider that it's really worth it to optimize Py_UNREACHABLE(), I would prefer to have a new macro which is only used for compiler warnings: for cases which really "cannot happen".

But it sounds hard to ensure that bugs cannot happen. I'm not sure that it's worth it to optimize Py_UNREACHABLE(). IMHO the current implementation is a good tradeoff between correctness and performance.

----------

_______________________________________
Python tracker <report at bugs.python.org>
<https://bugs.python.org/issue38249>
_______________________________________