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:

https://www.securecoding.cert.org/confluence/display/c/MSC00-C.+Compile+cleanly+at+high+warning+levels

https://books.google.com/books?id=pDsFCAAAQBAJ&pg=PA557&lpg=PA557&dq=compile+at+highest+warning+level&source=bl&ots=RlKoHFuuWW&sig=mcz-wqdpf3MhiyGZSYKFvpTmd9A&hl=en&sa=X&ved=0ahUKEwjkhN2Rh9_JAhXIth4KHTTbCiMQ6AEIgwEwEQ#v=onepage&q=compile%20at%20highest%20warning%20level&f=false

http://programmers.stackexchange.com/questions/232626/is-it-a-good-practice-to-choose-the-highest-warning-level-in-c-programming

http://ptgmedia.pearsoncmg.com/images/0321113586/items/sutter_item1.pdf


...so 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.

The warnings that I've disabled are:

C4054, "'conversion' : from function pointer 'type1' to data pointer 'type2'": https://msdn.microsoft.com/en-us/library/07d15ax5(v=vs.90).aspx

    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": https://msdn.microsoft.com/en-us/library/26kb9fy0.aspx

    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": https://msdn.microsoft.com/en-us/library/hhzc0806(v=vs.90).aspx

    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": https://msdn.microsoft.com/en-us/library/6t66728h(v=vs.90).aspx

    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": https://msdn.microsoft.com/en-us/library/b92s55e9(v=vs.90).aspx

    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": https://msdn.microsoft.com/en-us/library/bacb5038(v=vs.90).aspx

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


C4200, "nonstandard extension used : zero-sized array in struct/union": https://msdn.microsoft.com/en-us/library/79wf64bc.aspx

    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": https://msdn.microsoft.com/en-us/library/6b73z23c.aspx

    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": https://msdn.microsoft.com/en-us/library/th7a07tz.aspx

    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": https://msdn.microsoft.com/en-us/library/6kck0s93.aspx

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


C4706, "assignment within conditional expression": https://msdn.microsoft.com/en-us/library/7hw7c1he.aspx

    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.

I've added the text build output.

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.

> 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 https://hg.python.org/cpython/file/tip/Modules/_decimal/libmpdec/memory.c#l44 - 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 (https://hg.python.org/cpython/file/tip/Objects/unicodeobject.c#l6897):

            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 (https://hg.python.org/cpython/file/tip/PC/msvcrtmodule.c#l538):

    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",  (https://msdn.microsoft.com/en-us/library/9a1sy630.aspx)

I will try to sort a few of the easy categories, so we can discuss more generally.

Cut out more noisy warnings.

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.

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.

> 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.

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.

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.

Could you explain warning #4232 in the case of libmpdec? I thought
all files are compiled and linked together. Why is the function
imported?

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.)

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.

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.

If there are few enough instances, then using a #pragma warning(suppress:4232) is probably the best idea.

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.

Pragma added for libmpdec in #26139.

Is there a way to document why certain warnings are being suppressed, in case someone wants to revisit the suppression later?

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.

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

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) https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4100?view=vs-2019
C4115 'type' : named type definition in parentheses https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-levels-1-and-4-c4115?view=vs-2019
C4127 conditional expression is constant https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4127?view=vs-2019
C4131 'function' : uses old-style declarator https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4131?view=vs-2019
C4152 non standard extension, function/data ptr conversion in expression https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4152?view=vs-2019

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 https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-levels-2-and-4-c4200?view=vs-2019
C4201 nonstandard extension used : nameless struct/union https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4201?view=vs-2019
C4204 nonstandard extension used : non-constant aggregate initializer https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4204?view=vs-2019
C4221 nonstandard extension used : 'identifier' : cannot be initialized using address of automatic variable https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4221?view=vs-2019
C4232 nonstandard extension used : 'identifier' : address of dllimport 'dllimport' is not static, identity not guaranteed https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4232?view=vs-2019 
C4244 'conversion' conversion from 'type1' to 'type2', possible loss of data https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-levels-3-and-4-c4244?view=vs-2019
C4245 'conversion' : conversion from 'type1' to 'type2', signed/unsigned mismatch https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4245?view=vs-2019


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 https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4389?view=vs-2019


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 https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4456?view=vs-2019
C4457 declaration of 'identifier' hides function parameter https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4457?view=vs-2019


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 https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4701?view=vs-2019
C4702 unreachable code https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4702?view=vs-2019
C4703 Potentially uninitialized local pointer variable 'name' used https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4703?view=vs-2019
C4706 assignment within conditional expression https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4706?view=vs-2019


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:
4100;4115;4127;4131;4152;4200;4201;4204;4221;4232;4244;4245;4389;4456;4457;4701;4702;4703;4706

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.

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: https://github.com/ariccio/cpython/commit/60152aa065a3ad861f0359a8ada7f2fbc83a3933

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.