bpo-9566: Fix some Windows x64 compiler warnings #2492
Conversation
|
@segevfiner, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zware, @serhiy-storchaka and @tiran to be potential reviewers. |
|
Oh, this change is big. I will take time to review it. Reviewers must be careful. The goal is not to make warnings quiet, but to make sure that warnings didn't show a real bug. Can you please revert change son ssl.c and create a dedicated review. Since it's a sensitive module, I would prefer to have more reviewers on this specific file. |
This reverts commit a639001.
segevfiner
changed the title
|
I merged #2890 because it looked fine. Despite @Haypo's request for changes on the SSL one, I think it's alright. The others probably need to be checked by people closer to the modified code (particularly dtrace, though I'm not sure who is the best person there, given it's a non-functional change...) |
|
Sorry, but each time I open this PR, I say "hum, it's too big, I will try to review it next time"... :-p |
|
Okay, I reviewed it again and I'm still happy, so I'm going to merge. |
|
@segevfiner I think all of your PRs are merged now apart from #2852, right? |
|
@zooba The ones related to Windows x64 warnings. Yes. |
I tried to fix the x64 compiler warnings on Windows. Here is a list of the ones I encountered: python-x64-warnings.txt
The following remain:
LINK : warning LNK4013: image size 0x29B000 exceeds specified maximum 0x200000Caused by externals\tcl-core-8.6.6.0\win\coffbase.txt not giving enough room for an x64 debug build.
– "Fixed" by pre-building tcl in bpo-30916 ([bpo-30916] Pre-build OpenSSL and Tcl/Tk for Windows #2688)
..\Modules\gcmodule.c(1085): warning C4244: 'function': conversion from 'Py_ssize_t' to 'int', possible loss of dataCaused by DTrace stubs being defined as taking
int. It might be correct to change them to take a larger type since it's possible the real ones take something bigger, but I don't know what the real ones take...EDIT: The real one takes a long (Include/pydtrace.d:12), which is probably 64-bit on architectures where DTrace is available but it is 32-bit on Windows x64. So just changing the stub to the correct type won't help (Though I think we can simply change the stub to a larger integer type without it causing issues...) It might be possible to redefine the real probe to take a larger integral type (It really needs
Py_ssize_tbut I'm not sure we can use it there. But I'm not familiar with enough...– bpo-9566: Silence warning in gcmodule.c caused by pydtrace.h #2852
..\Modules\_tracemalloc.c(88): warning C4359: '<unnamed-tag>': Alignment specifier is less than actual alignment (8), and will be ignored.This optimization doesn't really work as is. Need to use #pragma pack instead.
– bpo-31018 (bpo-31018: Switch to #pragma pack from __declspec(align) in _tracemalloc #2848)
..\Python\getargs.c(2058): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of dataI'm not sure about this one.
– bpo-9566: Silence a warning in Python/getargs.c (Windows x64) #2890
peephole.c warningsThey seem to be related to mixing opcode args which are unsigned int with size_t/Py_ssize_t.
..\Modules\_ssl.c(2947): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of dataThe Py_buffer can theoretically be larger... But I'm not sure that the protocol even supports that...
Should this be casted away or fixed some other way??
..\Modules\_ssl.c(4158): warning C4244: 'function': conversion from 'Py_ssize_t' to 'int', possible loss of dataSomeone can theoretically pass a buffer big enough... But I doubt such a buffer is going to be written at once...
Should this be casted away...?
cc @zooba
https://bugs.python.org/issue9566