_Py_stat and _Py_wstat using incorrect type for status argument

[pacampbell]

BPO	46303
Nosy	@gvanrossum, @pfmoore, @vstinner, @tiran, @tjguk, @zware, @zooba, @pacampbell
PRs	bpo-46303: Fix _Py_stat and _Py_wstat status argument type #30478 bpo-46303: Move fileutils.h private functions to internal C API #30484 [3.10] bpo-46303: Fix build error on "struct stat" on Windows #30528 bpo-46303: Don't define _Py_stat() and _Py_wstat() on Windows #30539 bpo-46303: Fix _Py_stat() compiler warning in _tkinter.c #30550

^{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-12.01:45:42.399>
created_at = <Date 2022-01-08.03:48:43.841>
labels = ['build', 'OS-windows', '3.11']
title = '_Py_stat and _Py_wstat using incorrect type for status argument'
updated_at = <Date 2022-01-12.20:44:01.880>
user = 'https://github.com/pacampbell'

bugs.python.org fields:

activity = <Date 2022-01-12.20:44:01.880>
actor = 'steve.dower'
assignee = 'none'
closed = True
closed_date = <Date 2022-01-12.01:45:42.399>
closer = 'vstinner'
components = ['Build', 'Windows']
creation = <Date 2022-01-08.03:48:43.841>
creator = 'pacampbell'
dependencies = []
files = []
hgrepos = []
issue_num = 46303
keywords = ['patch']
message_count = 21.0
messages = ['410073', '410095', '410288', '410292', '410306', '410309', '410311', '410351', '410355', '410357', '410359', '410360', '410361', '410365', '410366', '410367', '410369', '410374', '410377', '410379', '410429']
nosy_count = 8.0
nosy_names = ['gvanrossum', 'paul.moore', 'vstinner', 'christian.heimes', 'tim.golden', 'zach.ware', 'steve.dower', 'pacampbell']
pr_nums = ['30478', '30484', '30528', '30539', '30550']
priority = None
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue46303'
versions = ['Python 3.11']

[pacampbell]

While attempting to embed the full cpython source in my application, I found that during compilation on Windows, there was a compilation issue due to struct stat not being defined. Taking a look at the file cpython/fileutils.h, it seems that the type of the status argument should be _Py_stat_struct instead of just stat to satisfy compilation for all supported platforms.

[vstinner]

It's not the first time that private functions included by the public Python.h are causing build errors event if these functions are not used. The previous issue were functions for atomic operations. I solved this build error by moving the whole private C API into the internal C API: Include/internal/pycore_atomic.h. Since that time, we no longer got build error related to this header file.

I propose a similar fix: move all private fileutils.h functions from Include/cpython/fileutils.h to the internal Include/internal/pycore_fileutils.h.

I wrote PR 30484 to implement this change.

To keep the implementation simple, I kept _Py_fopen_obj() in Include/cpython/fileutils.h, for _testcapi which must not use the internal C API.

If this PR is merged, for Python 3.9 and 3.10, I will write a simpler change: modify Include/cpython/fileutils.h to only define functions using "struct stat" in the internal C API (if Py_BUILD_CORE is defined).

[vstinner]

New changeset ea1a545 by Victor Stinner in branch 'main':
bpo-46303: Move fileutils.h private functions to internal C API (GH-30484)
ea1a545

[vstinner]

While attempting to embed the full cpython source in my application, I found that during compilation on Windows, there was a compilation issue due to struct stat not being defined.

Do you get the error when building Python? Or on #include <Python.h> when using the Python C API?

How do you build Python? What is your C compiler?

Can you test if my proposed PR 30528 fix your issue?

[pacampbell]

I was trying to build python core (-DMS_WINDOWS -DPy_BUILD_CORE). I was using clang, which I think is unsupported looking at Windows doc. After looking at the issue though, it seemed that it was just some slight mistake which is why I filed the bug.

clang version 13.0.0
Target: x86_64-pc-windows-msvc
Thread model: posix

I can test to see if it fixes the immediate build problem, but I still find your fix not quite addressing the issue which I initially tried to create a patch for. Someone has already developed a shim here for Windows and it just was not used properly. _Py_stat_struct is a define which either evaluates to stat on non-Windows systems or a compatibility structure on Windows. Simply replacing the use of struct stat with struct _Py_stat_struct should solve the issue.

[vstinner]

Python.h indirectly (via fileutils.h) defines _Py_wstat() and _Py_stat() functions which use the "struct stat*" type for 12 years:

commit 4e31443
Author: Victor Stinner <victor.stinner@haypocalc.com>
Date: Thu Oct 7 21:45:39 2010 +0000

Create fileutils.c/.h

 * _Py_fopen() and _Py_stat() come from [Python/import.c](https://github.com/python/cpython/blob/main/Python/import.c)
 * (_Py)_wrealpath() comes from [Python/sysmodule.c](https://github.com/python/cpython/blob/main/Python/sysmodule.c)
 * _Py_char2wchar(), _Py_wchar2char() and _Py_wfopen() come from [Modules/main.c](https://github.com/python/cpython/blob/main/Modules/main.c)
 * (_Py)_wstat(), (_Py)_wgetcwd(), _Py_wreadlink() come from [Modules/getpath.c](https://github.com/python/cpython/blob/main/Modules/getpath.c)

[vstinner]

It seems like _Py_stat() and _Py_wstat() are only needed on non-Windows platforms: only the _get_tcl_lib_path() function of Modules/_tkinter.c uses _Py_stat(). I wrote PR 30539 to not define _Py_stat() and _Py_wstat() on Windows.

[vstinner]

I marked bpo-46346 as a duplicate of this issue. Copy of the first message:

"""
I am getting these warnings:

C:\Users\gvanrossum\cpython\PC\_testconsole.c(70,38): warning C4013: '_Py_get_osfhandle' undefined; assuming extern returnin
g int [C:\Users\gvanrossum\cpython\PCbuild\_testconsole.vcxproj]
C:\Users\gvanrossum\cpython\PC\_testconsole.c(70,1): warning C4047: 'initializing': 'HANDLE' differs in levels of indirectio
n from 'int' [C:\Users\gvanrossum\cpython\PCbuild\_testconsole.vcxproj]
C:\Users\gvanrossum\cpython\Modules\_tkinter.c(144,37): warning C4013: '_Py_stat' undefined; assuming extern returning int [
C:\Users\gvanrossum\cpython\PCbuild\_tkinter.vcxproj]

I noticed that GitHub also was flagging these in unrelated PRs.
"""

I proposed #74735 to fix these warnings.

[tiran]

I set the release blocker flag for the ticket.

[vstinner]

New changeset 08bc1ba by Victor Stinner in branch 'main':
bpo-46303: Fix fileutils.h compiler warnings (GH-30550)
08bc1ba

[vstinner]

Christian Heimes: "I set the release blocker flag for the ticket."

It's just a compiler warning, why marking it as a release blocker?

Anyway, it's now fixed.

[vstinner]

Paul: Can you please try to build the main branch of Python with clang and tell me if you still have the compiler warnings? If yes, can you please copy/paste the compiler warnings?

Do you build Python on Windows or from another OS (cross-compilation)?

[vstinner]

If possible, I would prefer to not change 3.9 and 3.10 to avoid any risk of introducing a *new* build error, while trying to support a new platform. I don't think that we currently supporting build Python with clang on Windows yet.

[pacampbell]

Hi Victor, I was trying to compile with clang on Windows 10. I will try to pull your 3.11 changes and test. Sorry to cause so much churn. It looked to me like a simple issue that was missed, probably because whatever was trying to compile was not normally compiled on Windows. I was not trying to make a lot of work to support a new platform :)

[tiran]

It's unlikely that you can reproduce the issue with clang. We use MSVC and a manually maintained pyconfig.h on Windows.

[vstinner]

Hi Victor, I was trying to compile with clang on Windows 10. I will try to pull your 3.11 changes and test. Sorry to cause so much churn. It looked to me like a simple issue that was missed, probably because whatever was trying to compile was not normally compiled on Windows. I was not trying to make a lot of work to support a new platform :)

I tried to write a change which doesn't increase the maintenance on other platforms.

I dislike declaring private functions in the *public* Python.h header file, especially if they cause build error. Over the last years, I moved many private functions to the internal C API.

In Python, we are trying to provide a same C API on all platforms. If "struct stat" is no longer considered as portable, IMO we should attempt to avoid it, at least in the public C API. For these reasons, I dislike PR 30478. The problem is that if the work is written on Linux using "strut stat", the build can fail on Windows if "struct _Py_stat_struct" is now required on Windows. I expected "struct stat" to be portable, but it seems like I was wrong.

[pacampbell]

In Python, we are trying to provide a same C API on all platforms. If "struct stat" is no longer considered as portable, IMO we should attempt to avoid it, at least in the public C API.

Microsoft provides stat and struct stat, but they prepend the names with an underscore. So functions like stat are named _stat and struct stat is named struct _stat, etc. Not sure if pros/cons of using such functions are though. Seems it would go back to depending on some type nonstandard python macro to translate between the two during build.

[vstinner]

Seems it would go back to depending on some type nonstandard python macro to translate between the two during build.

In the internal C API, there are less concerns about writing portable code. We expect users of this API to pay more attention to what they do and there is no backward compatibility warranty.

In the internal C API, it's acceptable to change the parameter type depending on the platform.

[pacampbell]

Victor: The changes in the main branch gets me past this issue without having to make additional changes.

[vstinner]

Paul Campbell: "The changes in the main branch gets me past this issue without having to make additional changes."

Ok, nice. I close the issue.

I consider that building Python with clang is a new feature. I prefer to not backport these changes to 3.9 and 3.10 to avoid any risk of regression.

I wrote #74713: simple fix for Python 3.10, but I don't know if it works. I don't know how to test my change. It seems like clang is not commonly used on Windows, so I close the issue.

If someone wants me to fix build issues with clang on Windows, please comment this issue or open a new issue with an error message and a detailed use case.

[zooba]

Microsoft provides stat and struct stat, but they prepend the names with an underscore.
They are also influenced by various compiler options to choose between
32-bit and 64-bit fields. This makes it impossible to use the standard
names as part of an ABI, because we can't/don't enforce that the
preprocessor definitions match.

We should isolate all structures from libc/equivalent in our public API
so that we can ensure compatibility. (FILE* was historically also an
issue, but that was bad enough that Windows fixed it on their side. The
rest of the C runtime library still bleeds everywhere, so we definitely
don't want it or its semantics in our public API if avoidable.)