[PATCH] Properly implement Py_UNREACHABLE macro using autoconf.

[congma]

BPO	43615
Nosy	@vstinner, @congma
Files	0001-Properly-implement-Py_UNREACHABLE-macro-using-autoco.patch: Patch to configure.ac and Include/pymacro.h

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-03-24.14:34:32.815>
created_at = <Date 2021-03-24.13:59:19.771>
labels = ['type-feature', 'invalid', 'build']
title = '[PATCH] Properly implement Py_UNREACHABLE macro using autoconf.'
updated_at = <Date 2021-03-24.14:54:02.443>
user = 'https://github.com/congma'

bugs.python.org fields:

activity = <Date 2021-03-24.14:54:02.443>
actor = 'congma'
assignee = 'none'
closed = True
closed_date = <Date 2021-03-24.14:34:32.815>
closer = 'congma'
components = ['Build']
creation = <Date 2021-03-24.13:59:19.771>
creator = 'congma'
dependencies = []
files = ['49910']
hgrepos = []
issue_num = 43615
keywords = ['patch']
message_count = 7.0
messages = ['389454', '389455', '389457', '389458', '389459', '389461', '389464']
nosy_count = 2.0
nosy_names = ['vstinner', 'congma']
pr_nums = []
priority = 'normal'
resolution = 'not a bug'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue43615'
versions = []

[congma]

(This is a summarized form of the commit message in the attached patch. I'm submitting a patch instead of a PR over GitHub, because it seems that the autoreconf output files are part of the repository. In order for the changes to take effect in the repo, I may have to run autoreconf and add the clobbered output files to the repo, which I don't think is a good idea. Also on my system the autoreconf can only work correctly if I add a missing M4 file "ax_check_compile_flag.m4" from the Autoconf Archive https://www.gnu.org/software/autoconf-archive/ax_check_compile_flag.html for the AX_CHECK_COMPILE_FLAG macro used in the existing configure.ac. I don't think it's wise for me to introduce so many changes at once if most developers don't need to run autoreconf often.)

The problem
-----------

Definition of the Py_UNREACHABLE() macro relied on testing compiler versions in preprocessor directives. This is unreliable chiefly because compilers masquerade as each other.

The current implementation tests the __GNUC__ and __GNUC_MINOR__ macros as the logic (GCC version >= 4.5) for determining whether the compiler intrinsic __builtin_unreachable() is present (see commits eebaa9b, 24ba3b0). However, Clang defines these macros too and can cause confusion. Clang 11 pretends to be GCC 4.2.1 in its predefined macros. As a result, Clang won't use the intrinsic even if it's supported. This doesn't seem to match the intent behind the original implementation.

The solution
------------

Test the presence of the compiler-builtin __builtin_unreachable() at configure-time using Autoconf, and conditionally define the Py_UNREACHABLE() macro depending on the configuration.

The idea is based on the ax_gcc_builtin.m4 code [0] by Gabriele Svelto.

Alternative ideas
-----------------

Recent versions of Clang and GCC support the __has_builtin() macro.
However, this may be unreliable before Clang 10 [1], while GCC support is only available as of GCC 10 and its semantics may not be the same as Clang's [2]. Therefore __has_builtin() may not be as useful as it seems.

We may attempt to improve the accuracy of version checking in #if directives, but this could be brittle and difficult to explain, verify, or maintain.

Links
-----

[0] https://www.gnu.org/software/autoconf-archive/ax_gcc_builtin.html
[1] https://clang.llvm.org/docs/LanguageExtensions.html#has-builtin
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66970#c24

[vstinner]

The current implementation tests the __GNUC__ and __GNUC_MINOR__ macros as the logic (GCC version >= 4.5) for determining whether the compiler intrinsic __builtin_unreachable() is present (see commits eebaa9b, 24ba3b0). However, Clang defines these macros too and can cause confusion. Clang 11 pretends to be GCC 4.2.1 in its predefined macros. As a result, Clang won't use the intrinsic even if it's supported. This doesn't seem to match the intent behind the original implementation.

Hum. The current code is:

#elif defined(GNUC) && (GNUC > 4 || (GNUC == 4 && __GNUC_MINOR__ >= 5))
# define Py_UNREACHABLE() __builtin_unreachable()
#elif defined(clang) || defined(__INTEL_COMPILER)
# define Py_UNREACHABLE() __builtin_unreachable()

Even if clang pretends to be GCC 4.2 and the first test fails, the second test is true and so Py_UNREACHABLE() is always defined as __builtin_unreachable() on clang, no?

You can please better explain your problem? You can write a short C code demonstrating the bug?

[congma]

Hello Victor,

I think you're right. This is bogus on my part. TL;DR: The Python version is 3.8 but I was trying to understand what's going on using the latest source.

Full version: I was trying to understand why the following C file when compiled with -shared using Clang 11 generates a call to Py_FatalError():

#define PY_SSIZE_T_CLEAN
#include "Python.h"


void
unreach(void)
{
    Py_UNREACHABLE();
}

The headers Python.h and also the ones pulled in by it were actually from Python 3.8 release, which unconditionally defines the macro as a call to Py_FatalError.

[congma]

BTW, do we need to fix the missing definition of the AX_CHECK_COMPILE_FLAG macro in configure.ac? This is a separate problem, if a problem at all.

[vstinner]

The headers Python.h and also the ones pulled in by it were actually from Python 3.8 release, which unconditionally defines the macro as a call to Py_FatalError.

Using __builtin_unreachable() is a recent change (bpo-38249), Python 3.9.0:

• commit eebaa9b

Fix for old GCC (bpo-41875):

• master: commit 24ba3b0
• 3.9.1: commit cca896e

I prefer to not backport this feature, since, as you wrote, it's tricky to make sure that it's available on the C compiler.

I close the issue since Py_UNREACHABLE() works as expected on Python 3.9 and newer.

[vstinner]

BTW, do we need to fix the missing definition of the AX_CHECK_COMPILE_FLAG macro in configure.ac? This is a separate problem, if a problem at all.

I'm not sure which problem you are trying to solve. If you consider that there is a bug, please open a new issue since you closed this one.

[congma]

If you consider that there is a bug, please open a new issue since you closed this one.

Please see the follow up in bpo-43617.

Many thanks for bearing with me.