Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support pragma pack #45

Open
nanonyme opened this issue Jan 10, 2024 · 7 comments
Open

Support pragma pack #45

nanonyme opened this issue Jan 10, 2024 · 7 comments

Comments

@nanonyme
Copy link

nanonyme commented Jan 10, 2024

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
Copy link
Author

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

@arigo
Copy link
Contributor

arigo commented Jan 10, 2024

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
Copy link
Author

I am referring to this commit as for the "wipe out" part 6da6407. 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
Copy link
Author

nanonyme commented Jan 10, 2024

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
Copy link
Author

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
Copy link
Contributor

arigo commented Jan 10, 2024

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
Copy link
Author

nanonyme commented Jan 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants