[issue43615] [PATCH] Properly implement Py_UNREACHABLE macro using autoconf.

[Cong Ma]

Cong Ma  added the comment:

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

Please see the follow up in Issue 43617.

Many thanks for bearing with me.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue43615] [PATCH] Properly implement Py_UNREACHABLE macro using autoconf.

[STINNER Victor]

STINNER Victor  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue43615] [PATCH] Properly implement Py_UNREACHABLE macro using autoconf.

[STINNER Victor]

STINNER Victor  added the comment:

> 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 eebaa9bfc593d5a46b293c1abd929fbfbfd28199

Fix for old GCC (bpo-41875):

* master: commit 24ba3b0df5e5f2f237d7b23b4017ba12f16320ae
* 3.9.1: commit cca896e13b230a934fdb709b7f10c5451ffc25ba

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.

--
stage: resolved -> 

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue43615] [PATCH] Properly implement Py_UNREACHABLE macro using autoconf.

[Cong Ma]

Cong Ma  added the comment:

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.

--
resolution:  -> not a bug
stage:  -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue43615] [PATCH] Properly implement Py_UNREACHABLE macro using autoconf.

[Cong Ma]

Cong Ma  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue43615] [PATCH] Properly implement Py_UNREACHABLE macro using autoconf.

[STINNER Victor]

STINNER Victor  added the comment:

> 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 eebaa9bf, 
> 24ba3b0d). 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?

--
nosy: +vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue43615] [PATCH] Properly implement Py_UNREACHABLE macro using autoconf.

[Cong Ma]

New submission from Cong Ma :

(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 
 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 eebaa9bf, 
24ba3b0d). 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

--
components: Build
files: 0001-Properly-implement-Py_UNREACHABLE-macro-using-autoco.patch
keywords: patch
messages: 389454
nosy: congma
priority: normal
severity: normal
status: open
title: [PATCH] Properly implement Py_UNREACHABLE macro using autoconf.
type: enhancement
Added file: 
https://bugs.python.org/file49910/0001-Properly-implement-Py_UNREACHABLE-macro-using-autoco.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com