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

Turn on extra warnings on GCC #67733

Closed
serhiy-storchaka opened this issue Feb 27, 2015 · 20 comments
Closed

Turn on extra warnings on GCC #67733

serhiy-storchaka opened this issue Feb 27, 2015 · 20 comments
Labels
build The build process and cross-build type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 23545
Nosy @vstinner, @benjaminp, @skrah, @vadmium, @serhiy-storchaka, @ztane
Files
  • configure_extra_warnings.patch
  • configure_extra_warnings2.patch
  • 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 2016-09-11.19:09:12.723>
    created_at = <Date 2015-02-27.22:29:43.909>
    labels = ['type-feature', 'build']
    title = 'Turn on extra warnings on GCC'
    updated_at = <Date 2016-09-13.05:32:13.488>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2016-09-13.05:32:13.488>
    actor = 'martin.panter'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-09-11.19:09:12.723>
    closer = 'serhiy.storchaka'
    components = ['Build']
    creation = <Date 2015-02-27.22:29:43.909>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['38547', '44512']
    hgrepos = []
    issue_num = 23545
    keywords = ['patch']
    message_count = 20.0
    messages = ['236851', '236887', '238469', '269017', '271153', '272444', '272480', '272499', '272500', '272501', '272502', '275470', '275586', '275830', '275832', '276072', '276075', '276076', '276077', '276178']
    nosy_count = 8.0
    nosy_names = ['vstinner', 'benjamin.peterson', 'SilentGhost', 'skrah', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'ztane']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue23545'
    versions = ['Python 3.6']

    @serhiy-storchaka
    Copy link
    Member Author

    What if make GCC emit extra warnings? Adding following options:

    -Wall -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers

    will make GCC emit all warnings except few types that would produce too many warnings. With these options the compilation is almost clean on 32-bit Linux.

    @serhiy-storchaka serhiy-storchaka added build The build process and cross-build type-feature A feature request or enhancement labels Feb 27, 2015
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Feb 28, 2015

    +1. I think the flags should go into CFLAGS_NODIST.

    @serhiy-storchaka
    Copy link
    Member Author

    I'm not well experienced with autoconf. Here is my attempt.

    @serhiy-storchaka
    Copy link
    Member Author

    Could anyone please make a review?

    @vadmium
    Copy link
    Member

    vadmium commented Jul 24, 2016

    -Wall is already added (unconditionally) to $OPT above. What’s the point of also adding it to $CFLAGS_NODIST as well? Does it have to be added to both?

    @vadmium
    Copy link
    Member

    vadmium commented Aug 11, 2016

    The proposed options add exactly one warning for me (ignoring warnings from libffi). How would you work around this:

    ./Include/pymem.h:136:18: warning: comparison is always false due to limited range of data type [-Wtype-limits]
    ( ((size_t)(n) > PY_SSIZE_T_MAX / sizeof(type)) ? NULL : \
    ^
    /media/disk/home/proj/python/cpython/Modules/_ssl.c:4435:22: note: in expansion of macro ‘PyMem_New’
    _ssl_locks = PyMem_New(PyThread_type_lock, _ssl_locks_count);
    ^~~~~~~~~

    @serhiy-storchaka
    Copy link
    Member Author

    Just add -Wno-type-limits.

    @ztane
    Copy link
    Mannequin

    ztane mannequin commented Aug 12, 2016

    I don't think adding -Wno-type-limits is a good idea.

    The good question is how that can be happening, e.g. how PY_SSIZE_T_MAX divided by sizeof anything can be *more* than max(size_t)? E.g now that I stare at the code, *that* warning should be impossible if everything is correct. It means either that the RHS is negative or size_t is defined to be 32-bit in this compilation unit whereas PY_SSIZE_T is 64-bit. Neither sound like a good idea.

    @vadmium
    Copy link
    Member

    vadmium commented Aug 12, 2016

    I didn’t look too closely, but I did see that _ssl_locks_count is unsigned int. I was compiling for x86-64 Linux, where int is 32 bits and size_t is 64. So maybe GCC was optimizing the (size_t) cast away, and then rightfully warning that the largest unsigned int will never exceed the largest possible array size.

    @serhiy-storchaka
    Copy link
    Member Author

    Seems GCC is such smart, that can deduce that the maximal value of _ssl_locks_count is never larger than PY_SSIZE_T_MAX / sizeof(PyThread_type_lock). The other workaround is making _ssl_locks_count of type size_t instead of unsigned int, but I wouldn't do this with good reasons.

    @ztane
    Copy link
    Mannequin

    ztane mannequin commented Aug 12, 2016

    Ah, indeed, I somehow missed that. Though, there is no good reason for it being unsigned either; as the actual type in SSL API's is of type int. Another argument of type int is cast to unsigned just for the comparison on line 4419, and unsigned int counters i and j are used in function _setup_ssl_threads.

    The variable could be safely changed to size_t (along with those index variables) without it affecting anything at all, as it is a static variable within that module and only used to hold a size of an array, and never passed back to another function.

    @serhiy-storchaka
    Copy link
    Member Author

    Rebased patch. Removed save_CFLAGS="$CFLAGS" and CFLAGS="$save_CFLAGS" lines as Marin suggested.

    Benjamin, seems you are experienced with autoconf. Could you please make a review?

    @vadmium
    Copy link
    Member

    vadmium commented Sep 10, 2016

    Can one of the -Wall flags be dropped? What is the difference between $OPT and $CFLAGS_NODIST?

    gcc -pthread -c -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -std=c99 -Wall -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -I. -IInclude -I./Include -DPy_BUILD_CORE -o Programs/python.o ./Programs/python.c

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 11, 2016

    New changeset 99adb5df8cb6 by Serhiy Storchaka in branch 'default':
    Issue bpo-23545: Turn on extra warnings on GCC.
    https://hg.python.org/cpython/rev/99adb5df8cb6

    @serhiy-storchaka
    Copy link
    Member Author

    AFAIK $CFLAGS_NODIST is used only for compiling the core and standard modules, $OPT is included in $CFLAGS and also used for compiling third-party extensions. Since -Wall is already included in $OPT, there is no need of it in $CFLAGS_NODIST. Removed this part. Thank you for your review Martin.

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Sep 12, 2016

    The commit didn't resolve the warning though.

    @serhiy-storchaka
    Copy link
    Member Author

    All warnings were resolved at the moment of providing previous version, but the code was changed too much last days. New warnings was added and disappeared in these days. Now we can just sit and resolve the leftover.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 12, 2016

    New changeset da485c6c744e by Stefan Krah in branch 'default':
    Issue bpo-23545: Adding -Wextra in setup.py is no longer necessary, since it
    https://hg.python.org/cpython/rev/da485c6c744e

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 12, 2016

    Agreed. SilentGhost, it's probably best to open new issues for the warnings.

    @vadmium
    Copy link
    Member

    vadmium commented Sep 13, 2016

    I think Silent Ghost is talking about my -Wtype-limits warning, which is still present. That is the only warning I am getting. I suspect you won’t see it with a 32-bit build.

    @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
    build The build process and cross-build type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants