CPython not using Visual Studio code analysis!

[ariccio]

BPO	25847
Nosy	@loewis, @terryjreedy, @pfmoore, @tiran, @tjguk, @bitdancer, @zware, @zooba, @ariccio
Files	EnableCodeAnalysis.patch: Patch to add --enable-code-analysis flag to build.bat, very noisy build. CPythonW3.PNG

^{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-12.05:59:47.347>
labels = ['type-feature', 'OS-windows', 'build']
title = 'CPython not using Visual Studio code analysis!'
updated_at = <Date 2016-08-27.14:48:30.858>
user = 'https://github.com/ariccio'

bugs.python.org fields:

activity = <Date 2016-08-27.14:48:30.858>
actor = 'BreamoreBoy'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Build', 'Windows']
creation = <Date 2015-12-12.05:59:47.347>
creator = 'Alexander Riccio'
dependencies = []
files = ['41311', '41312']
hgrepos = []
issue_num = 25847
keywords = ['patch']
message_count = 21.0
messages = ['256265', '256286', '256370', '256388', '256419', '256420', '256421', '256422', '256425', '256426', '256427', '256428', '256429', '256432', '256490', '256498', '273585', '273616', '273634', '273653', '273658']
nosy_count = 9.0
nosy_names = ['loewis', 'terry.reedy', 'paul.moore', 'christian.heimes', 'tim.golden', 'r.david.murray', 'zach.ware', 'steve.dower', 'Alexander Riccio']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue25847'
versions = ['Python 3.6']

[ariccio]

Visual Studio comes with static analysis, enabled by /analyze (command line) or "Code analysis" in the project configuration dialog. Currently, none of the CPython projects in PCbuild have Code Analysis turned on, in any configuration.

I was going to write my first patch, for bpo-25386, but noticed this, ran a (partial) build with /analyze, and ended up filing three bugs instead (bpo-25844, bpo-25845, bpo-25846) from bugs /analyze found.

There's quite a bad signal-to-noise ratio at the moment, as there's lots of variable shadowing, and there's lots of code that /analyze doesn't understand is benign (parsing a tuple into a variable confuses /analyze), but there is also lots of code that isn't *obviously* incorrect.

Of the code that's not obviously incorrect, /analyze usually complains about possibly out-of-bounds reads in very complex conditions, and I really can't tell. Some assertions would probably help.

Thoughts?

[bitdancer]

Is analyze something that can be used from the command line only, or does it require the GUI? Also, we aren't likely to make the code more complex in order to deal with shortcomings in analyze's algorithms (meaning in that case we couldn't turn it on automatically). For valgrind, for example, we have an exceptions list, and similarly for covarity. Does analyze support something similar?

I'm surprised it is catching things that coverity doesn't.

[ariccio]

Is analyze something that can be used from the command line only, or does it require the GUI?

You can do it from the command line - Chrome/chromium makes use of it as such.
See: https://code.google.com/p/chromium/issues/detail?id=427616

The /analyze option is documented here:
https://msdn.microsoft.com/en-us/library/ms173498.aspx

/analyze:WX- Prevents compilation failure when compiling with /WX (warn as errors) and /analyze warnings are disabled the same way that normal warnings are.

For example, /analyze an extremely large number of variable shadowing issues, which I think should be suppressed (as CPython's code base tolerates them?), to get to the more important issues.

Also, we aren't likely to make the code more complex in order to deal with shortcomings in analyze's algorithms

I assume you're referring to the out-of-bounds in complex conditions? I can't imagine how making the code *more* complex would help :)

I'm surprised it is catching things that coverity doesn't.

Every tool has its strengths and weaknesses; I am, however surprised that coverity didn't catch these issues, as they're common, and platform agnostic.

/analyze can pick up many issues that coverity doesn't, simply because /analyze understands SAL, so it understands how the Windows API is supposed to be used.

Also: Of the three issues that I opened, one is already fix, and two are in the pipeline. Impressive!

[bitdancer]

OK, let's move this to patch needed, then, and see if anyone is ambitious enough to do the work needed to make it useful to us :)

[ariccio]

OK, let's move this to patch needed, then, and see if anyone is ambitious enough to do the work needed to make it useful to us :)

I can try and hack it in, just as proof of concept. I think I should just be able to add something like:

/p:EnablePREfast=%EnablePREfast%^

...along with something like:

if "%~1"=="--enable-code-analysis" (set EnablePREfast=true) & shift & goto CheckOpts

God I hate batch scripting.

[bitdancer]

It seems to me that adding the CLI flag is the least of the work. You then have to make it compile cleanly :) (That's why I said 'ambitious enough'). That is, (as I undersatnd it) we've done a lot of work to not have compiler warnings generated during compilation, and we don't want to backtrack on that. Over time we've cleaned up the code so that we could make stricter compile flags the default, so this would be in that tradition.

[ariccio]

Yup, the very naive version works.

[BreamoreBoy]

There are all ready numerous compiler warnings in the Windows builds, see bpo-9566, bpo-18295 and bpo-18407.

[zooba]

Doing the work to clean up the warnings really has to come second, ultimately.

The buildbot script also should be updated to pass that option.

[ariccio]

That is, (as I undersatnd it) we've done a lot of work to not have compiler warnings generated during compilation, and we don't want to backtrack on that.

Well, as-is, simply building as x64 generates a bunch of warnings, so it's not *quite* clean. I totally understand the need to keep warning-clean, but, well:

..\Modules\_pickle.c(5087): warning C4244: 'function': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Modules\posixmodule.c(3321): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Modules\_tracemalloc.c(67): warning C4359: '<unnamed-tag>': Alignment specifier is less than actual alignment (8), and will be ignored. [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Modules\zipimport.c(914): warning C4244: 'function': conversion from 'Py_ssize_t' to 'long', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Objects\codeobject.c(111): warning C4244: '=': conversion from 'Py_ssize_t' to 'unsigned char', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Objects\funcobject.c(635): warning C4244: 'function': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Objects\funcobject.c(636): warning C4244: 'function': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\PC\winreg.c(885): warning C4311: 'type cast': pointer truncation from 'void *' to 'DWORD' [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\PC\getpathp.c(144): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\PC\msvcrtmodule.c(391): warning C4312: 'type cast': conversion from 'int' to '_HFILE' of greater size [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\PC\msvcrtmodule.c(391): warning C4311: 'type cast': pointer truncation from '_HFILE' to 'long' [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\PC\msvcrtmodule.c(538): warning C4311: 'type cast': pointer truncation from '_HFILE' to 'int' [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\PC\msvcrtmodule.c(539): warning C4311: 'type cast': pointer truncation from '_HFILE' to 'int' [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\PC\msvcrtmodule.c(540): warning C4311: 'type cast': pointer truncation from '_HFILE' to 'int' [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\ceval.c(4826): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\ceval.c(5021): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\compile.c(480): warning C4312: 'type cast': conversion from 'unsigned int' to 'void *' of greater size [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\compile.c(481): warning C4312: 'type cast': conversion from 'unsigned int' to 'void *' of greater size [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\compile.c(482): warning C4312: 'type cast': conversion from 'unsigned int' to 'void *' of greater size [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\compile.c(3155): warning C4244: 'initializing': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\compile.c(3385): warning C4244: 'initializing': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\peephole.c(482): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\peephole.c(535): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\peephole.c(553): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\peephole.c(581): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\peephole.c(592): warning C4244: '=': conversion from 'Py_ssize_t' to 'unsigned char', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\peephole.c(625): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Python\peephole.c(639): warning C4244: '-=': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\pythoncore.vcxproj]

..\Modules\expat\xmlparse.c(1844): warning C4244: 'return': conversion from '__int64' to 'XML_Index', possible loss of data [C:\PythonDev\repo\PCbuild\_elementtree.vcxproj

..\PC\_msi.c(969): warning C4312: 'type cast': conversion from 'int' to 'LPCSTR' of greater size [C:\PythonDev\repo\PCbuild\_msi.vcxproj]

..\PC\_msi.c(1035): warning C4311: 'type cast': pointer truncation from 'LPCTSTR' to 'int' [C:\PythonDev\repo\PCbuild\_msi.vcxproj]

..\PC\_msi.c(1036): warning C4311: 'type cast': pointer truncation from 'LPCTSTR' to 'int' [C:\PythonDev\repo\PCbuild\_msi.vcxproj]

..\PC\_msi.c(1037): warning C4311: 'type cast': pointer truncation from 'LPCTSTR' to 'int' [C:\PythonDev\repo\PCbuild\_msi.vcxproj]

..\PC\_msi.c(1038): warning C4311: 'type cast': pointer truncation from 'LPCTSTR' to 'int' [C:\PythonDev\repo\PCbuild\_msi.vcxproj]

..\PC\_msi.c(1039): warning C4311: 'type cast': pointer truncation from 'LPCTSTR' to 'int' [C:\PythonDev\repo\PCbuild\_msi.vcxproj]

..\Modules\overlapped.c(977): warning C4996: 'WSAStringToAddressA': Use WSAStringToAddressW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_overlapped.vcxproj]

..\Modules\overlapped.c(988): warning C4996: 'WSAStringToAddressA': Use WSAStringToAddressW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_overlapped.vcxproj]

..\Modules\socketmodule.c(1014): warning C4996: 'inet_addr': Use inet_pton() or InetPton() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(1850): note: see declaration of 'inet_addr'

..\Modules\socketmodule.c(2439): warning C4244: 'function': conversion from 'Py_ssize_t' to 'int', possible loss of data [C:\PythonDev\repo\PCbuild\_socket.vcxproj]

..\Modules\socketmodule.c(4040): warning C4996: 'WSADuplicateSocketA': Use WSADuplicateSocketW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(2872): note: see declaration of 'WSADuplicateSocketA'

..\Modules\socketmodule.c(4253): warning C4996: 'WSASocketA': Use WSASocketW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(3457): note: see declaration of 'WSASocketA'

..\Modules\socketmodule.c(4286): warning C4996: 'WSASocketA': Use WSASocketW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(3457): note: see declaration of 'WSASocketA'

..\Modules\socketmodule.c(4694): warning C4996: 'gethostbyname': Use getaddrinfo() or GetAddrInfoW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(2238): note: see declaration of 'gethostbyname'

..\Modules\socketmodule.c(4792): warning C4996: 'gethostbyaddr': Use getnameinfo() or GetNameInfoW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(2216): note: see declaration of 'gethostbyaddr'

..\Modules\socketmodule.c(4922): warning C4996: 'WSADuplicateSocketA': Use WSADuplicateSocketW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(2872): note: see declaration of 'WSADuplicateSocketA'

..\Modules\socketmodule.c(4925): warning C4996: 'WSASocketA': Use WSASocketW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(3457): note: see declaration of 'WSASocketA'

..\Modules\socketmodule.c(5216): warning C4996: 'inet_addr': Use inet_pton() or InetPton() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(1850): note: see declaration of 'inet_addr'

..\Modules\socketmodule.c(5259): warning C4996: 'inet_ntoa': Use inet_ntop() or InetNtop() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(1868): note: see declaration of 'inet_ntoa'

..\Modules\socketmodule.c(5333): warning C4996: 'WSAStringToAddressA': Use WSAStringToAddressW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]
C:\Program Files (x86)\Windows Kits\8.1\Include\um\winsock2.h(3623): note: see declaration of 'WSAStringToAddressA'

..\Modules\socketmodule.c(5476): warning C4996: 'WSAAddressToStringA': Use WSAAddressToStringW() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings [C:\PythonDev\repo\PCbuild\_socket.vcxproj]

..\Modules\_ctypes\libffi_msvc\prep_cif.c(170): warning C4267: '+=': conversion from 'size_t' to 'unsigned int', possible loss of data [C:\PythonDev\repo\PCbuild\_ctypes.vcxproj]

[ariccio]

Actually, hmm... the very naive version *DOES NOT* work. Grr.

[ariccio]

Hold on... CPython builds at /W3???!? What is this madness??!?

[zware]

In which direction do you find us to be mad?

[ariccio]

In which direction do you find us to be mad?

That's really quite a low warning level! For a large project, I can't imagine anything less than /W4!

[ariccio]

I'll open up a new issue for /W4, and deal with that first.

[ariccio]

See bpo-25878.

[terryjreedy]

Like Mark, I have noticed that there are enough ignored VS warnings now to not think we need more.

Every time socketmodule is recompiled, for instance, I see all the scary deprecation warnings. These are Windows-specific. Should the code be changed or should the warnings be silenced with _WINSOCK_DEPRECATED_NO_WARNINGS, as suggested?

Other warnings appear to come from code compiled on all systems. I presume such code is usually developed on Linux. Is such code compiling warning free on gcc? Are there conflicting demands and opinions from gcc and vs?

I am a little confused on the actual policy for compiling on Windows. I believe Victor Stinner has done some patches to stop them, but...
(Steve)> Doing the work to clean up the warnings really has to come second, ultimately.
Second to what?

Should I just close my eyes when running build.bat?

[zooba]

(Steve)> Doing the work to clean up the warnings really has to come second, ultimately.
Second to what?

Second to enabling all the warnings, which is where the thread started. I need to break my nasty habit on here of not quoting posts, but it's such a pain when I'm on my phone (latest post is a long way away and selection never works properly in this text box for some reason).

I'd like to see the WinSock warnings fixed as there are security implications around them (no proven vulnerabilities, but the point of the new APIs is to make that even less likely). I already disabled other warnings that didn't have the same risks.

Adding more checks probably isn't a bad idea. MSVC doesn't entirely overlap with coverity, so it could find things they miss. But that would create work for someone to go fix them.

[tiran]

+1, if somebody is able to find time. Coverity Scan checks only X86_64 Linux builds.

[zooba]

Does it only check built code? I thought it would statically analyze everything? Is MS_WINDOWS code excluded, because that would explain why it "misses" issues.

[tiran]

Coverity Scan has two steps. In the first step the code is compiled with the coverity tool chain. It's a wrapper around the preprocessor and compiler. Next up the result is uploaded and analyzed by a service.

So yes, the project needs to be build before it can be analyzed.