bpo-9566: Fix some Windows x64 compiler warnings by segevfiner · Pull Request #2492 · python/cpython

@segevfiner

@segevfiner segevfiner commented

Jun 29, 2017

edited by serhiy-storchaka

Loading

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 0x200000
    Caused 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 data
    Caused 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_t but 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 data
    I'm not sure about this one.
    bpo-9566: Silence a warning in Python/getargs.c (Windows x64) #2890
  • peephole.c warnings
    They 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 data
    The 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??
    – Split to bpo-9566: Fixed _ssl module warnings #2495
  • ..\Modules\_ssl.c(4158): warning C4244: 'function': conversion from 'Py_ssize_t' to 'int', possible loss of data
    Someone 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...?
    – Split to bpo-9566: Fixed _ssl module warnings #2495

cc @zooba

https://bugs.python.org/issue9566

@mention-bot

@vstinner

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.

@segevfiner

zooba

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but wait for @Haypo to approve too.

@segevfiner

@segevfiner

@segevfiner segevfiner changed the title [WIP] bpo-9566: Fix some Windows x64 compiler warnings bpo-9566: Fix some Windows x64 compiler warnings

Jul 24, 2017

@zooba

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

@vstinner

Sorry, but each time I open this PR, I say "hum, it's too big, I will try to review it next time"... :-p

@zooba

Okay, I reviewed it again and I'm still happy, so I'm going to merge.

@zooba

@segevfiner

@zooba The ones related to Windows x64 warnings. Yes.

@segevfiner segevfiner deleted the bpo-9566-x64-compiler-warnings branch

July 26, 2017 22:43