search

eerimoq / bincopy

Mangling of various file formats that conveys binary information (Motorola S-Record, Intel HEX, TI-TXT, Verilog VMEM, ELF and binary files).
MIT License
109 stars 38 forks source link

Add optional `padding` parameter to `chunks` #41

Closed bessman closed 10 months ago

bessman commented 11 months ago

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

Close #40.

bessman commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

bessman commented 11 months ago

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

eerimoq commented 11 months ago

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

bessman commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

Will have a look tonight

Den ons 25 okt. 2023 11:49Alexander Bessman @.***> skrev:

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.

— Reply to this email directly, view it on GitHub https://github.com/eerimoq/bincopy/pull/41#issuecomment-1778903473, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABLFKWOLTVZ2TPYAWOC3ETYBDOBLAVCNFSM6AAAAAA5W2A4MSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZYHEYDGNBXGM . You are receiving this because you commented.Message ID: @.***>