nodejs
src: zero-initialize data that are copied into the snapshot
This reverts one commit from https://github.com/nodejs/node/pull/50983 because I found a better & simpler approach to prevent the padding from affecting snapshot reproducibility (in the second commit).
Revert "src: make sure that memcpy-ed structs in snapshot have no padding"
This reverts commit 4e58cde589dfd980c8976b158853a331142e1e4b.
src: zero-initialize data that are copied into the snapshot
To prevent padding from making the snapshot unreproducible, zero-initialize the data that are copied into the snapshot so that the padding copied are all zeros. This is better than enlarging the enums to align the fields since it doesn't make the snapshot bigger than necessary, and it removes the need of using static assertions to ensure alignment.
Refs: https://github.com/nodejs/node/pull/50983
cc original reviewers of https://github.com/nodejs/node/pull/50983 @lemire @jasnell @legendecas
For some reason, the value-initialize trick is not working in dynamically linked builds. So I removed the update and got the branch back to the original shape.
For some reason, the value-initialize trick is not working in dynamically linked builds. So I removed the update and got the branch back to the original shape.
It should work as it is specified in the standard. The compiler basically calls memset for you, see https://godbolt.org/z/7hTW9norW
But the explicit memset is quite fine and, so we are clear, I did not request you change the code. :-/
I think that the code as it stands now is fine.
The tests are still broken, so it may be something that landed in the main branch after cd8e61fe2630906fc53de6ff9b98ef8355533785 that broke it (or could be the build infra?)
Actually I am seeing the test failing on other unrelated PRs, so it could be either some infra update, or 296ce1ba2ac7728c6e1b016964b8fe6ba55a6ba4 or 4c730aed7f825af1691740663d599e9de5958f89
Actually I am seeing the test failing on other unrelated PRs, so it could be either some infra update, or 296ce1b or 4c730ae
FWIW It started showing up in the reliability reports since yesterday: https://github.com/nodejs/reliability/issues/905
https://github.com/nodejs/node/commit/4c730aed7f825af1691740663d599e9de5958f89 may have exacerbated the issue, but not caused it. See https://github.com/nodejs/node/pull/53583#issuecomment-2189106131
Landed in bea91db2c14ef864434a4404f077857eb2174cbd...de766a8709aaeb9b506b278bfb67c78aa69ff69d