Still broken ctypes calling convention on MSVC / 64-bit Windows (large structs)

@doko42

BPO	29565
Nosy	@doko42, @pfmoore, @vsajip, @mdickinson, @vstinner, @tiran, @tjguk, @meadori, @zware, @serhiy-storchaka, @eryksun, @zooba
PRs	Fixed bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. #168 Fixed bpo-29565: Corrected ctypes passing of large structs by value o… #220 Fixed bpo-29565: Corrected ctypes passing of large structs by value o… #221 [2.7] bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. (#168) #8625 [2.7] bpo-29565: Fix compilation for C89 #8626

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-02-22.08:18:50.237>
created_at = <Date 2017-02-15.09:48:34.253>
labels = ['ctypes', 'type-bug', '3.7', 'OS-windows']
title = 'Still broken ctypes calling convention on MSVC / 64-bit Windows (large structs)'
updated_at = <Date 2018-08-02.15:40:23.052>
user = 'https://bugs.python.org/IgorKudrin'

bugs.python.org fields:

activity = <Date 2018-08-02.15:40:23.052>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-02-22.08:18:50.237>
closer = 'vinay.sajip'
components = ['Windows', 'ctypes']
creation = <Date 2017-02-15.09:48:34.253>
creator = 'Igor Kudrin'
dependencies = []
files = []
hgrepos = []
issue_num = 29565
keywords = []
message_count = 15.0
messages = ['287831', '287878', '287959', '287963', '288031', '288111', '288114', '288170', '288322', '288336', '288337', '289090', '322963', '322969', '322971']
nosy_count = 13.0
nosy_names = ['doko', 'paul.moore', 'vinay.sajip', 'mark.dickinson', 'vstinner', 'christian.heimes', 'tim.golden', 'meador.inge', 'zach.ware', 'serhiy.storchaka', 'eryksun', 'steve.dower', 'Igor Kudrin']
pr_nums = ['168', '220', '221', '8625', '8626']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29565'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

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

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 bpo-22273.

Just trying to confirm my understanding - in both this issue and bpo-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 bpo-22273 seems to suggest so.

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.

This issue is more straight-forward than bpo-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;

When adding the fix for bpo-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 python/cpython#73751.
            # 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 python/cpython#73751.
        # 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.

PR submitted.

New changeset a86339b by GitHub in branch 'master':
Fixed bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. (#168)
a86339b

I approved the two backports. Thanks Vinay!

New changeset 8fa7e22 by GitHub in branch '3.5':
Fixed bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. (#168) (#221)
8fa7e22

New changeset 3cc5817 by GitHub in branch '3.6':
Fixed bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64. (#168) (#220)
3cc5817

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?

New changeset 3243f8c by Victor Stinner in branch '2.7':
bpo-29565: Corrected ctypes passing of large structs by value on Windows AMD64 (GH-168) (GH-8625)
3243f8c

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.

New changeset 6a6b248 by Victor Stinner in branch '2.7':
bpo-29565: Fix compilation for C89 (GH-8626)
6a6b248

Navigation Menu

Repository navigation

Still broken ctypes calling convention on MSVC / 64-bit Windows (large structs) #73751

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Still broken ctypes calling convention on MSVC / 64-bit Windows (large structs) #73751

Description

Activity

IgorKudrin commented on Feb 15, 2017

eryksun commented on Feb 15, 2017

vsajip commented on Feb 16, 2017

zooba commented on Feb 16, 2017

eryksun commented on Feb 17, 2017

vsajip commented on Feb 19, 2017

vsajip commented on Feb 19, 2017

vsajip commented on Feb 20, 2017

zooba commented on Feb 21, 2017

vsajip commented on Feb 22, 2017

vsajip commented on Feb 22, 2017

serhiy-storchaka commented on Mar 6, 2017

vstinner commented on Aug 2, 2018

vstinner commented on Aug 2, 2018

vstinner commented on Aug 2, 2018

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions