classification
Title: Prohibit implicit C function declarations
Type: enhancement Stage: resolved
Components: Build, Cross-Build Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Alex.Willmer, Chi Hsuan Yen, benjamin.peterson, haypo, martin.panter, ned.deily, python-dev, ronaldoussoren, xdegaye
Priority: normal Keywords: patch

Created on 2016-07-31 07:04 by Chi Hsuan Yen, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
check-crypt.patch Chi Hsuan Yen, 2016-07-31 07:04 review
check-crypt.patch Chi Hsuan Yen, 2016-07-31 07:09 version 2, autoreconf necessary review
prohibit-implicit-function-declarations.patch Chi Hsuan Yen, 2016-12-25 14:28 review
prohibit-implicit-function-declarations.patch Chi Hsuan Yen, 2016-12-25 17:05 Patch version 2 review
prohibit-implicit-function-declarations.patch Chi Hsuan Yen, 2017-01-16 14:37 Patch version 3, autoreconf necessary review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (19)
msg271727 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-07-31 07:04
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?
msg271728 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-07-31 07:09
Version 2: correct the name added to `missing`
msg271729 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-07-31 07:12
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>.
msg271730 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-07-31 08:18
> 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.
msg271732 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-07-31 08:28
Thanks, added to the patch description
msg278770 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-10-16 15:44
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.
msg278824 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-10-17 17:23
Agreed. Adding -Werror=implicit-function-declaration is much simpler. Feel free to close it as rejected.
msg278836 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-10-18 03:07
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.
msg283996 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-12-25 14:28
(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.
msg284002 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-12-25 16:43
Would it be possible to not add this option for third party extensions?
msg284004 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2016-12-25 17:05
> Would it be possible to not add this option for third party extensions?

Good suggestion. Just use $CFLAGS_NODIST instead of $BASECFLAGS.
msg285567 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2017-01-16 14:37
Thanks for the comment and sorry for the mistake. Here's another updated patch.

In PEP7:

> Use 4-space indents and no tabs at all.

Does that apply to configuration files, too?
msg285583 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-01-16 19:42
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.
msg286759 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2017-02-02 10:17
Hello, any updates here? I hope this merged soon so that potential issues on obscure platforms can be fixed as soon as possible.
msg287126 - (view) Author: Roundup Robot (python-dev) Date: 2017-02-06 13:25
New changeset ca2f024ce7cb by Victor Stinner in branch 'default':
Prohibit implicit C function declarations
https://hg.python.org/cpython/rev/ca2f024ce7cb
msg287127 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-06 13:30
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 #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.
msg287128 - (view) Author: Chi Hsuan Yen (Chi Hsuan Yen) * Date: 2017-02-06 13:45
> If yes, can we close the issue?

Yes and thanks! As a side note, on Android it prevents broken grp.cpython-37m.so, too.
msg287130 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-06 13:50
Oh by the way, if someone sees a build error because of a missing function declaration, please report a new issue.
msg287132 - (view) Author: Roundup Robot (python-dev) Date: 2017-02-06 14:00
New changeset 9a26d20d2baa27407501b13435d733dcc26f3d53 by Victor Stinner in branch 'master':
Prohibit implicit C function declarations
https://github.com/python/cpython/commit/9a26d20d2baa27407501b13435d733dcc26f3d53
History
Date User Action Args
2017-03-31 16:36:21dstufftsetpull_requests: + pull_request946
2017-02-06 14:00:23python-devsetmessages: + msg287132
2017-02-06 13:50:39hayposetmessages: + msg287130
2017-02-06 13:45:59Chi Hsuan Yensetstatus: open -> closed
resolution: fixed
messages: + msg287128

stage: resolved
2017-02-06 13:30:29hayposetmessages: + msg287127
2017-02-06 13:25:45python-devsetnosy: + python-dev
messages: + msg287126
2017-02-02 10:17:08Chi Hsuan Yensetmessages: + msg286759
2017-01-16 19:42:34martin.pantersetmessages: + msg285583
2017-01-16 14:37:55Chi Hsuan Yensetfiles: + prohibit-implicit-function-declarations.patch

messages: + msg285567
2016-12-25 17:05:06Chi Hsuan Yensetfiles: + prohibit-implicit-function-declarations.patch

messages: + msg284004
2016-12-25 16:43:38hayposetmessages: + msg284002
2016-12-25 14:28:02Chi Hsuan Yensetfiles: + prohibit-implicit-function-declarations.patch

type: compile error -> enhancement
components: + Build
title: Check for the existence of crypt() -> Prohibit implicit C function declarations
nosy: + ronaldoussoren, ned.deily

messages: + msg283996
2016-10-20 10:20:20xdegayesetversions: + Python 3.7, - Python 3.6
2016-10-18 03:07:53martin.pantersetmessages: + msg278836
2016-10-17 17:23:05Chi Hsuan Yensetmessages: + msg278824
2016-10-16 15:44:26xdegayesetnosy: + haypo, benjamin.peterson, martin.panter
messages: + msg278770
2016-07-31 08:28:00Chi Hsuan Yensetmessages: + msg271732
2016-07-31 08:22:55xdegayelinkissue26865 dependencies
2016-07-31 08:18:29xdegayesetmessages: + msg271730
2016-07-31 07:12:39Chi Hsuan Yensetmessages: + msg271729
2016-07-31 07:09:52Chi Hsuan Yensetfiles: + check-crypt.patch

messages: + msg271728
2016-07-31 07:04:38Chi Hsuan Yencreate