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

Update boxed::Box docs on memory layout #60963

Merged
merged 3 commits into from
May 22, 2019
Merged

Conversation

blkerby
Copy link
Contributor

@blkerby blkerby commented May 19, 2019

The existing docs for the Box type state that "the way Box allocates and releases memory is unspecified", and that therefore the only valid pointer to pass to Box::from_raw is one obtained from Box::into_raw. This is inconsistent with the module-level docs which specify,

It is valid to convert both ways between a Box and a raw pointer allocated with the Global allocator, given that the Layout used with the allocator is correct for the type. More precisely, a value: *mut T that has been allocated with the Global allocator with Layout::for_value(&*value) may be converted into a box using Box::::from_raw(value). Conversely, the memory backing a value: *mut T obtained from Box::::into_raw may be deallocated using the Global allocator with Layout::for_value(&*value).

This pull request updates the docs for Box to make them consistent with the module-level docs and adds some examples of how to use the global allocator in conjunction with Box::from_raw and Box::into_raw.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:042a5b50:start=1558280043988078483,finish=1558280130812161724,duration=86824083241
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:04:11] tidy error: /checkout/src/liballoc/boxed.rs:194: trailing whitespace
[00:04:15] some tidy checks failed
[00:04:15] 
[00:04:15] 
[00:04:15] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:15] 
[00:04:15] 
[00:04:15] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:15] Build completed unsuccessfully in 0:01:15
[00:04:15] Build completed unsuccessfully in 0:01:15
[00:04:15] make: *** [tidy] Error 1
[00:04:15] Makefile:67: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0fa7e254
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun May 19 15:39:55 UTC 2019
---
travis_time:end:04659828:start=1558280396565573518,finish=1558280396570133112,duration=4559594
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:09af0dd0
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0126c068
travis_time:start:0126c068
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:1dcda038
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@shepmaster
Copy link
Member

Whew. Above my paygrade.

r? @alexcrichton

@hellow554
Copy link
Contributor

Could you rebase and then force-push your commits? Merge commits are not wanted. See https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#pull-requests

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for the clarifications here!

/// only valid pointer to pass to this function is the one taken
/// from another `Box` via the [`Box::into_raw`] function.
/// the destructor of `T` and free the allocated memory. For this
/// to be safe, the memory must have been allocated in the precise
Copy link
Member

Choose a reason for hiding this comment

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

Could this perhaps avoid duplicating the module docs? Maybe a section could be created in the module docs specifying the global allocator business, and this coul djust point there indicating how to interact with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great! I've just pushed a commit up to do this, creating a section called "Memory Layout" in the module docs. I also moved this section down to the bottom since it seemed better to have the simple examples first, before the more detailed information on memory layout.

///
/// let ptr = unsafe{ alloc(Layout::new::<i32>()) } as *mut i32;
/// unsafe{ *ptr = 5; }
/// let x = unsafe{ Box::from_raw(ptr) };
Copy link
Member

Choose a reason for hiding this comment

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

Instead of an unsafe on each line, could this just be wrapped in one block? Additionally could "rustfmt be run over the code examples" here? I think that there's typically a space after the unsafe word, for example.

Copy link
Member

@shepmaster shepmaster May 20, 2019

Choose a reason for hiding this comment

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

rustfmt be run over the code examples

This can actually be done with an unstable rustfmt option. You'll want to make sure to only add the formatting changes to the code you were already changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! And thanks for the tip on the new rustfmt feature; it worked!

@alexcrichton
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented May 21, 2019

📌 Commit 4e37785 has been approved by alexcrichton

@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 May 21, 2019
Centril added a commit to Centril/rust that referenced this pull request May 21, 2019
Update boxed::Box docs on memory layout

The existing docs for the `Box` type state that "the way `Box` allocates and releases memory is unspecified", and that therefore the only valid pointer to pass to `Box::from_raw` is one obtained from `Box::into_raw`. This is inconsistent with the module-level docs which specify,

> It is valid to convert both ways between a Box and a raw pointer allocated with the Global allocator, given that the Layout used with the allocator is correct for the type. More precisely, a value: *mut T that has been allocated with the Global allocator with Layout::for_value(&*value) may be converted into a box using Box::<T>::from_raw(value). Conversely, the memory backing a value: *mut T obtained from Box::<T>::into_raw may be deallocated using the Global allocator with Layout::for_value(&*value).

This pull request updates the docs for `Box` to make them consistent with the module-level docs and adds some examples of how to use the global allocator in conjunction with `Box::from_raw` and `Box::into_raw`.
Centril added a commit to Centril/rust that referenced this pull request May 21, 2019
Update boxed::Box docs on memory layout

The existing docs for the `Box` type state that "the way `Box` allocates and releases memory is unspecified", and that therefore the only valid pointer to pass to `Box::from_raw` is one obtained from `Box::into_raw`. This is inconsistent with the module-level docs which specify,

> It is valid to convert both ways between a Box and a raw pointer allocated with the Global allocator, given that the Layout used with the allocator is correct for the type. More precisely, a value: *mut T that has been allocated with the Global allocator with Layout::for_value(&*value) may be converted into a box using Box::<T>::from_raw(value). Conversely, the memory backing a value: *mut T obtained from Box::<T>::into_raw may be deallocated using the Global allocator with Layout::for_value(&*value).

This pull request updates the docs for `Box` to make them consistent with the module-level docs and adds some examples of how to use the global allocator in conjunction with `Box::from_raw` and `Box::into_raw`.
Centril added a commit to Centril/rust that referenced this pull request May 21, 2019
Update boxed::Box docs on memory layout

The existing docs for the `Box` type state that "the way `Box` allocates and releases memory is unspecified", and that therefore the only valid pointer to pass to `Box::from_raw` is one obtained from `Box::into_raw`. This is inconsistent with the module-level docs which specify,

> It is valid to convert both ways between a Box and a raw pointer allocated with the Global allocator, given that the Layout used with the allocator is correct for the type. More precisely, a value: *mut T that has been allocated with the Global allocator with Layout::for_value(&*value) may be converted into a box using Box::<T>::from_raw(value). Conversely, the memory backing a value: *mut T obtained from Box::<T>::into_raw may be deallocated using the Global allocator with Layout::for_value(&*value).

This pull request updates the docs for `Box` to make them consistent with the module-level docs and adds some examples of how to use the global allocator in conjunction with `Box::from_raw` and `Box::into_raw`.
Centril added a commit to Centril/rust that referenced this pull request May 22, 2019
Update boxed::Box docs on memory layout

The existing docs for the `Box` type state that "the way `Box` allocates and releases memory is unspecified", and that therefore the only valid pointer to pass to `Box::from_raw` is one obtained from `Box::into_raw`. This is inconsistent with the module-level docs which specify,

> It is valid to convert both ways between a Box and a raw pointer allocated with the Global allocator, given that the Layout used with the allocator is correct for the type. More precisely, a value: *mut T that has been allocated with the Global allocator with Layout::for_value(&*value) may be converted into a box using Box::<T>::from_raw(value). Conversely, the memory backing a value: *mut T obtained from Box::<T>::into_raw may be deallocated using the Global allocator with Layout::for_value(&*value).

This pull request updates the docs for `Box` to make them consistent with the module-level docs and adds some examples of how to use the global allocator in conjunction with `Box::from_raw` and `Box::into_raw`.
Centril added a commit to Centril/rust that referenced this pull request May 22, 2019
Update boxed::Box docs on memory layout

The existing docs for the `Box` type state that "the way `Box` allocates and releases memory is unspecified", and that therefore the only valid pointer to pass to `Box::from_raw` is one obtained from `Box::into_raw`. This is inconsistent with the module-level docs which specify,

> It is valid to convert both ways between a Box and a raw pointer allocated with the Global allocator, given that the Layout used with the allocator is correct for the type. More precisely, a value: *mut T that has been allocated with the Global allocator with Layout::for_value(&*value) may be converted into a box using Box::<T>::from_raw(value). Conversely, the memory backing a value: *mut T obtained from Box::<T>::into_raw may be deallocated using the Global allocator with Layout::for_value(&*value).

This pull request updates the docs for `Box` to make them consistent with the module-level docs and adds some examples of how to use the global allocator in conjunction with `Box::from_raw` and `Box::into_raw`.
bors added a commit that referenced this pull request May 22, 2019
Rollup of 10 pull requests

Successful merges:

 - #59742 (Move `edition` outside the hygiene lock and avoid accessing it)
 - #60581 (convert custom try macro to `?`)
 - #60963 (Update boxed::Box docs on memory layout)
 - #60973 (Avoid symbol interning in `file_metadata`.)
 - #60982 (Do not fail on child without DefId)
 - #60991 (LocalDecl push returns Local len)
 - #60995 (Add stream_to_parser_with_base_dir)
 - #60998 (static_assert: make use of anonymous constants)
 - #61003 (Remove impls for `InternedString`/string equality.)
 - #61006 (adjust deprecation date of mem::uninitialized)

Failed merges:

r? @ghost
@bors bors merged commit 4e37785 into rust-lang:master May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants