This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Still broken ctypes calling convention on MSVC / 64-bit Windows (large structs)
Type: behavior Stage: resolved
Components: ctypes, Windows Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Igor Kudrin, christian.heimes, doko, eryksun, mark.dickinson, meador.inge, paul.moore, serhiy.storchaka, steve.dower, tim.golden, vinay.sajip, vstinner, zach.ware
Priority: high Keywords:

Created on 2017-02-15 09:48 by Igor Kudrin, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 168 merged vinay.sajip, 2017-02-19 06:44
PR 220 merged vinay.sajip, 2017-02-21 19:44
PR 221 merged vinay.sajip, 2017-02-21 19:47
PR 8625 merged vstinner, 2018-08-02 14:02
PR 8626 merged vstinner, 2018-08-02 15:20
Messages (15)
msg287831 - (view) Author: Igor Kudrin (Igor Kudrin) Date: 2017-02-15 09:48
It looks like resolving https://bugs.python.org/issue20160 didn't fix all the issues. When a large struct is used as a parameter, a reference to the object itself is passed to the function, not a reference to a temporary copy, as it is required in https://msdn.microsoft.com/en-us/library/zthk2dkh.aspx.

Here is the reproduction sample:

=== Sample.c: ===
typedef struct
{
  int v[3];
} Data;

__declspec(dllexport) void Cripple(Data data)
{
  data.v[0] = 0;
}

=== Sample.py ===
from ctypes import *

class Data(Structure):
    _fields_ = [("v", c_int * 3)]

lib = cdll.LoadLibrary('Sample.dll')
getattr(lib, 'Cripple').argtypes = [Data]

data = Data()
data.v[0] = 42
lib.Cripple(data)
assert data.v[0] == 42
print "OK"

=== repro.cmd ===
call "%VS140COMNTOOLS%/../../VC/vcvarsall.bat" x86_amd64
cl /LD Sample.c
python Sample.py
msg287878 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2017-02-15 18:34
Our support of passing structs and unions by value is really bad. Passing this particular struct by value is also broken on 64-bit Unix, but instead because it's small. It crashes Python due to an abort(). See issue 22273.
msg287959 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2017-02-16 17:13
Just trying to confirm my understanding - in both this issue and #22273, the example struct has a single array member. Is the problem confined just to how ctypes describes arrays in structs/unions to libffi? The comment on #22273 seems to suggest so.
msg287963 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-02-16 17:44
The contents of the struct is just being big enough to reach the point where it is passed by reference rather than by value.

This issue is pointing out that it should be a reference to a temporary copy of the struct, so that if the called function modifies the argument then it doesn't affect the caller.
msg288031 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2017-02-17 18:06
This issue is more straight-forward than #22273. ISTM, we just have to copy large values. For example, in ffi_call, we'd iterate over the arguments and alloca and memcpy the large values prior to calling ffi_call_AMD64:

    case FFI_SYSV:
      /* If a single argument takes more than 8 bytes,
         then a copy is passed by reference. */
      for (unsigned i = 0; i < cif->nargs; i++) {
          size_t z = cif->arg_types[i]->size;
          if (z > 8) {
              void *temp = alloca(z);
              memcpy(temp, avalue[i], z);
              avalue[i] = temp;
          }
      }
      /*@-usedef@*/
      return ffi_call_AMD64(ffi_prep_args, &ecif, cif->bytes,
                            cif->flags, ecif.rvalue, fn);
      /*@=usedef@*/
      break;
msg288111 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2017-02-19 06:09
When adding the fix for #20160 I had added a test involving passing a struct by value to a Python callback. I modified that test to update the struct in the callback, and check that the passed-in struct hadn't changed.

    def test_callback_large_struct(self):
        class Check: pass

        class X(Structure):
            _fields_ = [
                ('first', c_ulong),
                ('second', c_ulong),
                ('third', c_ulong),
            ]

        def callback(check, s):
            check.first = s.first
            check.second = s.second
            check.third = s.third
            # See issue #29565.
            # The structure should be passed by value, so
            # any changes to it should not be reflected in
            # the value passed
            s.first = s.second = s.third = 0x0badf00d

        check = Check()
        s = X()
        s.first = 0xdeadbeef
        s.second = 0xcafebabe
        s.third = 0x0bad1dea

        CALLBACK = CFUNCTYPE(None, X)
        dll = CDLL(_ctypes_test.__file__)
        func = dll._testfunc_cbk_large_struct
        func.argtypes = (X, CALLBACK)
        func.restype = None
        # the function just calls the callback with the passed structure
        func(s, CALLBACK(functools.partial(callback, check)))
        self.assertEqual(check.first, s.first)
        self.assertEqual(check.second, s.second)
        self.assertEqual(check.third, s.third)
        self.assertEqual(check.first, 0xdeadbeef)
        self.assertEqual(check.second, 0xcafebabe)
        self.assertEqual(check.third, 0x0bad1dea)
        # See issue #29565.
        # Ensure that the original struct is unchanged.
        self.assertEqual(s.first, check.first)
        self.assertEqual(s.second, check.second)
        self.assertEqual(s.third, check.third)

The changed test still passes, so it looks like ctypes makes a copy when passing from C to a Python callback, but not when passing from Python to C.
msg288114 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2017-02-19 06:47
PR submitted.
msg288170 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2017-02-20 00:16
New changeset a86339b83fbd0932e0529a3c91935e997a234582 by GitHub in branch 'master':
Fixed bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. (#168)
https://github.com/python/cpython/commit/a86339b83fbd0932e0529a3c91935e997a234582
msg288322 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-02-21 21:36
I approved the two backports. Thanks Vinay!
msg288336 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2017-02-22 06:19
New changeset 8fa7e22134ac9626b2188ff877f6aeecd3c9e618 by GitHub in branch '3.5':
Fixed bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. (#168) (#221)
https://github.com/python/cpython/commit/8fa7e22134ac9626b2188ff877f6aeecd3c9e618
msg288337 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2017-02-22 06:21
New changeset 3cc5817cfaf5663645f4ee447eaed603d2ad290a by GitHub in branch '3.6':
Fixed bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. (#168) (#220)
https://github.com/python/cpython/commit/3cc5817cfaf5663645f4ee447eaed603d2ad290a
msg289090 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-06 10:40
New test causes a compiler warning:

/home/serhiy/py/cpython/Modules/_ctypes/_ctypes_test.c: In function ‘_testfunc_large_struct_update_value’:
/home/serhiy/py/cpython/Modules/_ctypes/_ctypes_test.c:53:42: warning: parameter ‘in’ set but not used [-Wunused-but-set-parameter]
 _testfunc_large_struct_update_value(Test in)
                                          ^

Could it be silenced?
msg322963 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-08-02 14:47
New changeset 3243f8c1fb16b6de73f1d7a30f5d09047553bce3 by Victor Stinner in branch '2.7':
bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64 (GH-168) (GH-8625)
https://github.com/python/cpython/commit/3243f8c1fb16b6de73f1d7a30f5d09047553bce3
msg322969 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-08-02 15:25
Hum, the compilation succeeded on AppVeyor:
https://ci.appveyor.com/project/python/cpython/build/2.7build20258

But it fails on AMD64 Windows8 2.7:
https://buildbot.python.org/all/#/builders/74/builds/194

It seems like this buildbot uses Visual Studio 2008 ("VS 9.0")... but it seems like AppVeyor also uses Visual Studio 2008. No idea why AppVeyor missed the compilation error :-(

Anyway, I proposed PR 8626 to fix it.
msg322971 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-08-02 15:40
New changeset 6a6b2483479a1ad0ab82300452f0ce71fa90b2d7 by Victor Stinner in branch '2.7':
bpo-29565: Fix compilation for C89 (GH-8626)
https://github.com/python/cpython/commit/6a6b2483479a1ad0ab82300452f0ce71fa90b2d7
History
Date User Action Args
2022-04-11 14:58:43adminsetgithub: 73751
2018-08-02 15:40:23vstinnersetmessages: + msg322971
2018-08-02 15:25:31vstinnersetmessages: + msg322969
2018-08-02 15:20:21vstinnersetpull_requests: + pull_request8133
2018-08-02 14:47:31vstinnersetnosy: + vstinner
messages: + msg322963
2018-08-02 14:02:57vstinnersetpull_requests: + pull_request8130
2017-03-06 10:40:46serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg289090
2017-02-22 08:18:50vinay.sajipsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-02-22 06:21:19vinay.sajipsetmessages: + msg288337
2017-02-22 06:19:57vinay.sajipsetmessages: + msg288336
2017-02-21 21:36:54steve.dowersetmessages: + msg288322
2017-02-21 19:47:51vinay.sajipsetpull_requests: + pull_request185
2017-02-21 19:44:40vinay.sajipsetpull_requests: + pull_request184
2017-02-20 00:16:36vinay.sajipsetmessages: + msg288170
2017-02-19 06:47:43vinay.sajipsetmessages: + msg288114
stage: needs patch -> patch review
2017-02-19 06:44:53vinay.sajipsetpull_requests: + pull_request133
2017-02-19 06:09:08vinay.sajipsetmessages: + msg288111
2017-02-17 18:06:08eryksunsetmessages: + msg288031
2017-02-16 17:44:46steve.dowersetmessages: + msg287963
2017-02-16 17:13:10vinay.sajipsetmessages: + msg287959
2017-02-15 18:34:14eryksunsetpriority: normal -> high
versions: + Python 3.5, Python 3.6, Python 3.7
nosy: + eryksun

messages: + msg287878

stage: needs patch
2017-02-15 09:48:34Igor Kudrincreate