FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-29753: fix merging packed bitfields in ctypes struct/union by FFY00 · Pull Request #19850 · python/cpython · GitHub

/ cpython Public

bpo-29753: fix merging packed bitfields in ctypes struct/union - #19850

Merged
miss-islington merged 5 commits into
python:masterfrom
FFY00:bpo-29753
Feb 28, 2021
ConversationCommits (5)ChecksFiles changed
Merged

bpo-29753: fix merging packed bitfields in ctypes struct/union#19850
miss-islington merged 5 commits into
python:masterfrom
FFY00:bpo-29753

Conversation

FFY00 commented May 2, 2020 •
edited by miss-islington
Loading

Copy link
Copy Markdown
Member

From the commit message:

When the structure is packed we should always expand when needed,
otherwise we will add some padding between the fields. This patch makes
sure we always merge bitfields together. It also changes the field merging
algorithm so that it handles bitfields correctly.

https://bugs.python.org/issue29753

Automerge-Triggered-By: GH:jaraco

the-knights-who-say-ni commented May 2, 2020

Copy link
Copy Markdown

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 Missing

Our records indicate the following people have not signed the CLA:

@FFY00

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
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

FFY00 commented May 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 May 2, 2020 02:48
FFY00 changed the title bpo-29753: fix populating packed bitfields in ctypes struct/union bpo-29753: fix merging packed bitfields in ctypes struct/union May 2, 2020
FFY00 force-pushed the bpo-29753 branch 6 times, most recently from a083ecb to 2cc6b5d Compare May 2, 2020 16:19
FFY00 commented May 27, 2020
View reviewed changes

FFY00 left a comment •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Comment thread Modules/_ctypes/cfield.c Outdated

FFY00 May 27, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Otherwise, just use the field size.

Comment thread Modules/_ctypes/cfield.c Outdated
Comment on lines 76 to 79

FFY00 May 27, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If we have packed bitfield, calculate the minimum number of bytes needed to fit the bitfield.

Comment thread Modules/_ctypes/cfield.c Outdated

FFY00 May 27, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

When we have a packed bitfield and it doesn't fit the current open bitfield, always expand it.

jstasiak Nov 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm quite convinced this comment should go in the source code.

Comment thread Modules/_ctypes/cfield.c Outdated

FFY00 May 27, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

jstasiak Nov 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ditto here.

Comment thread Modules/_ctypes/cfield.c Outdated

pganssle commented May 27, 2020

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

@eryksun's comment in the bug explains fairly well what is happening wrong. Anyway, just this initial review is a big help :D

FFY00 commented Jun 25, 2020
View reviewed changes
Comment thread Modules/_ctypes/cfield.c Outdated

FFY00 Jun 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(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.

jstasiak Nov 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

cc @meadori

FFY00 commented Sep 19, 2020

Copy link
Copy Markdown
Member Author

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.

bpo-29753: improve documentation PyCField_FromDesc relating packed bi…
9f015f5 
…tfields

Signed-off-by: Filipe Laíns <lains@riseup.net>

FFY00 commented Feb 28, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

requested changes; please review again

FFY00 requested a review from jaraco February 28, 2021 18:36

jaraco commented Feb 28, 2021

Copy link
Copy Markdown
Member

BTW, some commits have descriptions associated, so if it would be possible to merge/rebase instead of squashing, it would be appreciated 😊

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

Copy link
Copy Markdown
Member Author

Eh, it's fine. We have sufficient documentation, it was just a nitpick.

jaraco requested changes Feb 28, 2021
View reviewed changes

jaraco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

I have done that in the latest commit.

9f015f5 (#19850)

jaraco approved these changes Feb 28, 2021
View reviewed changes

jaraco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry I missed that. Sounds good.

miss-islington merged commit 0d7ad9f into python:master Feb 28, 2021

phantom1299 commented Apr 30, 2021

Copy link
Copy Markdown

Hey, which version of python does/will include this fix?

jaraco commented Apr 30, 2021

Copy link
Copy Markdown
Member

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.

Half9000 commented Jun 28, 2021 •
edited
Loading

Copy link
Copy Markdown 
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:

size of data: 2
x: 2, y: 0, z: 0

However, with version 3.9.5, I got:

size of data: 3
x: 2, y: 2, z: 2

It seems the size of packing bitfields in ctypes structure is now the same as what GCC does, but still unexpected behavor here.

FFY00 added a commit to FFY00/cpython that referenced this pull request Jul 11, 2021
bpo-29753: revert 0d7ad9f (pythonGH-19850)
c619b80 
This reverts commit 0d7ad9f.

See python#19850 (comment)
FFY00 mentioned this pull request Jul 11, 2021
Merged

FFY00 commented Jul 11, 2021

Copy link
Copy Markdown
Member Author

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.

pablogsal pushed a commit that referenced this pull request Jul 11, 2021
bpo-29753: revert 0d7ad9f (GH-19850) (GH-27085)
e14d5ae 
This reverts commit 0d7ad9f as it has a regression.

See #19850 (comment)
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 11, 2021
bpo-29753: revert 0d7ad9f (pythonGH-19850) (pythonGH-27085)
9ce53ae 
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>
miss-islington added a commit that referenced this pull request Jul 11, 2021
bpo-29753: revert 0d7ad9f (GH-19850) (GH-27085)
42da46e 
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>
sthagen mentioned this pull request Jul 11, 2021
Merged
sthagen added a commit to sthagen/python-cpython that referenced this pull request Jul 11, 2021
Merge pull request #515 from python/main
18ede45 
bpo-29753: revert 0d7ad9f (pythonGH-19850) (pythonGH-27085)

bedevere-bot commented Jul 11, 2021 •
edited
Loading

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Arch Linux Asan Debug 3.10 has failed when building commit 42da46e.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/621/builds/142) and take a look at the build logs.
  4. Check if the failure is related to this commit (42da46e) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/621/builds/142

Summary of the results of the build (if available):

Click to see traceback logs 
remote: Enumerating objects: 8, done.        
remote: Counting objects:  12% (1/8)        
remote: Counting objects:  25% (2/8)        
remote: Counting objects:  37% (3/8)        
remote: Counting objects:  50% (4/8)        
remote: Counting objects:  62% (5/8)        
remote: Counting objects:  75% (6/8)        
remote: Counting objects:  87% (7/8)        
remote: Counting objects: 100% (8/8)        
remote: Counting objects: 100% (8/8), done.        
remote: Compressing objects:  50% (1/2)        
remote: Compressing objects: 100% (2/2)        
remote: Compressing objects: 100% (2/2), done.        
remote: Total 8 (delta 6), reused 6 (delta 6), pack-reused 0        
From https://github.com/python/cpython
 * branch                  3.10       -> FETCH_HEAD
Note: switching to '42da46ed522157b057d73e6b623615ef6427999e'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 42da46ed52 [bpo-29753](https://bugs.python.org/issue29753): revert 0d7ad9f (GH-19850) (GH-27085)
Switched to and reset branch '3.10'

renaming build/scripts-3.10/pydoc3 to build/scripts-3.10/pydoc3.10
renaming build/scripts-3.10/idle3 to build/scripts-3.10/idle3.10
renaming build/scripts-3.10/2to3 to build/scripts-3.10/2to3-3.10

renaming build/scripts-3.10/pydoc3 to build/scripts-3.10/pydoc3.10
renaming build/scripts-3.10/idle3 to build/scripts-3.10/idle3.10
renaming build/scripts-3.10/2to3 to build/scripts-3.10/2to3-3.10

renaming build/scripts-3.10/pydoc3 to build/scripts-3.10/pydoc3.10
renaming build/scripts-3.10/idle3 to build/scripts-3.10/idle3.10
renaming build/scripts-3.10/2to3 to build/scripts-3.10/2to3-3.10
test_winconsoleio skipped -- test only relevant on win32
test_badargs (__main__.GeneralTest) ... ok
test_bound_methods (__main__.GeneralTest) ... ok
test_clear (__main__.GeneralTest) ... ok
test_exit (__main__.GeneralTest) ... ok
test_order (__main__.GeneralTest) ... ok
test_raise (__main__.GeneralTest) ... ok
test_raise_unnormalized (__main__.GeneralTest) ... ok
test_stress (__main__.GeneralTest) ... ok
test_unregister (__main__.GeneralTest) ... ok

----------------------------------------------------------------------
Ran 9 tests in 0.003s

OK
test_winreg skipped -- No module named 'winreg'
test_ttk_guionly skipped -- Tk unavailable due to TclError: couldn't connect to display ":99"
test_tk skipped -- Tk unavailable due to TclError: couldn't connect to display ":99"
test_ossaudiodev skipped -- [Errno 2] No such file or directory: '/dev/dsp'
test_winsound skipped -- No module named 'winsound'
test_devpoll skipped -- test works only on Solaris OS family
test_msilib skipped -- No module named '_msi'
test_zipfile64 skipped -- test requires loads of disk-space bytes and a long time to run
test_tix skipped -- Tk unavailable due to TclError: couldn't connect to display ":99"
test_ioctl skipped -- Unable to open /dev/tty
test_kqueue skipped -- test works only on BSD
test_startfile skipped -- object <module 'os' from '/buildbot/buildarea/3.10.pablogsal-arch-x86_64.asan_debug/build/Lib/os.py'> has no attribute 'startfile'
test_flock (__main__.FNTLEINTRTest) ... ok
test_lockf (__main__.FNTLEINTRTest) ... ok
test_read (__main__.OSEINTRTest) ... ok
test_wait (__main__.OSEINTRTest) ... ok
test_wait3 (__main__.OSEINTRTest) ... ok
test_wait4 (__main__.OSEINTRTest) ... ok
test_waitpid (__main__.OSEINTRTest) ... ok
test_write (__main__.OSEINTRTest) ... ok
test_devpoll (__main__.SelectEINTRTest) ... skipped 'need select.devpoll'
test_epoll (__main__.SelectEINTRTest) ... ok
test_kqueue (__main__.SelectEINTRTest) ... skipped 'need select.kqueue'
test_poll (__main__.SelectEINTRTest) ... ok
test_select (__main__.SelectEINTRTest) ... ok
test_sigtimedwait (__main__.SignalEINTRTest) ... ok
test_sigwaitinfo (__main__.SignalEINTRTest) ... ok
test_accept (__main__.SocketEINTRTest) ... ok
test_open (__main__.SocketEINTRTest) ... ok
test_os_open (__main__.SocketEINTRTest) ... ok
test_recv (__main__.SocketEINTRTest) ... ok
test_recvmsg (__main__.SocketEINTRTest) ... ok
test_send (__main__.SocketEINTRTest) ... ok
test_sendall (__main__.SocketEINTRTest) ... ok
test_sendmsg (__main__.SocketEINTRTest) ... ok
test_sleep (__main__.TimeEINTRTest) ... ok

----------------------------------------------------------------------
Ran 24 tests in 7.356s

OK (skipped=2)
make: *** [Makefile:1256: buildbottest] Terminated

Cannot open file '/buildbot/buildarea/3.10.pablogsal-arch-x86_64.asan_debug/build/test-results.xml' for upload

pablogsal mentioned this pull request Jul 18, 2021
Closed
matthiasgoergens mentioned this pull request Oct 9, 2022
Merged
CharlesMachalow mannequin mentioned this pull request Oct 9, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jaraco jaraco approved these changes
pganssle Awaiting requested review from pganssle
+2 more reviewers
ZackerySpytz ZackerySpytz left review comments
jstasiak jstasiak requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL