bpo-30353 Fix pass by value for structs on 64-bit Cygwin/MinGW #1559
Conversation
…libffi has a similar bug; fixes https://bugs.python.org/issue29804
|
Per discussion on bpo-30353, I've updated this to apply the same workaround for builds on arm64, which has a similar bug in libffi (bpo-29804). |
Codecov Report
@@ Coverage Diff @@
## master #1559 +/- ##
==========================================
- Coverage 83.7% 82.67% -1.03%
==========================================
Files 1371 1432 +61
Lines 346545 353139 +6594
==========================================
+ Hits 290065 291964 +1899
- Misses 56480 61175 +4695
Continue to review full report at Codecov.
|
Modules/_ctypes/callproc.c
Outdated
| #if (defined(__x86_64__) && (defined(__MINGW64__) || defined(__CYGWIN__))) || \ | ||
| defined(__aarch64__) | ||
| void *tmp; | ||
| if (pa->ffi_type->type == FFI_TYPE_STRUCT) { |
There was a problem hiding this comment.
Is this definitely the correct test? Similar code to fix bpo-29565 uses a test of the type's size being greater than 8 bytes. Have you tested with structs both under and over this size limit?
There was a problem hiding this comment.
Oh, I didn't see that issue--interesting. I might be able to adapt the existing fix there to this issue as well.
As to whether or not it impacts just structs or any value > 8 bytes I'm not sure off the top of my head anymore. I'll have to double-check.
There was a problem hiding this comment.
@vsajip Okay, I think you're right here--the issue is basically the same as the old libffi_msvc code that's backported into ctypes--my changes actually cause a segfault if a small (<=8 bytes) struct is passed by value, so I should add a test for that.
There was a problem hiding this comment.
Oops, I should say, it doesn't cause a segfault (that was just a minor typo on my part). But regardless the fix is not needed for smaller structs which are, as expected, passed by value in registers.
There was a problem hiding this comment.
I should correct myself again--actually it is needed for smaller structs if they are not register-aligned.
Modules/_ctypes/callproc.c
Outdated
| } | ||
| #if (defined(__x86_64__) && (defined(__MINGW64__) || defined(__CYGWIN__))) || \ | ||
| defined(__aarch64__) | ||
| void *tmp; |
There was a problem hiding this comment.
Any reason this declaration can't be inside the if block? It's only ever used in that block.
There was a problem hiding this comment.
I'm not sure what you mean. It is inside the if block.
There was a problem hiding this comment.
By my reading, it's inside the #if block, but not inside the if (pa->ffi_type->type == FFI_TYPE_STRUCT) { block.
There was a problem hiding this comment.
You you meant the actual if statement. I don't mind moving it, but I don't see what difference it makes--it generates the exact same assembly either way.
There was a problem hiding this comment.
Just a little tidier. The reference (tmp) stays in the scope in which it's used. In fact, you can just do void * tmp =alloca(pa->ffi_type->size); and don't need a separate line just for the declaration.
There was a problem hiding this comment.
I don't really agree--I find variable declarations in an if statement pretty ugly, but I can't disagree it at least limits it scope (even if it's ultimately irrelevant). Maybe I'll change it if/when there are other more substantive changes to make.
There was a problem hiding this comment.
Sorry, I think my last comment came off as disrespectful--to be clear I have no problem with changing this later (I just meant I won't bother making a separate commit for it).
There was a problem hiding this comment.
No offence taken 😄
I find variable declarations in an if statement pretty ugly
As far as I know, it's considered good practice for declarations to be limited to the scope of the blocks where they're used, as long as the version of C in use permits it. There's nothing special about an if block here - one would do the same for for, while, do etc.
I won't bother making a separate commit for it
I wouldn't worry too much about that; commits are cheap and would normally be squashed on merge, anyway.
In fact, *any* struct (or other type such as long double) that does not fit in a 1/2/4/8 byte register should be passed by reference. Structs that do fit in a register are passed by value and we need to make sure this works properly too (it previously did not).
|
Okay, I reworked this a bit. I realized while figuring out that was going on that smaller structs that do fit in a register were actually not working properly (and I added a test for this). This is because libffi is currently pretty dumb about how it treats arguments. It only looks at the argument size, and if it's not 1, 2, 4, or 8 bytes it assumes the argument is a pointer and it puts that pointer on its stack. However, for structs of size 1/2/4/8 bytes the existing behavior (before my earlier fix) was correct because it libffi is passed the pointer to the struct value, and it treats this (e.g. for an 8 byte struct) as a uint64 pointer which it dereferences and copies the value. |
|
Looks like the Windows build on appveyor died for seemingly unrelated reasons. |
|
Thanks! |
|
@embray would you be able to create a PR for the 3.6 branch? |
|
There's no need, the way to do it is to cherry-pick that commit to the 3.6 branch - I'll look at it soon. |
…onGH-1559) (cherry picked from commit 9ba3aa4) Co-authored-by: Erik Bray <erik.m.bray@gmail.com>
|
GH-5954 is a backport of this pull request to the 3.6 branch. |
Fixes bpo-30353 by making additional stack allocations for structs and copying the struct data (as libffi would do otherwise).