classification
Title: use of STATUS_CONTROL_C_EXIT in Modules/main.c breaks compilation with non MSC compilers
Type: compile error Stage: resolved
Components: Windows Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: erikjanss, eryksun, paul.moore, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2019-05-19 13:02 by erikjanss, last changed 2019-05-22 11:05 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 13421 merged erikjanss, 2019-05-19 13:32
PR 13471 merged erikjanss, 2019-05-22 07:22
Messages (14)
msg342845 - (view) Author: Erik Janssens (erikjanss) * Date: 2019-05-19 13:02
in Modules/main.c STATUS_CONTROL_C_EXIT is included conditionally if compiling when _MSC_VER is defined.  Later on STATUS_CONTROL_C_EXIT is used if MS_WINDOWS is defined.

This breaks compilation of main.c with any non MSC compiler while compiling for MS Windows.

This appears to have been introduced in GH-12123 for bpo-36142
msg342848 - (view) Author: Erik Janssens (erikjanss) * Date: 2019-05-19 13:36
According to the Microsoft documentation at

https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/using-ntstatus-values

system-supplied status codes are defined in ntstatus.h. and the NTSTATUS type is defined in ntdef.h

PR 13421 includes both ntstatus.h and ntdef.h to be able to use STATUS_CONTROL_C_EXIT when compiling for windows.
msg342932 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-05-20 16:52
Is including just "winnt.h" sufficient?

It's very hard to tell which Windows headers are "public" vs "internal", but winternl.h is certainly one of the internal ones (according to the big warning comment at the top of the file)
msg342934 - (view) Author: Erik Janssens (erikjanss) * Date: 2019-05-20 17:01
including "winnt.h" gives me compilation problems with other undefined types.

however, simply including "windows.h" instead of both includes compiles just fine.

so maybe that is sufficient ??
msg342935 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-05-20 17:11
Some people say "windows.h" is the only one you're ever supposed to include, so if that works best, let's go with that :)
msg342936 - (view) Author: Erik Janssens (erikjanss) * Date: 2019-05-20 17:13
ok, thank you for the advice, I'll keep it in mind and adapt the PR !
msg342943 - (view) Author: Erik Janssens (erikjanss) * Date: 2019-05-20 18:49
PR has been changed to include "windows.h" ...
msg342977 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-05-21 05:08
"crtdbg.h" doesn't provide STATUS_CONTROL_C_EXIT, but it should be fine to remove it anyway. I think it was left behind by accident in 2007. It was added to support a PYTHONNOERRORWINDOW environment variable, but then this idea was dropped in favor of extending the msvcrt module:

* https://grokbase.com/t/python/python-3000/078wkax0sd/buildbots
* https://github.com/python/cpython/commit/945362cf971fb2e10f8f2a8e71e8ff10516ebe4c#diff-75445bdc3b6b3dd20b005698fa165444
* https://github.com/python/cpython/commit/3dc33d18452de871cff98914dda81ff00b4d00f6#diff-75445bdc3b6b3dd20b005698fa165444

I presume STATUS_CONTROL_C_EXIT gets included from "winnt.h" -> "Windows.h" -> "Include/internal/pycore_condvar.h" -> "Include/internal/pycore_gil.h" -> "Include/internal/pycore_pystate.h".

If [STATUS_]CONTROL_C_EXIT isn't defined, I suggest defining WIN32_LEAN_AND_MEAN before including "Windows.h". This reduces the number of included headers from about 350 down to about 200. Also, to stay strictly within the Windows API, we might want to use CONTROL_C_EXIT (from [min]winbase.h) instead of STATUS_CONTROL_C_EXIT (from "winnt.h").
msg342998 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-21 10:11
New changeset 925af1d99b69bf3e229411022ad840c5a0cfdcf8 by Victor Stinner (Erik Janssens) in branch 'master':
bpo-36965: Fix includes in main.c on Windows with non-MSC compilers (GH-13421)
https://github.com/python/cpython/commit/925af1d99b69bf3e229411022ad840c5a0cfdcf8
msg343009 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-21 10:57
eryksun commented there, but I prefer to discuss here:
https://github.com/python/cpython/commit/925af1d99b69bf3e229411022ad840c5a0cfdcf8#commitcomment-33617265

""Windows.h" was already being included, as I mentioned on the issue tracker, because we certainly were not getting STATUS_CONTROL_C_EXIT from "crtdbg.h", a header that was left in this file accidentally about 12 years ago. If it's included explicitly here, also define WIN32_LEAN_AND_MEAN to cut the number of included headers by about a half."

I prefer to explicitly include windows.h, it doesn't hurt :-)

WIN32_LEAN_AND_MEAN is defined by Include/internal/pycore_condvar.h which is indirectly included by pycore_pystate.h:

#include "pycore_gil.h"   /* _gil_runtime_state  */

pycore_gil.h:

#include "pycore_condvar.h"


By the way, WIN32_LEAN_AND_MEAN caused me issues while working on bpo-36728:
https://twitter.com/VictorStinner/status/1127884878027079680

I managed to workaround the issue: commit d5d9e81ce9a7efc5bc14a5c21398d1ef6f626884

Extract of (fixed) posixmodule.c:
---
...
#include "Python.h"
#ifdef MS_WINDOWS
   /* include <windows.h> early to avoid conflict with pycore_condvar.h:

        #define WIN32_LEAN_AND_MEAN
        #include <windows.h>

      FSCTL_GET_REPARSE_POINT is not exported with WIN32_LEAN_AND_MEAN. */
#  include <windows.h>
#endif

#include "pycore_ceval.h"     /* _PyEval_ReInitThreads() */
#include "pycore_pystate.h"   /* _PyRuntime */
...
---
msg343013 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-21 11:17
> WIN32_LEAN_AND_MEAN is defined by Include/internal/pycore_condvar.h (...)

It would be nice to get ride of #include <windows.h> in Python headers. The <windows.h> was introduced by this commit:

commit 2ebc5ce42a8a9e047e790aefbf9a94811569b2b6
Author: Eric Snow <ericsnowcurrently@gmail.com>
Date:   Thu Sep 7 23:51:28 2017 -0600

    bpo-30860: Consolidate stateful runtime globals. (#3397)
    
    * group the (stateful) runtime globals into various topical structs
    * consolidate the topical structs under a single top-level _PyRuntimeState struct
    * add a check-c-globals.py script that helps identify runtime globals
    
    Other globals are excluded (see globals.txt and check-c-globals.py).

The current problem is that we need the HANDLE type which comes from <windows.h> to get the full structure:

typedef struct _PyCOND_T
{
    HANDLE sem;
    int waiting; /* to allow PyCOND_SIGNAL to be a no-op */
} PyCOND_T;

I tried to avoid "HANDLE" using:

typedef struct _PyCOND_T
{
    void* sem;
    int waiting; /* to allow PyCOND_SIGNAL to be a no-op */
} PyCOND_T;

... but then pycore_condvar.h compilation fails on "typedef CRITICAL_SECTION PyMUTEX_T;": CRITICAL_SECTION type is not defined :-(

By the way, Python still uses _PY_EMULATED_WIN_CV by default, whereas we want to drop Vista support: bpo-32592.
msg343018 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-05-21 11:47
>  FSCTL_GET_REPARSE_POINT is not exported with WIN32_LEAN_AND_MEAN

You can explicitly include "winioctl.h" after "windows.h". Getting it from "windows.h" is indirect via "winscard.h" (smart card services), which will be skipped if either WIN32_LEAN_AND_MEAN or NOCRYPT is defined.
msg343172 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-22 11:04
New changeset 791e5fcbab9e444b62d13d08707cbbbeb9406297 by Victor Stinner (Erik Janssens) in branch '3.7':
 bpo-36965: Fix includes in main.c on Windows with non-MSC compilers (GH-13421) (GH-13471)
https://github.com/python/cpython/commit/791e5fcbab9e444b62d13d08707cbbbeb9406297
msg343173 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-22 11:05
The initial issue is fixed in 3.7 and master branches. If you want to work on related issue like WIN32_LEAN_AND_MEAN, please open a separated issue.
History
Date User Action Args
2019-05-22 11:05:10vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg343173

stage: patch review -> resolved
2019-05-22 11:04:09vstinnersetmessages: + msg343172
2019-05-22 07:22:06erikjansssetpull_requests: + pull_request13401
2019-05-21 11:47:43eryksunsetmessages: + msg343018
2019-05-21 11:17:41vstinnersetmessages: + msg343013
2019-05-21 10:57:02vstinnersetmessages: + msg343009
2019-05-21 10:11:15vstinnersetmessages: + msg342998
2019-05-21 05:08:18eryksunsetnosy: + eryksun
messages: + msg342977
2019-05-20 18:49:32erikjansssetmessages: + msg342943
2019-05-20 17:13:24erikjansssetmessages: + msg342936
2019-05-20 17:11:39steve.dowersetmessages: + msg342935
2019-05-20 17:01:33erikjansssetmessages: + msg342934
2019-05-20 16:52:15steve.dowersetmessages: + msg342932
2019-05-19 13:36:01erikjansssetmessages: + msg342848
2019-05-19 13:32:12erikjansssetkeywords: + patch
stage: patch review
pull_requests: + pull_request13330
2019-05-19 13:06:27SilentGhostsetnosy: + vstinner
2019-05-19 13:02:26erikjansscreate