Opened 3 years ago

Closed 3 years ago

#5373 defect closed fixed (fixed)

Make iocp 64-bit clean

Reported by: antoine Owned by: jesstess
Priority: normal Milestone:
Component: core Keywords:
Cc: solipsis@…, jesstess Branch: branches/iocp-64bit-5373-2
(diff, github, buildbot, log)
Author: antoine Launchpad Bug:

Description

iocpsupport is in many places 64-bit unclean. This is due to mismatching declarations. First, HANDLE, SOCKET and ULONG_PTR are really pointer-sized, not int- or long- sized. Second, Cython always defines PY_SSIZE_T_CLEAN, which means functions such as PyObject_AsReadBuffer take a "Py_ssize_t *" parameter, not an "int *". Here is a patch. I've verified that it compiles fine under 64-bit Windows (it reduces the number of compiler warnings a lot), and twisted.internet.test passes fine with "-r iocp".

Attachments (2)

iocp64.patch (253.5 KB) - added by antoine 3 years ago.
iocp64-2.patch (254.4 KB) - added by antoine 3 years ago.

Download all attachments as: .zip

Change History (13)

Changed 3 years ago by antoine

comment:1 Changed 3 years ago by antoine

  • Cc solipsis@… added
  • Keywords review added

(I've left the generated iocpsupport.c changes in the patch. Perhaps you want me to remove them for ease of review)

comment:2 Changed 3 years ago by jesstess

  • Owner set to jesstess

comment:3 Changed 3 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/iocp-64bit-5373

(In [33236]) Branching to 'iocp-64bit-5373'

comment:4 Changed 3 years ago by jesstess

(In [33237]) Apply iocp64.patch by antoine.

refs #5373

comment:5 Changed 3 years ago by jesstess

  • Author changed from jesstess to antoine
  • Cc jesstess added
  • Keywords review removed
  • Owner changed from jesstess to antoine

Thanks for the detailed bug report and patch, antoine!

Here are the buildbot results for this branch.

Comparing the windows7-64-py2.7 compile warnings from a recent trunk build and this branch:

you'll see that a bunch of warnings have gone away, which is great, but 1 warning did get displaced instead of fixed:

twisted/internet/iocpreactor/iocpsupport/iocpsupport.c(3206) : warning C4244: '=' : conversion from 'Py_ssize_t' to 'int', possible loss of data

is now

twisted/internet/iocpreactor/iocpsupport/iocpsupport.c(3438) : warning C4244: 'function' : conversion from 'Py_ssize_t' to 'DWORD', possible loss of data

Can you provide a patch against this branch for this warning in wsarecv.pxi:recv? After that I think this will be ready to merge.

Thanks again!

comment:6 Changed 3 years ago by antoine

Ok, here is an updated patch. I've only verified it compiles fine (it differs by a single cast from the previous patch).

Changed 3 years ago by antoine

comment:7 Changed 3 years ago by jesstess

  • Author changed from antoine to jesstess, antoine
  • Branch changed from branches/iocp-64bit-5373 to branches/iocp-64bit-5373-2

(In [33238]) Branching to 'iocp-64bit-5373-2'

comment:8 Changed 3 years ago by jesstess

(In [33239]) Apply patch iocp64-2.patch by antoine.

refs #5373

comment:9 Changed 3 years ago by jesstess

  • Author changed from jesstess, antoine to antoine
  • Keywords review added
  • Owner changed from antoine to jesstess

comment:10 Changed 3 years ago by jesstess

  • Keywords review removed

Thanks for the new patch, antoine.

In the future, please submit subsequent patches again the branch instead of against trunk. Please also add the 'review' keyword when the ticket is ready to be reviewed again.

Here are the buildbot results for the new branch. Everything looks good, and the displaced compiler warning is gone:

I think this is ready to merge!

comment:11 Changed 3 years ago by jesstess

  • Resolution set to fixed
  • Status changed from new to closed

(In [33241]) Merge iocp-64bit-5373-2

Author: antoine
Reviewer: jesstess
Fixes: #5373

Fix some iocpsupport declarations causing 64-bit compile warnings.

Note: See TracTickets for help on using tickets.