Skip to content

/rust

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

Associate an allocator to boxes #55139

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@SimonSapin SimonSapin

Assignees

@joshtriplett joshtriplett

Labels
S-blocked-closed S-inactive-closed
Projects
None yet
Milestone
No milestone
7 participants
@glandium @rust-highfive @Centril @TimNN @bors @SimonSapin @joshtriplett
@glandium
Copy link
Contributor

glandium commented Oct 17, 2018 •
edited

This turns Box<T> into Box<T, A: Alloc = Global>. This is a
minimalist change to achieve this, not touching anything that could have
backwards incompatible consequences like requiring type annotations in
places where they currently aren't required,
per #50822 (comment)

This is a rebased version of #50882, meant primarily to get CI results and see if #52694 is still a problem.

Cc: @SimonSapin

@rust-highfive rust-highfive assigned joshtriplett Oct 17, 2018

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Oct 17, 2018

r? @joshtriplett

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

@rust-highfive rust-highfive added the S-waiting-on-review label Oct 17, 2018

@glandium
Associate an allocator to boxes 
This turns `Box<T>` into `Box<T, A: Alloc = Global>`. This is a
minimalist change to achieve this, not touching anything that could have
backwards incompatible consequences like requiring type annotations in
places where they currently aren't required,
per #50822 (comment)
Loading status checks…
aa19e43

@glandium glandium force-pushed the glandium:box branch from f03e1c5 to aa19e43 Oct 17, 2018

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Oct 17, 2018

The job x86_64-gnu-llvm-5.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. 
[00:47:26] .................................................................................................... 2200/4605
[00:47:30] ...................i................................................................................ 2300/4605
[00:47:34] .................................................................................................... 2400/4605
[00:47:37] .................................................................................................... 2500/4605
[00:47:41] ................................iiiiiiiii........................................................... 2600/4605
[00:47:47] .................................................................................................... 2800/4605
[00:47:51] .................................................................................................... 2900/4605
[00:47:54] ......................................................i............................................. 3000/4605
[00:47:57] .................................................................................................... 3100/4605
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:00:38] 
[01:00:38] running 111 tests
[01:00:41] i..ii...iii.......i...i.........i..iii...........i.....i.....ii...i.i.ii..............i...ii..ii.i.. 100/111
[01:00:41] ..iiii.....
[01:00:41] 
[01:00:41]  finished in 3.387
[01:00:41] travis_fold:end:test_codegen

---
[01:15:27]  right: `2`', libstd/sync/mutex.rs:661:13
[01:15:27] thread '<unnamed>' panicked at 'explicit panic', libstd/sync/once.rs:582:28
[01:15:27] thread '<unnamed>' panicked at 'Once instance has previously been poisoned', libstd/sync/once.rs:372:21
[01:15:27] thread '<unnamed>' panicked at 'explicit panic', libstd/sync/once.rs:610:28
[01:15:27] thread '<unnamed>' panicked at 'assertion failed: t1.join().is_ok()', libstd/sync/once.rs:638:9
[01:15:27] thread '<unnamed>' panicked at 'test panic in inner thread to poison RwLock', libstd/sync/rwlock.rs:774:13
[01:15:27] thread '<unnamed>' panicked at 'explicit panic', libstd/sync/rwlock.rs:711:13
[01:15:27] thread '<unnamed>' panicked at 'explicit panic', libstd/sync/rwlock.rs:641:13
[01:15:27] thread '<unnamed>' panicked at 'explicit panic', libstd/sync/rwlock.rs:652:13
[01:15:27] thread '<unnamed>' panicked at 'explicit panic', libstd/sync/rwlock.rs:652:13
[01:15:27] thread '<unnamed>' panicked at 'explicit panic', libstd/sync/rwlock.rs:617:13
[01:15:27] thread '<unnamed>' panicked at 'explicit panic', libstd/sync/rwlock.rs:629:13
[01:15:28] thread '<unnamed>' panicked at 'What the answer to my lifetimes dilemma is?', libstd/sys_common/remutex.rs:241:13
[01:15:28] .......................................F............................................................ 700/768
[01:15:28] thread '<unnamed>' panicked at 'index 2 and/or 4 in `"aé 💩"` do not lie on character boundary', libstd/sys_common/wtf8.rs:784:5
[01:15:28] thread '<unnamed>' panicked at 'index 5 and/or 8 in `"aé 💩"` do not lie on character boundary', libstd/sys_common/wtf8.rs:784:5
[01:15:28] thread '<unnamed>' panicked at 'assertion failed: is_code_point_boundary(self, new_len)', libstd/sys_common/wtf8.rs:329:9
[01:15:28] thread '<unnamed>' panicked at 'assertion failed: is_code_point_boundary(self, new_len)', libstd/sys_common/wtf8.rs:329:9
---
[01:15:37] 
[01:15:37] error: test failed, to rerun pass '--lib'
[01:15:37] 
[01:15:37] 
[01:15:37] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std" "--" "--quiet"
[01:15:37] 
[01:15:37] 
[01:15:37] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:15:37] Build completed unsuccessfully in 0:32:49
[01:15:37] Build completed unsuccessfully in 0:32:49
[01:15:37] Makefile:58: recipe for target 'check' failed
[01:15:37] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:2f0cee8e
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:1a7273f0:start=1539765920243640848,finish=1539765920364819527,duration=121178679
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1070e04b
$ 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:12ccf678
$ 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)
@glandium

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

glandium commented Oct 17, 2018

As expected #52694 is still a problem. There is no way around it, short of requiring LLVM 7 for system LLVM builds. One problem here being that there isn't even a llvm-7-tools package for any version of Ubuntu (and travis builds are using 16.04, which doesn't even have LLVM 6)
@Centril

This comment has been minimized.

Sign in to view
Copy link
Contributor

Centril commented Oct 17, 2018

This PR introduces some new unsafe { .. }. It would be nice to have a comment on each usage.
@SimonSapin

SimonSapin reviewed Oct 17, 2018
View changes

src/liballoc/alloc.rs
let ptr = ptr.as_ptr();
let size = size_of_val(&*ptr);
let align = min_align_of_val(&*ptr);
// We do not allocate for Box<T> when T is ZST, so deallocation is also not necessary.
if size != 0 {
let layout = Layout::from_size_align_unchecked(size, align);
dealloc(ptr as *mut u8, layout);
a.dealloc(NonNull::new_unchecked(ptr).cast(), layout);

This comment has been minimized.

Sign in to view
Copy link
@SimonSapin

SimonSapin Oct 17, 2018

Contributor

impl<T> From<Unique<T>> for NonNull<T> is safe and can be used here.
@TimNN

This comment has been minimized.

Sign in to view
Copy link
Contributor

TimNN commented Oct 30, 2018

Ping from triage! Is there a way to work around the LLVM issues, short of requiring at least LLVM 7.0? Raising the minimum LLVM version would probably involve some kind of team RFC, but I'm not sure which team would be most appropriate.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Nov 2, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

TimNN commented Nov 13, 2018

I'm closing this as blocked on #55842.

@glandium: If you can come up with a way that does not require LLVM 7.0, please feel free to re-open. Thanks for your PR!

@TimNN TimNN closed this Nov 13, 2018

@TimNN TimNN added S-inactive-closed S-blocked-closed and removed S-waiting-on-review labels Nov 13, 2018

@kennytm kennytm referenced this pull request Feb 14, 2019

Open

Associate an allocator to boxes #58457

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.