{"action":"created","issue":{"url":"https://api.github.com/repos/ocaml/ocaml/issues/1683","repository_url":"https://api.github.com/repos/ocaml/ocaml","labels_url":"https://api.github.com/repos/ocaml/ocaml/issues/1683/labels{/name}","comments_url":"https://api.github.com/repos/ocaml/ocaml/issues/1683/comments","events_url":"https://api.github.com/repos/ocaml/ocaml/issues/1683/events","html_url":"https://github.com/ocaml/ocaml/pull/1683","id":309744766,"node_id":"MDExOlB1bGxSZXF1ZXN0MTc4MzIxMDc4","number":1683,"title":"Make marshalled Custom_tag objects store their length.","user":{"login":"stedolan","id":79765,"node_id":"MDQ6VXNlcjc5NzY1","avatar_url":"https://avatars3.githubusercontent.com/u/79765?v=4","gravatar_id":"","url":"https://api.github.com/users/stedolan","html_url":"https://github.com/stedolan","followers_url":"https://api.github.com/users/stedolan/followers","following_url":"https://api.github.com/users/stedolan/following{/other_user}","gists_url":"https://api.github.com/users/stedolan/gists{/gist_id}","starred_url":"https://api.github.com/users/stedolan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stedolan/subscriptions","organizations_url":"https://api.github.com/users/stedolan/orgs","repos_url":"https://api.github.com/users/stedolan/repos","events_url":"https://api.github.com/users/stedolan/events{/privacy}","received_events_url":"https://api.github.com/users/stedolan/received_events","type":"User","site_admin":false},"labels":[{"id":851463272,"node_id":"MDU6TGFiZWw4NTE0NjMyNzI=","url":"https://api.github.com/repos/ocaml/ocaml/labels/high-priority","name":"high-priority","color":"fbca04","default":false},{"id":470821684,"node_id":"MDU6TGFiZWw0NzA4MjE2ODQ=","url":"https://api.github.com/repos/ocaml/ocaml/labels/multicore-prerequisite","name":"multicore-prerequisite","color":"0052cc","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":10,"created_at":"2018-03-29T12:59:32Z","updated_at":"2018-07-10T11:52:09Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/ocaml/ocaml/pulls/1683","html_url":"https://github.com/ocaml/ocaml/pull/1683","diff_url":"https://github.com/ocaml/ocaml/pull/1683.diff","patch_url":"https://github.com/ocaml/ocaml/pull/1683.patch"},"body":"The format used by the `Marshal` module does not currently store the length of `Custom_tag` objects, making correct unmarshalling essentially impossible on the multicore branch. This patch adds the length to the marshal format.\r\n\r\nThe change is backwards- but not forwards-compatible: old marshalled files continue to parse, but new marshalled files containing custom objects will raise an exception when unmarshalled with old OCaml.\r\n\r\n## Background\r\n\r\nIn the `struct custom_operations`, custom objects provide the following two callbacks:\r\n```\r\n  void (*serialize)(value v,\r\n                    /*out*/ uintnat * bsize_32 /*size in bytes*/,\r\n                    /*out*/ uintnat * bsize_64 /*size in bytes*/);\r\n  uintnat (*deserialize)(void * dst);\r\n```\r\n\r\nTo marshal a custom object, `serialize` writes some bytes using e.g. `caml_serialize_int_4` (and the other functions from `intext.h`), and returns two values in the `bsize_32` and `bsize_64` pointers: the number of bytes of memory that will be needed to store the unmarshalled value on 32- and 64-bit platforms respectively.\r\n\r\nTo unmarshal a custom object, `deserialize` takes a pointer to a piece of memory that is large enough to contain the unmarshalled object (that is, at least `bsize_32` or `bsize_64` bytes long, depending on word size), and fills it with the unmarshalled value using e.g. `caml_deserialize_int_4` and friends. `deserialize` returns the number of bytes of `dst` that it has filled in, which should be equal to the value of `bsize_32` or `bsize_64` (again, depending on word size) that was returned by `serialize`.\r\n\r\n## Current marshal format\r\n\r\nHowever, the marshal format *does not store* `bsize_32` and `bsize_64`, so how does the unmarshaller know how large a buffer to pass to `deserialize`?\r\n\r\nThe marshal format's header stores the total number of bytes necessary to unmarshal the entire file. The unmarshaller allocates space for the entire data structure in a single contiguous block, and fills it in sequentially. So, the buffer passed to `deserialize` is the entire rest of this block, large enough not only for the custom object but for all subsequent values as well. The unmarshaller has no way of knowing in advance how many bytes of this block `deserialize` will consume, which is why `deserialize` must return this count so that the unmarshaller can advance its pointer.\r\n\r\nThis is heavily tied to the current heap layout and memory allocator: it relies on being able to store arbitrarily many objects in a single contiguous block. Currently, the multicore branch does not use this object layout, and needs to know how many bytes to allocate for each custom block encountered during unmarshalling, making this trick unworkable.\r\n\r\n## Change to the format\r\n\r\nThis patch adds a new code `CODE_CUSTOM_LEN` to the marshal format, which differs from the existing `CODE_CUSTOM` by including the 32-bit and 64-bit size fields. The marshaller (`extern.c`) now only produces `CODE_CUSTOM_LEN`, but the unmarshaller still understands `CODE_CUSTOM`. This means that old marshalled files continue to work, but new ones will not work with old programs.\r\n\r\nThis makes marshalled custom objects 12 bytes longer (4 byte `bsize_32` + 8 byte `bsize_64`). More complicated and compact schemes are possible, but this is simplest.\r\n\r\nThis means that the unmarshaller now knows how many bytes of memory a custom deserialiser requires before calling the deserialiser, which is a requirement for the multicore branch. That branch will not support unmarshalling `CODE_CUSTOM`, so I'd like to get patch merged soon to make transition easier.\r\n\r\nCurrently, the only use that the unmarshaller makes of the new length fields is error-checking: with this patch, length inconsistencies between custom `serialize` and `deserialize` functions will be detected. There is now a test in `tests/lib-marshal` testing this.\r\n"},"comment":{"url":"https://api.github.com/repos/ocaml/ocaml/issues/comments/403795521","html_url":"https://github.com/ocaml/ocaml/pull/1683#issuecomment-403795521","issue_url":"https://api.github.com/repos/ocaml/ocaml/issues/1683","id":403795521,"node_id":"MDEyOklzc3VlQ29tbWVudDQwMzc5NTUyMQ==","user":{"login":"stedolan","id":79765,"node_id":"MDQ6VXNlcjc5NzY1","avatar_url":"https://avatars3.githubusercontent.com/u/79765?v=4","gravatar_id":"","url":"https://api.github.com/users/stedolan","html_url":"https://github.com/stedolan","followers_url":"https://api.github.com/users/stedolan/followers","following_url":"https://api.github.com/users/stedolan/following{/other_user}","gists_url":"https://api.github.com/users/stedolan/gists{/gist_id}","starred_url":"https://api.github.com/users/stedolan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stedolan/subscriptions","organizations_url":"https://api.github.com/users/stedolan/orgs","repos_url":"https://api.github.com/users/stedolan/repos","events_url":"https://api.github.com/users/stedolan/events{/privacy}","received_events_url":"https://api.github.com/users/stedolan/received_events","type":"User","site_admin":false},"created_at":"2018-07-10T11:52:09Z","updated_at":"2018-07-10T11:52:09Z","author_association":"CONTRIBUTOR","body":"Fixed line length to keep `check-typo` happy."}}
{"actor":{"display_login":"stedolan"}}