python-cffi / cffi

A Foreign Function Interface package for calling C libraries from Python.
https://cffi.readthedocs.io/en/latest/
Other
114 stars 41 forks source link

Support pragma pack #45

Open nanonyme opened 8 months ago

nanonyme commented 8 months ago

Hi, I have this problem that I am using CFFI in ABI mode and have a header using MSVC pragma pack convention.

Following is an example struct that results in different size with 64bit Python on Windows depending on whether pack is enabled or not. (36 bytes with packing, 40 bytes without packing)

import cffi

ffi = cffi.FFI()
ffi.cdef("""

typedef enum { FOO, BAR } Test;

typedef struct {
   size_t mysize;
   Test someenum;
   uint64_t x;
   uint64_t y;
   uint64_t z;
} Foo;""", packed=True)

assert ffi.sizeof("Foo") == 36

what CFFI does is it wipes the pragma pack declaration out completely. I may be able to workaround the issue as per above by using packed=True, however, CFFI functionality of silently generating invalid structs in ABI mode without any warning or error seems scary to me. Ideally it would be great if CFFI handled scoped packing correctly but it seems minimal scope would be at least erroring out if you have packing requested in cdef but packed==False.

nanonyme commented 8 months ago

pragma pack spec contained is described in https://learn.microsoft.com/en-us/cpp/preprocessor/pack?view=msvc-170

arigo commented 8 months ago

what CFFI does is it wipes the pragma pack declaration out completely.

I'm not sure I understand what you mean. CFFI did not "wipe out" any pragma pack declaration in your example, as there is none to start with. There is only the "packed" keyword argument you can give to cdef() or not, and doing so changes the packing of structures declared inside that cdef.

CFFI functionality of silently generating invalid structs in ABI mode without any warning or error seems scary to me.

That's what the API mode is for. There is nothing we can do in ABI mode to prevent cdefs from silently generating invalid structs if the code or the "packed" argument given to cdef() are bogus...

As a wild guess, maybe you used some third-party tool to generate the source code you pass to the cdef(), and that tool ignored the "#pragma"? In that case, the problem is in the third-party tool and not in CFFI.

nanonyme commented 8 months ago

I am referring to this commit as for the "wipe out" part https://github.com/python-cffi/cffi/commit/6da6407d3f714ac43a1d67cceb01d501eb3f5807. This code block was clearly being triggered by the real code which did have pragma pack, above is a synthetic example to generate a struct where packing actually matters since I don't at the moment have access to the original C API header.

nanonyme commented 8 months ago

Simply removing that "ignore pragmas" is not enough (nor did I really expect it to do so) to fix the generation of the struct but it would have shown that the C API header had code that CFFI cannot handle in ABI mode. (rather than silently generating incorrect data structure)

nanonyme commented 8 months ago

I can try to obtain the pragma pack declarations from original header for Friday. I thought I remembered the header well enough by heart to create a full-fledged example but had forgotten the exact pragma pack lines involved so I left them out.

arigo commented 8 months ago

You're right, CFFI should issue a warning instead of silently ignoring all #pragma... Even better, of course, if CFFI would recognize and parse the #pragma pack like MSVC does. At one point in time, the issue was that if you didn't install the latest version of pycparser then the pragma lines were ignored by pycparser itself. But that is already many years ago, so it should be fine now.

nanonyme commented 8 months ago

The warning sounds great. We're going to discuss in team whether or not we actually want to continue using pragma pack for this case in the future and we can probably use the parameter as a workaround for now if we do. (the only downside seems to be that pragma pack allows having packed and not packed structs in same header whereas parameter has full cdef scope; we can probably live with that) But it's important when things fail that there is proper context in logs.