Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AC_C_CHAR_UNSIGNED from configure.ac confuses GCC 12+ by defining __CHAR_UNSIGNED__ #90671

Closed
hroncok mannequin opened this issue Jan 25, 2022 · 13 comments
Closed

AC_C_CHAR_UNSIGNED from configure.ac confuses GCC 12+ by defining __CHAR_UNSIGNED__ #90671

hroncok mannequin opened this issue Jan 25, 2022 · 13 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes build The build process and cross-build

Comments

@hroncok
Copy link
Mannequin

hroncok mannequin commented Jan 25, 2022

BPO 46513
Nosy @vstinner, @tiran, @encukou, @hroncok
PRs
  • bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ #30851
  • [3.10] bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (GH-30851) (GH-30914) #30914
  • [3.9] bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (GH-30851) (GH-30915) #30915
  • 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 2022-01-26.11:21:50.243>
    created_at = <Date 2022-01-25.11:28:33.802>
    labels = ['build', '3.9', '3.10', '3.11']
    title = 'AC_C_CHAR_UNSIGNED from configure.ac confuses GCC 12+ by defining __CHAR_UNSIGNED__'
    updated_at = <Date 2022-01-26.14:30:01.087>
    user = 'https://github.com/hroncok'

    bugs.python.org fields:

    activity = <Date 2022-01-26.14:30:01.087>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-01-26.11:21:50.243>
    closer = 'christian.heimes'
    components = ['Build']
    creation = <Date 2022-01-25.11:28:33.802>
    creator = 'hroncok'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 46513
    keywords = ['patch']
    message_count = 13.0
    messages = ['411575', '411576', '411654', '411655', '411665', '411666', '411718', '411719', '411739', '411740', '411741', '411752', '411753']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'christian.heimes', 'petr.viktorin', 'hroncok']
    pr_nums = ['30851', '30914', '30915']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'compile error'
    url = 'https://bugs.python.org/issue46513'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @hroncok
    Copy link
    Mannequin Author

    hroncok mannequin commented Jan 25, 2022

    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 #30851 confirmed that all the buildbots are happy with such change. Hence, I open this issue to actually do it.

    Thanks

    @hroncok hroncok mannequin added 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes build The build process and cross-build labels Jan 25, 2022
    @tiran
    Copy link
    Member

    tiran commented Jan 25, 2022

    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

    @vstinner
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member

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

    @vstinner
    Copy link
    Member

    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;

    @miss-islington
    Copy link
    Contributor

    New changeset 6e5a193 by Christian Heimes in branch 'main':
    bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (GH-30851)
    6e5a193

    @encukou
    Copy link
    Member

    encukou commented Jan 26, 2022

    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.

    @tiran
    Copy link
    Member

    tiran commented Jan 26, 2022

    New changeset 4371fbd by Christian Heimes in branch '3.10':
    [3.10] bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (GH-30851) (GH-30914)
    4371fbd

    @tiran
    Copy link
    Member

    tiran commented Jan 26, 2022

    New changeset 04772cd by Christian Heimes in branch '3.9':
    [3.9] bpo-46513: Remove AC_C_CHAR_UNSIGNED / __CHAR_UNSIGNED__ (GH-30851) (GH-30915)
    04772cd

    @tiran
    Copy link
    Member

    tiran commented Jan 26, 2022

    The fix is in all stable branches. Thanks!

    @tiran tiran closed this as completed Jan 26, 2022
    @tiran tiran closed this as completed Jan 26, 2022
    @vstinner
    Copy link
    Member

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

    @vstinner
    Copy link
    Member

    In short, I did my checks on my side, and the merged change now LGTM. Thanks Christian for fixing it ;-)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes build The build process and cross-build
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants