bpo-29753: fix merging packed bitfields in ctypes struct/union#19850
Uh oh!
There was an error while loading. Please reload this page.
Conversation
the-knights-who-say-ni commented May 2, 2020
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>structtest1{uint8_ta : 4; uint8_tb : 4} __attribute__((packed)); structtest2{uint8_ta : 1; uint16_tb : 1; uint32_tc : 1; uint64_td : 1} __attribute__((packed)); structtest3{uint8_ta : 8; uint16_tb : 1; uint32_tc : 1; uint64_td : 1} __attribute__((packed)); structtest4{uint32_ta : 9; uint16_tb : 10; uint32_tc : 25; uint64_td : 1} __attribute__((packed)); structtest5{uint32_ta : 9; uint16_tb : 10; uint32_tc : 25; uint64_td : 5} __attribute__((packed)); structtest6{uint16_ta; uint16_tb : 9; uint16_tc : 1; uint16_td : 1; uint16_te : 1; uint16_tf : 1; uint16_tg : 3; uint32_th : 10; uint32_ti : 20; uint32_tj : 2} __attribute__((packed)); structtest7{uint16_ta : 9; uint16_tb : 10; uint16_tc; uint8_td : 8} __attribute__((packed)); structtest8{uint32_ta : 9; uint32_tb; uint32_tc : 8} __attribute__((packed)); intmain(void){printf("test1 = %lu\n", sizeof(structtest1)); printf("test2 = %lu\n", sizeof(structtest2)); printf("test3 = %lu\n", sizeof(structtest3)); printf("test4 = %lu\n", sizeof(structtest4)); printf("test5 = %lu\n", sizeof(structtest5)); printf("test6 = %lu\n", sizeof(structtest6)); printf("test7 = %lu\n", sizeof(structtest7)); printf("test8 = %lu\n", sizeof(structtest8)); return0} |
FFY00force-pushed the bpo-29753 branch 8 times, most recently from 35e1465 to b38f853CompareFFY00 changed the title FFY00force-pushed the bpo-29753 branch 6 times, most recently from a083ecb to 2cc6b5dCompare
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?
Modules/_ctypes/cfield.c Outdated
There was a problem hiding this comment.
Otherwise, just use the field size.
Modules/_ctypes/cfield.c Outdated
There was a problem hiding this comment.
If we have packed bitfield, calculate the minimum number of bytes needed to fit the bitfield.
Modules/_ctypes/cfield.c Outdated
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.
Modules/_ctypes/cfield.c Outdated
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.
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Library/2020-05-02-01-01-30.bpo-29753.n2M-AF.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
pganssle commented May 27, 2020
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. |
FFY00 commented May 27, 2020
@eryksun's comment in the bug explains fairly well what is happening wrong. Anyway, just this initial review is a big help :D |
Modules/_ctypes/cfield.c Outdated
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.
pitrou commented Jun 29, 2020
cc @meadori |
FFY00 commented Sep 19, 2020
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 😊 |
FFY00 commented Feb 28, 2021
requested changes; please review again |
jaraco commented Feb 28, 2021
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. |
FFY00 commented Feb 28, 2021
Eh, it's fine. We have sufficient documentation, it was just a nitpick. |
jaraco left a comment
jaraco left a comment There was a problem hiding this comment.
Looking good. I'd still like to see the comments from jstasiak resolved (by updating the code with the comments that are currently only in this review). Add those and we're good to go.
FFY00 commented Feb 28, 2021
I have done that in the latest commit. |
jaraco left a comment
jaraco left a comment There was a problem hiding this comment.
Sorry I missed that. Sounds good.
phantom1299 commented Apr 30, 2021
Hey, which version of python does/will include this fix? |
jaraco commented Apr 30, 2021
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. |
fromctypesimportLittleEndianStructure, c_uint8, sizeofclassPoint(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)
FFY00 commented Jul 11, 2021
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)
|
13 participants
From the commit message:
https://bugs.python.org/issue29753
Automerge-Triggered-By: GH:jaraco