Skip to content
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.

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

Add optional padding parameter to chunks #41

Merged
merged 10 commits into from
Oct 26, 2023
Merged

Conversation

bessman
Copy link
Contributor

@bessman bessman commented Oct 7, 2023

If padding is given, the first and final chunks are padded to conform to size and alignment, which is otherwise not guaranteed.

Close #40.

If `padding` is given, the first and final chunks are padded to
conform to `size` and `alignment`, which is otherwise not guaranteed.
@bessman
Copy link
Contributor Author

bessman commented Oct 8, 2023

In the first commit, I padded the final chunk's size to size. I realized this is not safe; The final chunk may be too large for the available memory.

In the most recent commit, I changed it so that the final chunk is padded so its size is a multiple of alignment. This should be safe; in situations that require alignment I think it's fair to assume that:

  1. The lowest writable address is aligned.
  2. The total writable area is a multiple of the alignment.

@bessman bessman marked this pull request as draft October 9, 2023 07:00
@bessman
Copy link
Contributor Author

bessman commented Oct 9, 2023

There is a potential bug here: If two nearly adjacent segments are separated by less than alignment words, the first chunk of the upper segment may overwrite the final chunk of the lower segment.

After thinking about this some more, I'm wondering if chunks is really the right place to handle alignment. I think alignment should actually be a property of the BinFile. Then all segments could be aligned during record parsing, and segments which are separated by less than alignment words would be considered a single segment.

What do you think?

@eerimoq
Copy link
Owner

eerimoq commented Oct 9, 2023

I think segments should only contain actual data. No padding. Just feels right. I'm not sure how to implement chunk padding in an intuitive way.

The user can fill gaps with padding prior to iterating over chunks. Not sure if that is flexible enough. Could be hard to know where the gaps between segments are small.

@bessman
Copy link
Contributor Author

bessman commented Oct 9, 2023

I added a check for chunk overlap and merge the chunks if needed.

@bessman bessman marked this pull request as ready for review October 9, 2023 20:39
@bessman bessman marked this pull request as draft October 10, 2023 16:44
@bessman
Copy link
Contributor Author

bessman commented Oct 10, 2023

Sorry, the chunk merging is wrong. This approach may be too fragile.

@eerimoq
Copy link
Owner

eerimoq commented Oct 13, 2023

Ok, just let me know if/when it is ready to merge. Close the PR if the feature should not be added.

@bessman bessman marked this pull request as ready for review October 13, 2023 08:18
@bessman
Copy link
Contributor Author

bessman commented Oct 13, 2023

I think this is ready now. But I thought so twice before and then thought of a case I hadn't considered. That tells me the complexity of this approach is a bit high, at least for my brain. I can't think of a different way to do it from chunks, though.

I think the feature itself (aligning first and final chunks) seems like it would be a good fit for bincopy, but I'll let you be the judge of that.

@bessman
Copy link
Contributor Author

bessman commented Oct 25, 2023

Could I bother you for a decision on this PR? Right now I have some workarounds for alignment of first and final chunks in my own project, and it would be nice to know whether or not I need to keep maintaining those workarounds.

FWIW, I haven't thought of any further edge cases where the approach in this PR would not work. It has had some time to percolate in my mind, and I now feel more confident about it than I did before.

@eerimoq
Copy link
Owner

eerimoq commented Oct 25, 2023 via email

@eerimoq eerimoq merged commit 2371374 into eerimoq:master Oct 26, 2023
5 checks passed
@bessman bessman deleted the chunkpad branch April 30, 2024 12:52
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.

First chunk is misaligned if segment has non-aligned start address
2 participants