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

Jump to bottom

Prohibit implicit C function declarations #71846

Closed
yan12125 mannequin opened this issue Jul 31, 2016 · 19 comments
Closed

Prohibit implicit C function declarations #71846

yan12125 mannequin opened this issue Jul 31, 2016 · 19 comments
Labels
3.7 (EOL) end of life build The build process and cross-build type-feature A feature request or enhancement

Comments

@yan12125
Copy link
Mannequin

yan12125 mannequin commented Jul 31, 2016

BPO 27659
Nosy @ronaldoussoren, @vstinner, @benjaminp, @ned-deily, @xdegaye, @vadmium, @moreati, @yan12125
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
    		•
    Files
  • check-crypt.patch
  • check-crypt.patch: version 2, autoreconf necessary
  • prohibit-implicit-function-declarations.patch
  • prohibit-implicit-function-declarations.patch: Patch version 2
  • prohibit-implicit-function-declarations.patch: Patch version 3, autoreconf necessary
    		•

    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 2017-02-06.13:45:59.719>
created_at = <Date 2016-07-31.07:04:38.341>
labels = ['type-feature', '3.7', 'build']
title = 'Prohibit implicit C function declarations'
updated_at = <Date 2017-03-31.16:36:20.997>
user = 'https://github.com/yan12125'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:20.997>
actor = 'dstufft'
assignee = 'none'
closed = True
closed_date = <Date 2017-02-06.13:45:59.719>
closer = 'yan12125'
components = ['Build', 'Cross-Build']
creation = <Date 2016-07-31.07:04:38.341>
creator = 'yan12125'
dependencies = []
files = ['43954', '43955', '46033', '46036', '46302']
hgrepos = []
issue_num = 27659
keywords = ['patch']
message_count = 19.0
messages = ['271727', '271728', '271729', '271730', '271732', '278770', '278824', '278836', '283996', '284002', '284004', '285567', '285583', '286759', '287126', '287127', '287128', '287130', '287132']
nosy_count = 9.0
nosy_names = ['ronaldoussoren', 'vstinner', 'benjamin.peterson', 'ned.deily', 'xdegaye', 'python-dev', 'martin.panter', 'Alex.Willmer', 'yan12125']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27659'
versions = ['Python 3.7']
    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Jul 31, 2016

    On Android the crypt() function is missing, causing ugly linking errors when compiling the _crypt module. This patch handles it elegantly.

    A question: should I include changes to configure and pyconfig.h.in in the patch?

    @yan12125 yan12125 mannequin added build The build process and cross-build labels Jul 31, 2016
    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Jul 31, 2016

    Version 2: correct the name added to missing

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Jul 31, 2016

    Some references for crypt():

    POSIX standard: http://pubs.opengroup.org/onlinepubs/9699919799/functions/crypt.html
    Linux man page: http://man7.org/linux/man-pages/man3/crypt.3.html
    FreeBSD man page: https://www.freebsd.org/cgi/man.cgi?crypt(3)
    Mac OS X man page: https://developer.apple.com/library/ios/documentation/System/Conceptual/ManPages_iPhoneOS/man3/crypt.3.html

    All requires <unistd.h>.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Jul 31, 2016

    A question: should I include changes to configure and pyconfig.h.in in the patch?

    You just need to mention that one should run autoreconf.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Jul 31, 2016

    Thanks, added to the patch description

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Oct 16, 2016

    Android does not have crypt, but the crypt module is cross-built nevertheless after this warning has been issued:
    warning: implicit declaration of function 'crypt' is invalid in C99 [-Wimplicit-function-declaration]
    And at runtime, importing the crypt module fails with:
    ImportError: dlopen failed: cannot locate symbol "crypt" referenced by "_crypt.cpython-37m-i686-linux-android.so"

    gcc and clang do not enforce the C99 rules and emit just a warning for implicit function declarations instead of the error that would be conforming to C99. This can be changed with the flag '-Werror=implicit-function-declaration' and the compilation of the crypt extension module rightly fails in that case.

    I think this issue should be fixed by adding this flag to the Makefile.
    Maybe in another issue.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Oct 17, 2016

    Agreed. Adding -Werror=implicit-function-declaration is much simpler. Feel free to close it as rejected.

    @vadmium
    Copy link
    Member

    vadmium commented Oct 18, 2016

    If there is an obscure platform where we don’t include the right header file for a function, changing the warning into an error would cause the build to fail. If we do make it an error, it should only be so for 3.7.

    @xdegaye xdegaye mannequin added the 3.7 (EOL) end of life label Oct 20, 2016
    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 25, 2016

    (Re-use the existing issue)

    Here's a patch that tries to add -Werror=implicit-function-declaration to $BASECFLAGS.

    This is useful for cross-compiling. When a function is missing, the error jumps out during the build time rather than runtime.

    Tested configurations:

    • Arch Linux x86_64 native build
    • macOS Sierra native build
    • Android ARM, API 21 cross build

    I'd like to hear some ideas from macOS experts as in my memory macOS's build system is fragile than that on Linux.

    @yan12125 yan12125 mannequin added the build The build process and cross-build label Dec 25, 2016
    @yan12125 yan12125 mannequin changed the title Check for the existence of crypt() Prohibit implicit C function declarations Dec 25, 2016
    @yan12125 yan12125 mannequin added type-feature A feature request or enhancement and removed build The build process and cross-build labels Dec 25, 2016
    @vstinner
    Copy link
    Member

    Would it be possible to not add this option for third party extensions?

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 25, 2016

    Would it be possible to not add this option for third party extensions?

    Good suggestion. Just use $CFLAGS_NODIST instead of $BASECFLAGS.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Jan 16, 2017

    Thanks for the comment and sorry for the mistake. Here's another updated patch.

    In PEP-7:

    Use 4-space indents and no tabs at all.

    Does that apply to configuration files, too?

    @vadmium
    Copy link
    Member

    vadmium commented Jan 16, 2017

    I would say it is more important to fit in with the surrounding style than mindlessly follow PEP-7. IMO the indentation in the configure script is a mess, but if we fix it up, it should probably be done separately to adding this extra flag.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 2, 2017

    Hello, any updates here? I hope this merged soon so that potential issues on obscure platforms can be fixed as soon as possible.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 6, 2017

    New changeset ca2f024ce7cb by Victor Stinner in branch 'default':
    Prohibit implicit C function declarations
    https://hg.python.org/cpython/rev/ca2f024ce7cb

    @vstinner
    Copy link
    Member

    vstinner commented Feb 6, 2017

    Martin Panter: "If there is an obscure platform where we don’t include the right header file for a function, changing the warning into an error would cause the build to fail."

    In my experience, calling a function which was not declared is very likely to cause a bug, or a crash in the worst case. For example, on 64-bit, if the return type is a pointer, the C compiler uses the int type by default, whereas a pointer is 32-bit, not 64-bit, and so it will immediately crash.

    Martin: "If we do make it an error, it should only be so for 3.7."

    Ok. I pushed the patch to Python 3.6.

    @Chi Hsuan Yen: Thanks for the patch! Is this change enough to fix the crypt build issue? If yes, can we close the issue?

    It is likely that the cause causes compilation errors on some platforms where we call non-existent functions or call functions with a missing header. IMHO it's a good thing to get a build error rather than a crash at runtime.

    A concrete issue is that the compilation of the curses module will probably fails now on Solaris: issue bpo-13552, whereas before the build only emitted warnings. The curses module is broken for years on Solaris, and it seems like nobody is able to fix it, so it's not a big deal.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 6, 2017

    If yes, can we close the issue?

    Yes and thanks! As a side note, on Android it prevents broken grp.cpython-37m.so, too.

    @yan12125 yan12125 mannequin closed this as completed Feb 6, 2017
    @vstinner
    Copy link
    Member

    vstinner commented Feb 6, 2017

    Oh by the way, if someone sees a build error because of a missing function declaration, please report a new issue.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 6, 2017

    New changeset 9a26d20d2baa27407501b13435d733dcc26f3d53 by Victor Stinner in branch 'master':
    Prohibit implicit C function declarations
    9a26d20

    @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
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life build The build process and cross-build type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @vstinner @vadmium