CPython on Windows builds with /W3, not /W4

[ariccio]

BPO	25878
Nosy	@loewis, @pfmoore, @tjguk, @skrah, @JimJJewett, @zware, @serhiy-storchaka, @zooba, @ariccio
Files	W4_v2.patch W4_v2_build_output: build.bat -d -v --no-ssl --no-tkinter -c Debug -p x64 W4_v3.patch 25878_1.patch cpython_xdigits_overrun.PNG 34PythonWarnings.txt

^{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 = None
created_at = <Date 2015-12-16.00:12:35.811>
labels = ['type-feature', 'OS-windows', 'build']
title = 'CPython on Windows builds with /W3, not /W4'
updated_at = <Date 2020-03-31.23:38:15.578>
user = 'https://github.com/ariccio'

bugs.python.org fields:

activity = <Date 2020-03-31.23:38:15.578>
actor = 'Alexander Riccio'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Build', 'Windows']
creation = <Date 2015-12-16.00:12:35.811>
creator = 'Alexander Riccio'
dependencies = []
files = ['41321', '41322', '41333', '41635', '48276', '49017']
hgrepos = []
issue_num = 25878
keywords = ['patch']
message_count = 24.0
messages = ['256494', '256496', '256497', '256516', '256548', '256575', '256577', '256601', '256819', '256821', '258410', '258420', '258430', '258434', '258439', '258454', '258460', '258463', '273672', '340513', '340522', '364634', '364636', '365438']
nosy_count = 9.0
nosy_names = ['loewis', 'paul.moore', 'tim.golden', 'skrah', 'Jim.Jewett', 'zach.ware', 'serhiy.storchaka', 'steve.dower', 'Alexander Riccio']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue25878'
versions = ['Python 3.6']

[ariccio]

This issue is related to bpo-25847.

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.

[ariccio]

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.

[ariccio]

I've added the text build output.

[loewis]

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.

[ariccio]

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

[ariccio]

Cut out more noisy warnings.

[zware]

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.

[serhiy-storchaka]

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

[ariccio]

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.

[zware]

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.

[zooba]

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.

[skrah]

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