classification
Title: CPython not using Visual Studio code analysis!
Type: enhancement Stage: needs patch
Components: Build, Windows Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Alexander Riccio, christian.heimes, loewis, paul.moore, r.david.murray, steve.dower, terry.reedy, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2015-12-12 05:59 by Alexander Riccio, last changed 2016-08-27 14:48 by BreamoreBoy.

Files
File name Uploaded Description Edit
EnableCodeAnalysis.patch Alexander Riccio, 2015-12-14 22:56 Patch to add `--enable-code-analysis` flag to build.bat, very noisy build. review
CPythonW3.PNG Alexander Riccio, 2015-12-15 00:33
Messages (21)
msg256265 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-12 05:59
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 issue25386, but noticed this, ran a (partial) build with /analyze, and ended up filing three bugs instead (Issue25844, Issue25845, Issue25846) 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?
msg256286 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-12-12 15:58
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.
msg256370 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-14 06:32
> 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!
msg256388 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-12-14 16:19
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 :)
msg256419 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-14 22:47
> 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.
msg256420 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-12-14 22:53
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.
msg256421 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-14 22:56
Yup, the very naive version works.
msg256422 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-12-14 23:23
There are all ready numerous compiler warnings in the Windows builds, see #9566, #18295 and #18407.
msg256425 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-12-14 23:52
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.
msg256426 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-14 23:59
> 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]
msg256427 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-15 00:27
Actually, hmm... the very naive version *DOES NOT* work. Grr.
msg256428 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-15 00:33
Hold on... CPython builds at /W3???!? What is this madness??!?
msg256429 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-12-15 01:11
In which direction do you find us to be mad?
msg256432 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-15 02:38
> 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!
msg256490 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-15 23:40
I'll open up a new issue for /W4, and deal with that first.
msg256498 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-12-16 01:06
See Issue25878.
msg273585 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-08-24 18:44
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?
msg273616 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-08-25 01:21
> (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.
msg273634 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-08-25 07:42
+1, if somebody is able to find time. Coverity Scan checks only X86_64 Linux builds.
msg273653 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-08-25 14:57
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.
msg273658 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-08-25 15:31
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.
History
Date User Action Args
2016-08-27 14:48:30BreamoreBoysetnosy: - BreamoreBoy
2016-08-25 15:31:27christian.heimessetmessages: + msg273658
2016-08-25 14:57:28steve.dowersetmessages: + msg273653
2016-08-25 07:42:24christian.heimessetmessages: + msg273634
2016-08-25 01:21:17steve.dowersetmessages: + msg273616
2016-08-24 18:44:04terry.reedysetnosy: + terry.reedy
messages: + msg273585
2015-12-16 01:06:11Alexander Ricciosetmessages: + msg256498
2015-12-15 23:40:21Alexander Ricciosetmessages: + msg256490
2015-12-15 02:38:16Alexander Ricciosetmessages: + msg256432
2015-12-15 01:11:13zach.waresetmessages: + msg256429
2015-12-15 00:33:05Alexander Ricciosetfiles: + CPythonW3.PNG

messages: + msg256428
2015-12-15 00:27:30Alexander Ricciosetmessages: + msg256427
2015-12-15 00:00:00Alexander Ricciosetmessages: + msg256426
2015-12-14 23:52:45steve.dowersetmessages: + msg256425
2015-12-14 23:23:05BreamoreBoysetnosy: + BreamoreBoy
messages: + msg256422
components: + Windows
2015-12-14 22:56:46Alexander Ricciosetfiles: + EnableCodeAnalysis.patch
keywords: + patch
messages: + msg256421
2015-12-14 22:53:47r.david.murraysetmessages: + msg256420
2015-12-14 22:47:56Alexander Ricciosetmessages: + msg256419
2015-12-14 16:19:26r.david.murraysettype: enhancement
stage: needs patch
messages: + msg256388
versions: + Python 3.6
2015-12-14 06:32:39Alexander Ricciosetmessages: + msg256370
components: + Build, - Windows
2015-12-13 04:30:24brett.cannonsetcomponents: + Windows, - Build
2015-12-13 01:23:53rhettingersetnosy: + loewis
2015-12-12 15:58:18r.david.murraysetnosy: + r.david.murray, christian.heimes
messages: + msg256286
2015-12-12 05:59:47Alexander Ricciocreate