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

src: zero-initialize data that are copied into the snapshot #53563

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jun 23, 2024

This reverts one commit from #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 4e58cde.

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: #50983

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.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 23, 2024
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 23, 2024
@joyeecheung
Copy link
Member Author

cc original reviewers of #50983 @lemire @jasnell @legendecas

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 23, 2024
@nodejs-github-bot

This comment was marked as outdated.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@joyeecheung
Copy link
Member Author

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.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2024
@nodejs-github-bot
Copy link
Collaborator

@lemire
Copy link
Member

lemire commented Jun 24, 2024

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.

@joyeecheung
Copy link
Member Author

joyeecheung commented Jun 25, 2024

The tests are still broken, so it may be something that landed in the main branch after cd8e61f that broke it (or could be the build infra?)

@joyeecheung
Copy link
Member Author

joyeecheung commented Jun 25, 2024

Actually I am seeing the test failing on other unrelated PRs, so it could be either some infra update, or 296ce1b or 4c730ae

@richardlau
Copy link
Member

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: nodejs/reliability#905

@targos
Copy link
Member

targos commented Jun 25, 2024

4c730ae may have exacerbated the issue, but not caused it. See #53583 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants