Skip to content

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

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.

Sign up for GitHub

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

Merged
miss-islington merged 5 commits into from
Feb 28, 2021
+91 −8
Merged

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b159a0
bpo-29753: fix merging packed bitfields in ctypes struct/union
FFY00 May 1, 2020
80ee351
bpo-29753: shrink packed bitfields on creation
FFY00 May 2, 2020
192e4b9
bpo-29753: add tests for packed bitfields
FFY00 May 2, 2020
de3df46
bpo-29753: calculate bitfield size in a single arithmetic expression
FFY00 May 27, 2020
9f015f5
bpo-29753: improve documentation PyCField_FromDesc relating packed bi…
FFY00 Feb 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
bpo-29753: improve documentation PyCField_FromDesc relating packed bi… 
…tfields

Signed-off-by: Filipe Laíns <lains@riseup.net>
  • Loading branch information
@FFY00
FFY00 committed Feb 28, 2021
commit 9f015f598feadc47020034c063cbfc180f4e8907
11 changes: 10 additions & 1 deletion Modules/_ctypes/cfield.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ PyCField_FromDesc(PyObject *desc, Py_ssize_t index,
}

#ifndef MS_WIN32
if (pack && bitsize) { /* packed bitfield */
/* if we have a packed bitfield, calculate the minimum number of bytes we
need to fit it. otherwise use the specified size. */
if (pack && bitsize) {
size = (bitsize - 1) / 8 + 1;
Copy link
Copy Markdown
Member Author

@FFY00 FFY00 Jun 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@jstasiak jstasiak Nov 20, 2020

Choose a reason for hiding this comment

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

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.

} else
#endif
Expand All @@ -97,6 +99,8 @@ 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
/* if this is a packed bitfield, always expand it.
otherwise calculate if we need to expand it. */
&& (((*pbitofs + bitsize) <= dict->size * 8) || pack)) {
Copy link
Copy Markdown
Member Author

@FFY00 FFY00 May 27, 2020

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@jstasiak jstasiak Nov 20, 2020

Choose a reason for hiding this comment

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

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

/* expand bit field */
fieldtype = EXPAND_BITFIELD;
Expand All @@ -105,6 +109,8 @@ PyCField_FromDesc(PyObject *desc, Py_ssize_t index,
/* start new bitfield */
fieldtype = NEW_BITFIELD;
*pbitofs = 0;
/* use our calculated size (size) instead of type size (dict->size),
which can be different for packed bitfields */
*pfield_size = size * 8;
Copy link
Copy Markdown
Member Author

@FFY00 FFY00 May 27, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@jstasiak jstasiak Nov 20, 2020

Choose a reason for hiding this comment

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

Ditto here.

} else {
/* not a bit field */
Expand Down Expand Up @@ -177,6 +183,9 @@ PyCField_FromDesc(PyObject *desc, Py_ssize_t index,
break;

case EXPAND_BITFIELD:
/* increase the size if it is a packed bitfield.
EXPAND_BITFIELD should not be selected for non-packed fields if the
current size isn't already enough. */
if (pack)
Comment thread
FFY00 marked this conversation as resolved.
Outdated
size = (*pbitofs + bitsize - 1) / 8 + 1;

Expand Down