factor icon indicating copy to clipboard operation
factor copied to clipboard

VM prepare image reserved fields usage

Open nomennescio opened this issue 3 years ago • 5 comments

The version 4 image format did not write 0 into reserved_* fields (#2580). Therefore current version 4 images are not forward compatible with image versions that use these fields. By introducing an escape code 0 into the data_size field, three reserved_* fields now become available for redefinition, while keeping image version at 4.

The image.cpp changes can transparently read regular v4 image files, as well as ones with h.data_size=0 and h.escaped_data_size=data_size.

nomennescio avatar Feb 10 '22 17:02 nomennescio

I’m a little unsure about this by itself.

Why not see what changes are needed by compressed images and maybe increment to version 5 instead of this indirection?

On Feb 10, 2022, at 9:37 AM, nomennescio @.***> wrote:

 The version 4 image format did not write 0 into reserved_* fields (#2580). Therefore current version 4 images are not forward compatible with image versions that use these fields. By introducing an escape code 0 into the data_size field, three reserved_* fields now become available for redefinition, while keeping image version at 4.

The image.cpp changes can transparently read regular v4 image files, as well as ones with h.data_size=0 and h.escaped_data_size=data_size.

You can view, comment on, or merge this pull request online at:

https://github.com/factor/factor/pull/2591

Commit Summary

97a525a VM prepare image reserved fields usage File Changes (2 files) M vm/image.cpp (11) M vm/image.hpp (6) Patch Links:

https://github.com/factor/factor/pull/2591.patch https://github.com/factor/factor/pull/2591.diff — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you are subscribed to this thread.

mrjbq7 avatar Feb 10 '22 17:02 mrjbq7

I can imagine your viewpoint. Version 5 version bump might be most appropriate, this is more like a premature contract while keeping all options open and staying at version 4, while only explicitly declaring how fields are to be interpreted which is compatible with current ways. I was hoping for a kind of baseline to build upon, but we can also postpone such a decision.

nomennescio avatar Feb 10 '22 21:02 nomennescio

Made the code change less intrusive, with the same result.

nomennescio avatar Feb 10 '22 21:02 nomennescio

Would you like this merged while you are also considering what other changes are needed?

mrjbq7 avatar Feb 10 '22 21:02 mrjbq7

Would you like this merged while you are also considering what other changes are needed?

Yes please. It doesn't break existing code, and we can still change it later, as we nowhere explicitly save into this format. As a next step for my compression proposal on my other experimental branch, I intend to use reserved_2 and reserved_3 for compressed_data_size and compressed_code_size, which requires some non-intrusive image header change like this.

I can also just put everything in an experimental branch, but I like to keep feature/experimental branches small.

You can decide.

nomennescio avatar Feb 10 '22 22:02 nomennescio

Note that in a valid version 4 image file, actual data_size can never be 0 due to an assert in raw_fread. We will use this invariant as an escape value.

nomennescio avatar Aug 27 '23 14:08 nomennescio