bpo-29753: fix merging packed bitfields in ctypes struct/union #19850
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
|
Btw, here is a demo showing the test values are correct to make the review quicker. https://repl.it/repls/MistyPassionateLaboratory If you want to run locally: #include <stdio.h>
#include <stdint.h>
struct test1 {
uint8_t a : 4;
uint8_t b : 4;
} __attribute__((packed));
struct test2 {
uint8_t a : 1;
uint16_t b : 1;
uint32_t c : 1;
uint64_t d : 1;
} __attribute__((packed));
struct test3 {
uint8_t a : 8;
uint16_t b : 1;
uint32_t c : 1;
uint64_t d : 1;
} __attribute__((packed));
struct test4 {
uint32_t a : 9;
uint16_t b : 10;
uint32_t c : 25;
uint64_t d : 1;
} __attribute__((packed));
struct test5 {
uint32_t a : 9;
uint16_t b : 10;
uint32_t c : 25;
uint64_t d : 5;
} __attribute__((packed));
struct test6 {
uint16_t a;
uint16_t b : 9;
uint16_t c : 1;
uint16_t d : 1;
uint16_t e : 1;
uint16_t f : 1;
uint16_t g : 3;
uint32_t h : 10;
uint32_t i : 20;
uint32_t j : 2;
} __attribute__((packed));
struct test7 {
uint16_t a : 9;
uint16_t b : 10;
uint16_t c;
uint8_t d : 8;
} __attribute__((packed));
struct test8 {
uint32_t a : 9;
uint32_t b;
uint32_t c : 8;
} __attribute__((packed));
int main(void) {
printf("test1 = %lu\n", sizeof(struct test1));
printf("test2 = %lu\n", sizeof(struct test2));
printf("test3 = %lu\n", sizeof(struct test3));
printf("test4 = %lu\n", sizeof(struct test4));
printf("test5 = %lu\n", sizeof(struct test5));
printf("test6 = %lu\n", sizeof(struct test6));
printf("test7 = %lu\n", sizeof(struct test7));
printf("test8 = %lu\n", sizeof(struct test8));
return 0;
} |
FFY00
force-pushed
the
bpo-29753
branch
8 times, most recently
from
35e1465
to
b38f853
Compare
FFY00
changed the title
FFY00
force-pushed
the
bpo-29753
branch
6 times, most recently
from
a083ecb
to
2cc6b5d
Compare
There was a problem hiding this comment.
This is a long standing bug and I would really like to see it fixed. I added some comments to help with reviews.
There were two major problems:
- the bitfields were not being expanded when they should, resulting in padding in the middle of a packed bitfield group, as shown in bpo-29753
- the bitfields were being created with the specified field size, instead of the minimum needed size, in other words, they were not packed
- this was not such a big of a deal as we could change the specified field size as a workaround, but still a very relevant issue
@pganssle given the lack of reviews could you please take a look if you have time?
| size += 1; | ||
| } else | ||
| #endif | ||
| size = dict->size; |
There was a problem hiding this comment.
Otherwise, just use the field size.
Modules/_ctypes/cfield.c
Outdated
| if (pack && bitsize) { /* packed bitfield */ | ||
| size = 1; | ||
| while(size * 8 < bitsize) | ||
| size += 1; |
There was a problem hiding this comment.
If we have packed bitfield, calculate the minimum number of bytes needed to fit the bitfield.
| @@ -87,25 +99,22 @@ PyCField_FromDesc(PyObject *desc, Py_ssize_t index, | |||
| } else if (bitsize /* this is a bitfield request */ | |||
| && *pfield_size /* we have a bitfield open */ | |||
| && dict->size * 8 >= *pfield_size | |||
| && (*pbitofs + bitsize) <= dict->size * 8) { | |||
| && (((*pbitofs + bitsize) <= dict->size * 8) || pack)) { | |||
There was a problem hiding this comment.
When we have a packed bitfield and it doesn't fit the current open bitfield, always expand it.
There was a problem hiding this comment.
I'm quite convinced this comment should go in the source code.
| /* expand bit field */ | ||
| fieldtype = EXPAND_BITFIELD; | ||
| #endif | ||
| } else if (bitsize) { | ||
| /* start new bitfield */ | ||
| fieldtype = NEW_BITFIELD; | ||
| *pbitofs = 0; | ||
| *pfield_size = dict->size * 8; | ||
| *pfield_size = size * 8; |
There was a problem hiding this comment.
Start a new bitfield with the size we calculated above. This means if it is a packed bitfield we start a new bitfield with only the needed size, not the specified field size.
Misc/NEWS.d/next/Library/2020-05-02-01-01-30.bpo-29753.n2M-AF.rst
Outdated
Show resolved
Hide resolved
|
I've pointed out a few minor things but I don't really understand the problem or ctypes well enough to give it a thorough review at the moment. Might be a few weeks before I have time to understand it well enough that I'd be comfortable merging, but in the scheme of things that's not so long, I suppose. |
|
@eryksun's comment in the bug explains fairly well what is happening wrong. Anyway, just this initial review is a big help :D |
|
|
||
| #ifndef MS_WIN32 | ||
| if (pack && bitsize) { /* packed bitfield */ | ||
| size = (bitsize - 1) / 8 + 1; |
There was a problem hiding this comment.
(copying the old comment, so that it shows up in the github editor for reviewers)
If we have a packed bitfield, calculate the minimum number of bytes needed to fit the bitfield.
There was a problem hiding this comment.
I'd follow this rule: if a comment is necessary/helpful/crucial for reviewers it should be in the source code so that everyone looking at this code at any point later on sees it, not in GitHub comments.
|
cc @meadori |
|
Okay, so it has become obvious there are no active maintainers with interest in this part of the code. Could someone else pick it up? The code had no tests, I fixed the issues and added tests. I also provided a link to a REPL with C showing that the tests I added are correct, which can be run directly from your browser. I think I have already pinged enough people, I am not sure what I need to do to get this merged. If needed, I could hop on a call with anyone to explain what was wrong and how it was fixed... Let me know what I should do, because at this point I have no clue. |
…tfields Signed-off-by: Filipe Laíns <lains@riseup.net>
|
BTW, some commits have descriptions associated, so if it would be possible to merge/rebase instead of squashing, it would be appreciated 😊 |
|
requested changes; please review again |
It's not possible. The project demands squash and merge. The only option for the future is to find the commit in which the change occurred, then extract the pull request number from the commit message and trace that back to this PR where the commits will remain visible. |
|
Eh, it's fine. We have sufficient documentation, it was just a nitpick. |
|
I have done that in the latest commit. |
|
Hey, which version of python does/will include this fix? |
|
3.10.0a6 and later. I figured this out by clicking on the commit hash above (0d7ad9f) and looking at the tags of that commit. All Python changes are committed to the main branch for the next 0.1 release unless explicitly backported to older versions, which this change was not. |
from ctypes import LittleEndianStructure, c_uint8, sizeof
class Point(LittleEndianStructure):
_pack_ = 1
_fields_ = [
("x", c_uint8, 4),
("y", c_uint8, 8),
("z", c_uint8, 4),
]
data = Point(2, 2, 2)
print(f"size of data: {sizeof(data)}")
print(f"x: {data.x}, y: {data.y}, z: {data.z}")I executed this script with version 3.10.0b3 on Linux, but got this output: However, with version 3.9.5, I got: It seems the size of packing bitfields in ctypes structure is now the same as what GCC does, but still unexpected behavor here. |
This reverts commit 0d7ad9f. See python#19850 (comment)
|
I can confirm this issue. I am very surprised that was not covered by tests. Unfortunately, I don't have much time right now to look into that, so I opened #27085 reverting it, meaning this fix won't hit 3.10. I will try to look into the issue and submit a new fix for 3.11, but we'll see. |
This reverts commit 0d7ad9f as it has a regression. See #19850 (comment)
This reverts commit 0d7ad9f as it has a regression. See https://github.com/python/cpython/pull/19850GH-issuecomment-869410686 (cherry picked from commit e14d5ae) Co-authored-by: Filipe Laíns <lains@archlinux.org>
This reverts commit 0d7ad9f as it has a regression. See https://github.com/python/cpython/pull/19850GH-issuecomment-869410686 (cherry picked from commit e14d5ae) Co-authored-by: Filipe Laíns <lains@archlinux.org>
bpo-29753: revert 0d7ad9f (pythonGH-19850) (pythonGH-27085)
|
From the commit message:
https://bugs.python.org/issue29753
Automerge-Triggered-By: GH:jaraco