Skip to content

Add Structure method to determine field size#274

Open
shizmob wants to merge 2 commits intoerocarrera:masterfrom
shizmob:master
Open

Add Structure method to determine field size#274
shizmob wants to merge 2 commits intoerocarrera:masterfrom
shizmob:master

Conversation

@shizmob
Copy link
Copy Markdown

@shizmob shizmob commented Nov 8, 2019

This PR adds the method Structure.sizeof_field() to determine the size of a single field within the structure. I've found this useful to avoid hardcoding certain assumptions, especially with PE32 vs PE32+ in mind.

@nightlark nightlark mentioned this pull request Jan 2, 2026
@nightlark
Copy link
Copy Markdown
Collaborator

Overall this looks okay to me, it mostly depends on how we feel about adding an additional dictionary for keeping track of field sizes. I haven't really checked to see how much overhead it adds in terms of memory use (which perhaps we could start using __slots__ elsewhere as an optimization... might be worth adding some benchmarks to see how much of an impact a change like this would have).

@nightlark nightlark added this to the 2026 Release 1 milestone Mar 16, 2026
@nightlark
Copy link
Copy Markdown
Collaborator

I tried to resolve the merge conflicts using the web editor, and I know that in the web editor there were no merge conflict markers remaining -- and yet the merge commit made has those markers.

This will require some manual fixing to bring it up to date with the current pefile master branch:

  • __set_format__ is now called set_format and it is no longer a method tied to the Structure class. The changes from that method need to be moved to the new one, and the way the internal size field gets set must also be updated.
  • set_bitfields_format calls set_format; it needs to be updated to handle the added field sizes return argument that gets added to set_format. Then the bitfields structure class needs updating to handle the field sizes being returned by set_bitfields_format.

@nightlark
Copy link
Copy Markdown
Collaborator

I believe I got this updated to work with the latest master branch. It looks like it does change some function signatures (return values from set_format and set_bitfields_format), which based on their names are part of the public API, making this a breaking change. I'm not sure how frequently these functions are actually used by pefile users though.

I'm not finding other code on GitHub that calls set_format and references pefile, other than repositories that just have a copy of pefile.py in them. I'd like to get thoughts from a few other people on if they think this potential breaking change seems okay or not.

@nightlark nightlark requested review from erocarrera and j-t-1 March 28, 2026 19:03
Copy link
Copy Markdown
Collaborator

@j-t-1 j-t-1 left a comment

Choose a reason for hiding this comment

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

Seems okay.

This comment puts it well: #274 (comment). The checks took under ten minutes, so nothing stands out. A call graph may help, clarifying where the functions containing these classes are instantiated.

"""

def __init__(self, format, name=None, file_offset=None):
# Format is forced little endian, for big endian non-Intel platforms
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To match the output order of set_format, this can be reordered:

        self.__format_str__ = "<"
        self.__unpacked_data_elms__ = []
        self.__field_offsets__ = {}
        self.__field_sizes__ = {}
        self.__keys__ = []
        self.__format_length__ = 0

Outside of this PR we could put __format_length__ after __format_str__. Although it is a breaking change of set_format for small readability increase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants