This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Title: CPython on Windows builds with /W3, not /W4
Type: enhancement Stage: patch review
Components: Build, Windows Versions: Python 3.6
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Alexander Riccio, Jim.Jewett, loewis, paul.moore, serhiy.storchaka, skrah, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2015-12-16 00:12 by Alexander Riccio, last changed 2022-04-11 14:58 by admin.

File name Uploaded Description Edit
W4_v2.patch Alexander Riccio, 2015-12-16 01:04 review
W4_v2_build_output Alexander Riccio, 2015-12-16 01:05 `build.bat -d -v --no-ssl --no-tkinter -c Debug -p x64`
W4_v3.patch Alexander Riccio, 2015-12-17 04:38 review
25878_1.patch steve.dower, 2016-01-16 20:56 review
cpython_xdigits_overrun.PNG Alexander Riccio, 2019-04-19 02:04
34PythonWarnings.txt Alexander Riccio, 2020-03-31 23:38
Messages (24)
msg256494 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-16 00:12
This issue is related to Issue25847.

Compiling at /W4 is generally a good idea. It's an industry best practice, and even though I don't expect disagreement, I'll throw in a few coding standard links: I was surprised to discover that the Windows build of CPython compiles at /W3!

Bumping it up to /W4 produces ~9,000 warnings (up from ~20).

The patch that I'll include with this issue (first iteration) bumps the warning level up to /W4, and I've disabled warnings that (appear) aren't useful. That suppresses ~8,000 of those warnings.

The patch isn't quite yet perfect, as Visual Studio made changes that I didn't ask for, such as:

-    <ClCompile Include="..\PC\bdist_wininst\extract.c">
+    <ClCompile Include="extract.c">

...but that's about it. I had to use `hg diff -w`, because Visual Studio also decided to change the spacing in all of the .vcxproj files.
msg256496 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-16 01:04
The warnings that I've disabled are:

C4054, "'conversion' : from function pointer 'type1' to data pointer 'type2'":

    I disabled 4054because there are lots of void* to (somefuncptr) conversions in CPython. I couldn't see any problems with them, and they all seemed necessary.

C4100, "'identifier' : unreferenced formal parameter":

    I disabled C4100 because there are thousands of places in CPython where function parameters aren't referenced. Some of these could actually be bugs (!!), but right now there are too many benign cases of this to be of use. Commenting out parameter names where they're intentionally not referenced is an elegant way of suppressing this warning while documenting intent.

C4115, "'type' : named type definition in parentheses":

    I disabled C4115 because CPython appears to trigger it in benign conditions. This warning triggers when a function accepts a structure as an argument, and that structure type has not yet been properly declared.

C4127, "conditional expression is constant":

    I disabled C4127 because the do { } while (1) pattern is quite prevalent in CPython, and is unlikely to yield any useful warnings.

C4131, "'function' : uses old-style declarator":

    I disabled C4131 because CPython includes (third party) code that uses the silly old style C function declaration form.

C4152, "non standard extension, function/data ptr conversion in expression":

    I disabled C4152 for the same reason that I disabled C4054.

C4200, "nonstandard extension used : zero-sized array in struct/union":

    I disabled C4200 in a few projects, because this is a very common way of reducing memory allocations. Block allocations are trickier to correctly use, and don't have a standardized, safe, mechanism (not until C++17, at least), but they're just too darn useful & common to warn about every single use.

C4204, "nonstandard extension used : non-constant aggregate initializer":

    I disabled C4204 because CPython frequently initializes a struct with local variables. This is perfectly reasonable, despite ANSI C incompatibility.

C4244, "'conversion' conversion from 'type1' to 'type2', possible loss of data":

    I disabled C4244, if I remember correctly, because all instances appeared safe. I should revisit this one.

C4267, "'var' : conversion from 'size_t' to 'type', possible loss of data":

    I disabled C4267, if I remember correctly, because all instances appeared safe. I should revisit this one.

C4706, "assignment within conditional expression":

    I disabled C4706 because there's lots of code in CPython that allocates memory inside the conditional (i.e. `if (!(self = PyObject_New(Pdata, &Pdata_Type)))` from Pdata_New in _pickle.c), which is perfectly valid code. Even if it makes me nauseous. 

Running `build.bat -d -v --no-ssl --no-tkinter -c Debug -p x64` produces 524 warnings.
msg256497 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-16 01:05
I've added the text build output.
msg256516 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2015-12-16 10:10
The problem with this bug report is that there is little chance that it gets resolved in the near term, and it's quite possible that it will stay open for years. Somebody would have to sit down and start producing patches to fix these warnings correctly, before the actual patch can be applied.

I suggest not to add actual fixes to this issue, but instead create new issues that come with patches at creation, and that each fix a bunch of the warnings (either by warning type, or by code module). These issues could then be added as dependencies of this one. Otherwise, it will be difficult to track what precisely needs to be done on this issue.
msg256548 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-16 22:34
> The problem with this bug report is that there is little chance that it gets resolved in the near term, and it's quite possible that it will stay open for years. Somebody would have to sit down and start producing patches to fix these warnings correctly, before the actual patch can be applied.

That's perfectly reasonable. In the mean time, we may be able to dramatically reduce the timetable by suppressing warnings - C4232 for example - which don't offer anything on a case-by-case basis.

C4232(*), for example, warns any time we use the address of a `dllimport`ed function. mpd_mallocfunc, mpd_free, and friends - see - are assigned malloc, free, etc... but is it safe to ignore?

And then there are warnings that are safe to ignore in certain places, like C4310 in the _Py_ERROR_SURROGATEESCAPE case of PyUnicode_DecodeASCII in unicodeobject.c (

            if (error_handler == _Py_ERROR_REPLACE)
                PyUnicode_WRITE(kind, data, writer.pos, 0xfffd);

...(here, the constant value 0xfffd is cast to a Py_UCS1, truncating it, inside a macro) but are not valid in others, like these insertint calls in PyInit_msvcrt, in msvcrtmodule.c (

    insertint(d, "CRTDBG_FILE_STDERR", (int)_CRTDBG_FILE_STDERR);
    insertint(d, "CRTDBG_FILE_STDOUT", (int)_CRTDBG_FILE_STDOUT);
    insertint(d, "CRTDBG_REPORT_FILE", (int)_CRTDBG_REPORT_FILE);

...(here, _CRTDBG_FILE_STDERR, _CRTDBG_FILE_STDOUT, and _CRTDBG_REPORT_FILE, are _HFILEs, which is a void*, here {((_HFILE)(intptr_t)-4), ((_HFILE)(intptr_t)-5), and ((_HFILE)(intptr_t)-6)} respectively; the void* is truncated to an int in 64 bit builds)

(*)C4232, "nonstandard extension used : 'identifier' : address of dllimport 'dllimport' is not static, identity not guaranteed",  (

I will try to sort a few of the easy categories, so we can discuss more generally.
msg256575 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-17 04:38
Cut out more noisy warnings.
msg256577 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-12-17 04:56
At this point our MSBuild project files are hand-crafted, and are best edited by hand.  You can use the GUI to get a general idea of what the changes should be, but it just creates far too much spam to really be usable.

This should be about a 2 line change, but the current patch is several hundred lines of spam.  Have a look at PCbuild/pyproject.props:41 for where the change should be made.
msg256601 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-17 12:14
Thank you Alexander for your work. I agree that we have to use maximal warning level available in the compiler, and specially disable those warning categories that produces mainly false positives with CPython sources. See also similar issue23545 about GCC.

And of course I concur with Martin that we have first fix all non-false warnings before increase warning level (as we have done for GCC), and it would be better to do this in separate issues.
msg256819 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-22 02:20
> This should be about a 2 line change, but the current patch is several hundred lines of spam.

I agree, but wasn't immediately sure how to do so.

Unfortunately, I've been working on other things, and I'm not sure when I'll be able to finish this.
msg256821 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-12-22 02:41
That's alright, it'll be here whenever you come back to it, unless
somebody else takes it up :)

By the way, my wording was poor in my previous message.  My 'hundreds
of lines of spam' remark was not meant to disparage your patch, but
rather Visual Studio's project editing abilities.  Sorry about that.
msg258410 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-01-16 20:56
I made a new patch to replace all of the existing ones here with the two-line version of the change.

I also suppressed warning 4232 about dllimport addresses (essentially, you may get different values when taking the address of an imported function because of the way thunks are generated at compile time - this doesn't matter when you are calling them, only when comparing them), mainly because there's no other way to suppress them.

All warnings about conversions should be suppressed when you make the conversion explicit, and that's the preferable way to fix those.
msg258420 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-01-16 21:47
Could you explain warning #4232 in the case of libmpdec? I thought
all files are compiled and linked together. Why is the function
msg258430 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-01-16 22:06
libmpdec/memory.c keeps pointers to customisable memory functions (malloc/free) and initialises them to &malloc and &free. These functions are dllimport'd from the CRT, and so they trigger a warning letting you know that "&malloc == &malloc" may not always be true.

(That trivial comparison should always be true, but if you indirect one of the values through a few other modules it may be an address of a different stub function that calls the real malloc, and hence the addresses may not match.)
msg258434 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-01-16 22:39
Ah, thanks!  I guess this Visual Studio behavior is not compatible
with the C-standard, but indeed there are no function pointer
comparisons in libmpdec.
msg258439 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-01-17 02:21
I don't think the C standard necessarily applies or defines what happens when you call/link a __declspec'd function, but it's certainly not going to be the same as static linking.

That said, maybe the subtle nature of this means we should suppress the warning locally with a comment, rather than suppressing it globally.
msg258454 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2016-01-17 09:36
If there are few enough instances, then using a #pragma warning(suppress:4232) is probably the best idea.
msg258460 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-01-17 11:09
I would expect linking to behave "as if" the abstract machine defined
by the standard were executing the code.

We already had one function pointer equality issue due to COMDAT folding,
so I agree that we should leave the warning on.
msg258463 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-01-17 11:34
Pragma added for libmpdec in #26139.
msg273672 - (view) Author: Jim Jewett (Jim.Jewett) * (Python triager) Date: 2016-08-25 17:08
Is there a way to document why certain warnings are being suppressed, in case someone wants to revisit the suppression later?
msg340513 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2019-04-18 21:01
I decided to come back to this after a python meetup last night. By messing with this a bit, building in VS2019 with /W4, I see that fully 2/3rds of the total warnings are from two specific warnings:

C4100 (unreferenced formal parameter)
C4127 (conditional expression is constant)

...This seems to be a stylistic thing across the codebase. If it were a new codebase, I'd simply recommend not giving unreferenced formal parameters a variable name - the typical way of doing it - but there's no way anybody is gonna care enough to comment them out across the whole codebase. C4127 is not A Big Deal, since dispatching based on data type sizes in conditionals is just the easiest way to do The Right Thing in C.

The rest of the warnings are mostly datatype coercions ('=': conversion from 'int' to 'char', possible loss of data), old style declarators, and a bunch of type indirection mismatches ('function': 'volatile int *' differs in indirection to slightly different base types from 'volatile long *'), type cast truncation ('type cast': truncation from 'volatile __int64' to 'PyThreadState *'), named type declarations in parenthesis ('timeval': named type definition in parentheses), and assignments in conditionals (which I don't like, but are not a huge deal).

There really are only a few things that actually look sketchy. For example, in run_child (launcher.c), SetInformationJobObject is passing sizeof(info) as cbJobObjectInformationLength, where it should instead be the local variable rc.
msg340522 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2019-04-19 02:04
One more thing, after I ran code analysis:

This is obviously a potential memory leak:
Warning	C6308	'realloc' might return null pointer: assigning null pointer to 'arr->items', which is passed as an argument to 'realloc', will cause the original memory block to be leaked.
cpython\parser\parsetok.c	38	

I found some sketchy code that isn't obviously correct. Here are a few of the warnings:

Warning	C6294	Ill-defined for-loop:  initial condition does not satisfy test.  Loop body not executed.
\cpython\modules\gcmodule.c	1377	

Warning	C6011	Dereferencing NULL pointer 'cmdline'. See line 230 for an earlier location where this can occur
\cpython\python\preconfig.c	242	
(cmdline is checked for nullness after several uses?)

And finally there's one warning where I have no clue what's going on:

Warning	C6386	Buffer overrun while writing to 'x_digits':  the writable size is '10' bytes, but '286331156' bytes might be written.	
\cpython\objects\longobject.c	2972
msg364634 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2020-03-19 19:45
Ok, so I finally have some proper time to work on this. How would people (who are higher up in python than me, obviously) feel about suppressing most of the warnings via a user macro in Visual Studio? I've found that it's quite easy to add a macro to the project properties (i.e. pyproject), and have it import into the "Disable specific warnings" build options. This keeps our build files hand-crafted (let's keep the hipsters happy!), and consistent. By doing this, I can get it down to ~100 warnings, which are essentially code that we may want to keep an eye on.

The following warnings are essentially always spurious for python. C4100 is
everywhere, and is nearly always benign, but would still require a lot of
careful inspection to determine it its truly on purpose. C4127 is fine since C
doesn't have `constexpr`, let alone `if constexpr`, and it's just easier to
use if statements than  `#ifdef`s everywhere. C4152 is used in every module
exports array. 

C4100 (unreferenced formal parameter)
C4115 'type' : named type definition in parentheses
C4127 conditional expression is constant
C4131 'function' : uses old-style declarator
C4152 non standard extension, function/data ptr conversion in expression

These are usually spurious, since zero-sized arrays are common across the C
world, and is one of several ways of implementing dynamic structs (e.g. unsized
arrays, ANYSIZE_ARRAY, etc...). C4204 and C4221 exist all over the place, and
are probably fine as long as we don't make any lifetime mistakes, which of
course is a solved problem in C. Instances of C4244 should each be individually
inspected carefully since they could be bugs, but there are way too many of
them to allow the warning right now. 

C4200 nonstandard extension used : zero-sized array in struct/union
C4201 nonstandard extension used : nameless struct/union
C4204 nonstandard extension used : non-constant aggregate initializer
C4221 nonstandard extension used : 'identifier' : cannot be initialized using address of automatic variable
C4232 nonstandard extension used : 'identifier' : address of dllimport 'dllimport' is not static, identity not guaranteed 
C4244 'conversion' conversion from 'type1' to 'type2', possible loss of data
C4245 'conversion' : conversion from 'type1' to 'type2', signed/unsigned mismatch

Instances of C4389 should each be individually inspected carefully since they
could be bugs, but there are way too many of them to allow the warning right now.

C4389 'operator' : signed/unsigned mismatch

Instances of C4456 and C4457 are likely ok given the length of many CPython
functions, but should be inspected for mistakes. There are lots of places
where `i` and such are used as variables, and it's there's no way to know
if the author intended it.
C4456 declaration of 'identifier' hides previous local declaration
C4457 declaration of 'identifier' hides function parameter

Instances of C4701 occur pretty frequently around code that initializes C
objects with the funky PyArg_ParseTuple format string mechanism, since the
compiler has no built in support for understanding custom format strings.
Personally, I would support zero-initializing these variables. Would it break
anything? I assume there's a performance impact, but the compiler can't see
that across translation units. If there were some other way to enforce proper
variable initialization other than zeroing, that would work too. We could
ABSOLUTLEY use SAL for this to get the performance benefit of not zero
initializing everything and statically checking if the functions are being used
correctly, but I doubt there's community support for that. 4706 is addressed
already. 4702 is everywhere, and would require a lot of inspection to suppress the warning.

C4701 Potentially uninitialized local variable 'name' used
C4702 unreachable code
C4703 Potentially uninitialized local pointer variable 'name' used
C4706 assignment within conditional expression

I could submit a patch to set up the macro with a few warnings that we can all
agree on, and not turn on W4 yet, or I can submit a patch with those few
warnings disabled and W4 enabled but very noisy builds. What is preferred?

List formatted for macro:
msg364636 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2020-03-19 20:28
Thanks for all that work!

We definitely don't want a noisy build to be merged, but if you submit a PR with only your most confident suppressions then it'll be easier for us to look at the others and see which are scary/not-scary.
msg365438 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2020-03-31 23:38
Ok, so a draft of this produces 34 warnings, but makes way more changes to the .vcxproj and .filters files than I think it should:

Before I submit a PR, I think I should figure out how to change the defaults without Visual Studio adding a bunch of noise to the config - is that reasonable?

The warnings appear essentially innocuous - mixing signed/unsigned bytes, some benign truncation of constant values, and 3 places where local variables shadow the globals, where it looks like the global should've been used - but should be checked nonetheless.

The warnings are in the attached file.
Date User Action Args
2022-04-11 14:58:25adminsetgithub: 70066
2020-03-31 23:38:15Alexander Ricciosetfiles: + 34PythonWarnings.txt

messages: + msg365438
2020-03-19 20:28:05steve.dowersetmessages: + msg364636
2020-03-19 19:45:50Alexander Ricciosetmessages: + msg364634
2019-04-19 02:04:58Alexander Ricciosetfiles: + cpython_xdigits_overrun.PNG

messages: + msg340522
2019-04-18 21:01:59Alexander Ricciosetmessages: + msg340513
2016-08-25 17:08:57Jim.Jewettsetnosy: + Jim.Jewett
messages: + msg273672
2016-01-17 11:34:09skrahsetmessages: + msg258463
2016-01-17 11:09:32skrahsetmessages: + msg258460
2016-01-17 09:36:56Alexander Ricciosetmessages: + msg258454
2016-01-17 02:21:37steve.dowersetmessages: + msg258439
2016-01-16 22:39:59skrahsetmessages: + msg258434
2016-01-16 22:06:15steve.dowersetmessages: + msg258430
2016-01-16 21:47:03skrahsetnosy: + skrah
messages: + msg258420
2016-01-16 20:56:27steve.dowersetfiles: + 25878_1.patch

messages: + msg258410
2015-12-22 02:41:08zach.waresetmessages: + msg256821
2015-12-22 02:20:18Alexander Ricciosetmessages: + msg256819
2015-12-17 12:14:55serhiy.storchakasettype: enhancement
components: + Build
versions: + Python 3.6
nosy: + serhiy.storchaka

messages: + msg256601
stage: patch review
2015-12-17 04:56:31zach.waresetmessages: + msg256577
2015-12-17 04:38:08Alexander Ricciosetfiles: + W4_v3.patch

messages: + msg256575
2015-12-16 22:34:54Alexander Ricciosetmessages: + msg256548
2015-12-16 10:10:07loewissetnosy: + loewis
messages: + msg256516
2015-12-16 01:07:11Alexander Ricciosetcomponents: + Windows
2015-12-16 01:06:52Alexander Ricciosetnosy: + paul.moore, tim.golden, zach.ware, steve.dower
2015-12-16 01:05:22Alexander Ricciosetfiles: + W4_v2_build_output

messages: + msg256497
2015-12-16 01:04:38Alexander Ricciosetfiles: + W4_v2.patch
keywords: + patch
messages: + msg256496
2015-12-16 00:12:35Alexander Ricciocreate