Issue 46513: AC_C_CHAR_UNSIGNED from configure.ac confuses GCC 12+ by defining __CHAR_UNSIGNED__
Created on 2022-01-25 11:28 by hroncok, last changed 2022-04-11 14:59 by admin. This issue is now closed.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 30851 | merged | christian.heimes, 2022-01-25 11:34 | |
| PR 30914 | merged | christian.heimes, 2022-01-26 09:06 | |
| PR 30915 | merged | christian.heimes, 2022-01-26 09:07 | |
| Messages (13) | |||
|---|---|---|---|
| msg411575 - (view) | Author: Miro Hrončok (hroncok) * | Date: 2022-01-25 11:28 | |
As described at https://mail.python.org/archives/list/python-dev@python.org/thread/MPHZ3TGSHMSF7C4P7JEP2ZCYLRA3ERC5/ the AC_C_CHAR_UNSIGNED macro from configure.ac confuses GCC 12+ as it exports a reserved symbol __CHAR_UNSIGNED__ through pyconfig.h. My assumption was that AC_C_CHAR_UNSIGNED can be dropped entirely and the PR in https://github.com/python/cpython/pull/30851 confirmed that all the buildbots are happy with such change. Hence, I open this issue to actually do it. Thanks |
|||
| msg411576 - (view) | Author: Christian Heimes (christian.heimes) * ![]() |
Date: 2022-01-25 11:35 | |
Modules/audioop.c uses __CHAR_UNSIGNED__, but the absence or presence of the symbol does not affect behavior: #if defined(__CHAR_UNSIGNED__) #if defined(signed) /* This module currently does not work on systems where only unsigned characters are available. Take it out of Setup. Sorry. */ #endif #endif |
|||
| msg411654 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2022-01-25 18:13 | |
Fedora downstream issue: https://bugzilla.redhat.com/show_bug.cgi?id=2043555 We should make sure that Python can be built with -fsigned-char *and* with -funsigned-char: build Python, but also run its test suite. |
|||
| msg411655 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2022-01-25 18:14 | |
> Modules/audioop.c uses __CHAR_UNSIGNED__, but the absence or presence of the symbol does not affect behavior This comment is really weird since audioop explicitly uses "signed char" and "unsigned char". Maybe to avoid any risk of confusion, the C code should use int8_t and uint8_t. |
|||
| msg411665 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2022-01-25 19:13 | |
To test if a C type is signed or not, I wrote this macro: // Test if a C type is signed. // // Usage: assert(_Py_CTYPE_IS_SIGNED(char)); // fail if 'char' type is unsigned #define _Py_CTYPE_IS_SIGNED(T) (((T)-1) < 0) I planned to use it to raise an error on "import audioop" if the C "char" type is unsigned, but it seems like it's not needed, since the C extensions seems to work if char is signed or unsigned (I only read the C code, I didn't run test_audioop to actually test it). |
|||
| msg411666 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2022-01-25 19:16 | |
My audioop.c change which looks to be wrong (useless): diff --git a/Modules/audioop.c b/Modules/audioop.c index 3aeb6f04f13..4c04b17ce7f 100644 --- a/Modules/audioop.c +++ b/Modules/audioop.c @@ -1948,6 +1941,13 @@ audioop_exec(PyObject* module) { audioop_state *state = get_audioop_state(module); + if (!_Py_CTYPE_IS_SIGNED(char)) { + PyErr_SetString(PyExc_RuntimeError, + "the audioop module does not support systems " + "where the char type is unsigned"); + return -1; + } + state->AudioopError = PyErr_NewException("audioop.error", NULL, NULL); if (state->AudioopError == NULL) { return -1; |
|||
| msg411718 - (view) | Author: miss-islington (miss-islington) | Date: 2022-01-26 09:03 | |
New changeset 6e5a193816e1bdf11f5fb78d620995fd6987ccf8 by Christian Heimes in branch 'main': bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (GH-30851) https://github.com/python/cpython/commit/6e5a193816e1bdf11f5fb78d620995fd6987ccf8 |
|||
| msg411719 - (view) | Author: Petr Viktorin (petr.viktorin) * ![]() |
Date: 2022-01-26 09:04 | |
Yeah, that looks like it's for some long-forgotten compiler that didn't implement `signed char` at all. 1994 was a fun time, apparently. |
|||
| msg411739 - (view) | Author: Christian Heimes (christian.heimes) * ![]() |
Date: 2022-01-26 11:21 | |
New changeset 4371fbd4328781496f5f2c6938c4d9a84049b187 by Christian Heimes in branch '3.10': [3.10] bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (GH-30851) (GH-30914) https://github.com/python/cpython/commit/4371fbd4328781496f5f2c6938c4d9a84049b187 |
|||
| msg411740 - (view) | Author: Christian Heimes (christian.heimes) * ![]() |
Date: 2022-01-26 11:21 | |
New changeset 04772cd6f164c1219c8c74d55626ba114f01aa96 by Christian Heimes in branch '3.9': [3.9] bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (GH-30851) (GH-30915) https://github.com/python/cpython/commit/04772cd6f164c1219c8c74d55626ba114f01aa96 |
|||
| msg411741 - (view) | Author: Christian Heimes (christian.heimes) * ![]() |
Date: 2022-01-26 11:21 | |
The fix is in all stable branches. Thanks! |
|||
| msg411752 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2022-01-26 14:29 | |
IMO making the assumption that "char" is signed or not in C code is bad. If Python has code like that, it must be signed to explicitly use one of these types: unsigned char or uint8_t, signed char or int8_t. Hopefully, Python can now use C99 <stdint.h> since Python 3.6. On my x86-64 Fedora 35 (GCC 11.2.1), the "char" type is signed. I built Python with -funsigned-char and I ran the test suite: the whole test suite pass! Commands: --- make distclean ./configure --with-pydebug CFLAGS="-O0 -funsigned-char" --with-system-expat --with-system-ffi make ./python -m test -j0 -r --- Using ./configure CFLAGS, -funsigned-char is also used to build C extensions. Example: gcc (...) -O0 -funsigned-char (...) Modules/_elementtree.c (...) For completeness, I also built Python with -fsigned-char. Again, the full test suite passed ;-) --- make distclean ./configure --with-pydebug CFLAGS="-O0 -fsigned-char" --with-system-expat --with-system-ffi make ./python -m test -r -j0 --- |
|||
| msg411753 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2022-01-26 14:30 | |
In short, I did my checks on my side, and the merged change now LGTM. Thanks Christian for fixing it ;-) |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:59:55 | admin | set | github: 90671 |
| 2022-01-26 14:30:01 | vstinner | set | messages: + msg411753 |
| 2022-01-26 14:29:28 | vstinner | set | messages: + msg411752 |
| 2022-01-26 11:21:50 | christian.heimes | set | status: open -> closed resolution: fixed messages: + msg411741 stage: patch review -> resolved |
| 2022-01-26 11:21:08 | christian.heimes | set | messages: + msg411739 |
| 2022-01-26 11:21:08 | christian.heimes | set | messages: + msg411740 |
| 2022-01-26 09:07:01 | christian.heimes | set | pull_requests: + pull_request29094 |
| 2022-01-26 09:06:09 | christian.heimes | set | pull_requests: + pull_request29093 |
| 2022-01-26 09:04:44 | petr.viktorin | set | nosy:
- miss-islington messages: + msg411719 |
| 2022-01-26 09:03:58 | miss-islington | set | nosy:
+ miss-islington messages: + msg411718 |
| 2022-01-25 19:16:28 | vstinner | set | messages: + msg411666 |
| 2022-01-25 19:13:02 | vstinner | set | messages: + msg411665 |
| 2022-01-25 18:14:01 | vstinner | set | messages: + msg411655 |
| 2022-01-25 18:13:07 | vstinner | set | messages: + msg411654 |
| 2022-01-25 11:35:42 | christian.heimes | set | messages: + msg411576 |
| 2022-01-25 11:34:07 | christian.heimes | set | keywords:
+ patch stage: patch review pull_requests: + pull_request29062 |
| 2022-01-25 11:28:33 | hroncok | create | |
