New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CPython on Windows builds with /W3, not /W4 #70066
Comments
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: 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:
...but that's about it. I had to use |
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
C4100, "'identifier' : unreferenced formal parameter": https://msdn.microsoft.com/en-us/library/26kb9fy0.aspx
C4115, "'type' : named type definition in parentheses": https://msdn.microsoft.com/en-us/library/hhzc0806(v=vs.90).aspx
C4127, "conditional expression is constant": https://msdn.microsoft.com/en-us/library/6t66728h(v=vs.90).aspx
C4131, "'function' : uses old-style declarator": https://msdn.microsoft.com/en-us/library/b92s55e9(v=vs.90).aspx
C4152, "non standard extension, function/data ptr conversion in expression": https://msdn.microsoft.com/en-us/library/bacb5038(v=vs.90).aspx
C4200, "nonstandard extension used : zero-sized array in struct/union": https://msdn.microsoft.com/en-us/library/79wf64bc.aspx
C4204, "nonstandard extension used : non-constant aggregate initializer": https://msdn.microsoft.com/en-us/library/6b73z23c.aspx
C4244, "'conversion' conversion from 'type1' to 'type2', possible loss of data": https://msdn.microsoft.com/en-us/library/th7a07tz.aspx
C4267, "'var' : conversion from 'size_t' to 'type', possible loss of data": https://msdn.microsoft.com/en-us/library/6kck0s93.aspx
C4706, "assignment within conditional expression": https://msdn.microsoft.com/en-us/library/7hw7c1he.aspx
Running |
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. |
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 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):
...(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 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. |
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 By the way, my wording was poor in my previous message. My 'hundreds |
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 bpo-4232 in the case of libmpdec? I thought |
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 |
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 We already had one function pointer equality issue due to COMDAT folding, |
Pragma added for libmpdec in bpo-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) ...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: 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. Warning C6011 Dereferencing NULL pointer 'cmdline'. See line 230 for an earlier location where this can occur 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. |
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 C4100 (unreferenced formal parameter) https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4100?view=vs-2019 These are usually spurious, since zero-sized arrays are common across the C 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 Instances of C4389 should each be individually inspected carefully since they 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 Instances of C4701 occur pretty frequently around code that initializes C 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 I could submit a patch to set up the macro with a few warnings that we can all List formatted for macro: |
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: ariccio@60152aa 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. |
build.bat -d -v --no-ssl --no-tkinter -c Debug -p x64
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: