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.

Sign up for GitHub

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

Jump to bottom

Replace mem::zeroed with mem::MaybeUninit::uninit for large struct in Unix #136826

Merged
merged 4 commits into from
Feb 23, 2025
Merged

Replace mem::zeroed with mem::MaybeUninit::uninit for large struct in Unix #136826

merged 4 commits into from
Feb 23, 2025

Conversation

xizheyin
Copy link
Contributor

As discussion in #136737.

  • Replace mem::zeroed() with MaybeUninit::uninit() for sockaddr_storage in accept() and recvfrom() since these functions fill in the address structure
  • Replace mem::zeroed() with MaybeUninit::uninit() for pthread_attr_t in thread-related functions since pthread_attr_init() initializes the structure
  • Add references to man pages to document this behavior

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thomcc (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 10, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
joboet
joboet requested changes Feb 11, 2025
View reviewed changes
Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this a bit. We probably can't use .assume_init() as often as I thought since the C libraries neither guarantee that the structure can be moved, nor that it is fully initialized.

@bors
Copy link
Contributor

bors commented Feb 13, 2025

☔ The latest upstream changes (presumably #136954) made this pull request unmergeable. Please resolve the merge conflicts.

@xizheyin xizheyin requested a review from joboet February 13, 2025 12:29
@joboet joboet mentioned this pull request Feb 13, 2025
Closed
@xizheyin
Copy link
Contributor Author

That makes sense! I have removed assume_init. As far as I understand it, there is no difference in program behavior between using as_mut_ptr/as_ptr directly and using assume_init, but there is a difference in terms of intent. One does not assume that C performs the initialization, the other assumes that C must perform the initialization. So, we should just pass the pointer to C's API, and then C fills it, and it's not up to us to consider whether it's init or not. Is that correct? @thomcc

@xizheyin xizheyin requested a review from thomcc February 19, 2025 07:53
joboet
joboet requested changes Feb 19, 2025
View reviewed changes
@xizheyin
fix by comments
70f11ee 
Signed-off-by: xizheyin <[email protected]>
@xizheyin xizheyin requested a review from joboet February 19, 2025 12:17
@thomcc
Copy link
Member

thomcc commented Feb 22, 2025

@bors r+

@bors
Copy link
Contributor

bors commented Feb 22, 2025

📌 Commit 70f11ee has been approved by thomcc

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2025
@jhpratt jhpratt mentioned this pull request Feb 23, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2025
@bors
Auto merge of rust-lang#137466 - jhpratt:rollup-spyi02y, r=jhpratt
1805b33 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#135354 ([Debuginfo] Add MSVC Synthetic and Summary providers to LLDB)
 - rust-lang#136826 (Replace mem::zeroed with mem::MaybeUninit::uninit for large struct in Unix)
 - rust-lang#137194 (More const {} init in thread_local)
 - rust-lang#137334 (Greatly simplify lifetime captures in edition 2024)
 - rust-lang#137382 (bootstrap: add doc for vendor build step)
 - rust-lang#137423 (Improve a bit HIR pretty printer)
 - rust-lang#137435 (Fix "missing match arm body" suggestion involving `!`)
 - rust-lang#137448 (Fix bugs due to unhandled `ControlFlow` in compiler)
 - rust-lang#137458 (Fix missing self subst when rendering `impl Fn*<T>` with no output type)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4493159 into rust-lang:master Feb 23, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 23, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2025
@rust-timer
Unrolled build for rust-lang#136826
cbf6492 
Rollup merge of rust-lang#136826 - xizheyin:issue-136737, r=thomcc

Replace mem::zeroed with mem::MaybeUninit::uninit for large struct in Unix

As discussion in rust-lang#136737.

- Replace `mem::zeroed()` with `MaybeUninit::uninit()` for `sockaddr_storage` in `accept()` and `recvfrom()` since these functions fill in the address structure
- Replace `mem::zeroed()` with `MaybeUninit::uninit()` for `pthread_attr_t` in thread-related functions since `pthread_attr_init()` initializes the structure
- Add references to man pages to document this behavior
@SergioGasquez SergioGasquez mentioned this pull request Feb 25, 2025
Merged
fmease added a commit to fmease/rust that referenced this pull request Feb 26, 2025
@fmease
Rollup merge of rust-lang#137620 - SergioGasquez:fix/espidf-maybeunit…
ba41541 
…, r=ChrisDenton

Fix `attr` cast for espidf

rust-lang#136826 broke ESP-IDF builds with: https://github.com/esp-rs/esp-idf-template/actions/runs/13516221587/job/37765336588.

This PR fixes it.

cc: `@ivmarkov` `@xizheyin`
fmease added a commit to fmease/rust that referenced this pull request Feb 26, 2025
@fmease
Rollup merge of rust-lang#137620 - SergioGasquez:fix/espidf-maybeunit…
c5d5727 
…, r=ChrisDenton

Fix `attr` cast for espidf

rust-lang#136826 broke ESP-IDF builds with: https://github.com/esp-rs/esp-idf-template/actions/runs/13516221587/job/37765336588.

This PR fixes it.

cc: ``@ivmarkov`` ``@xizheyin``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2025
@rust-timer
Unrolled build for rust-lang#137620
1a184cb 
Rollup merge of rust-lang#137620 - SergioGasquez:fix/espidf-maybeunit, r=ChrisDenton

Fix `attr` cast for espidf

rust-lang#136826 broke ESP-IDF builds with: https://github.com/esp-rs/esp-idf-template/actions/runs/13516221587/job/37765336588.

This PR fixes it.

cc: ``@ivmarkov`` ``@xizheyin``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomcc thomcc Awaiting requested review from thomcc

@joboet joboet Awaiting requested review from joboet

Assignees

@thomcc thomcc

Labels
O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
6 participants
@xizheyin @rustbot @rust-log-analyzer @bors @thomcc @joboet