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

Future-proof the compiler for Box<T, A>. #47043

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@eddyb
Member

eddyb commented Dec 28, 2017

@Ericson2314 is playing around with Box<T, A = Heap> and there is still some special-casing in the compiler around Box, some of which is removed by this PR.
Ideally we can merge this before the beta cutoff to avoid needing a cfg(stage0) copy of Box.

r? @alexcrichton

@Ericson2314

This comment has been minimized.

Contributor

Ericson2314 commented Dec 28, 2017

For the record, Work is https://github.com/QuiltOS/rust/tree/allocator-error and described at #32838 (comment)

I'll kick off the rustc before I go to sleep, and test against my branch in the morning

Thanks again @eddyb!

@Ericson2314

This comment has been minimized.

Contributor

Ericson2314 commented Dec 28, 2017

@eddyb tested and everything indeed builds! I guess I should make a toy executable next?

@Ericson2314 Ericson2314 referenced this pull request Dec 28, 2017

Open

Allocator traits and std::heap #32838

5 of 12 tasks complete

@eddyb eddyb force-pushed the eddyb:future-box branch from 2c87b6a to 99bf699 Dec 29, 2017

@eddyb

This comment has been minimized.

Member

eddyb commented Dec 29, 2017

Added one more commit to allow changing box_free's signature. r? @nikomatsakis

@Ericson2314

This comment has been minimized.

Contributor

Ericson2314 commented Dec 29, 2017

@eddyb, with or without the functionality you specified (which I put in in alloc::string)

#[unstable(feature = "asdfasdf", issue = "12345678")]
#[derive(Debug)]
pub struct FooAlloc(usize);

#[unstable(feature = "asdfasdf", issue = "12345678")]
unsafe impl ::allocator::Alloc for FooAlloc {
    type Err = !;

    unsafe fn alloc(&mut self, layout: ::allocator::Layout) -> Result<*mut u8, Self::Err> {
        ::abort_adapter::AbortAdapter(::heap::Heap).alloc(layout)
    }

    unsafe fn dealloc(&mut self, ptr: *mut u8, layout: ::allocator::Layout) {
        ::abort_adapter::AbortAdapter(::heap::Heap).dealloc(ptr, layout)
    }
}

#[unstable(feature = "asdfasdf", issue = "12345678")]
pub fn foo(x: Box<String, FooAlloc>, y: bool) { if y { let _x = *x; } }

I get, when compiled with the old box_free:

thread 'rustc' panicked at 'assertion failed: sig.variadic || extra_args.is_empty()', src/librustc_trans/abi.rs:736:13

With the fixed box_free, however, everything works correctly!

@eddyb

This comment has been minimized.

Member

eddyb commented Jan 2, 2018

@rust-lang/infra There's no way for this to get into beta, is there?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Jan 2, 2018

@eddyb this can be backported like most other backports, right?

@Ericson2314

This comment has been minimized.

Contributor

Ericson2314 commented Jan 2, 2018

@alexcrichton I'd think so. it works with the standard library as it exists today.

@eddyb

This comment has been minimized.

Member

eddyb commented Jan 2, 2018

To be clear, I mean get into the nightly and then make the cut into the next beta. Backporting it after the cut seems okay-ish, assuming it gets a proper review from @nikomatsakis or someone else.

@Ericson2314

This comment has been minimized.

Contributor

Ericson2314 commented Jan 2, 2018

Hmm yeah the point would be to have it in beta for the sake of bootstrapping nightly, not to eventually get it into beta so that it makes stable but nightly needs to wait for the next beta.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Jan 3, 2018

@bors r+

This is a big PR to backport, though, and not entirely trivial.

@bors

This comment has been minimized.

Contributor

bors commented Jan 3, 2018

📌 Commit 99bf699 has been approved by nikomatsakis

@bors

This comment has been minimized.

Contributor

bors commented Jan 3, 2018

⌛️ Testing commit 99bf699 with merge a48c098...

bors added a commit that referenced this pull request Jan 3, 2018

Auto merge of #47043 - eddyb:future-box, r=nikomatsakis
Future-proof the compiler for Box<T, A>.

@Ericson2314 is playing around with `Box<T, A = Heap>` and there is still some special-casing in the compiler around `Box`, some of which is removed by this PR.
Ideally we can merge this before the beta cutoff to avoid needing a `cfg(stage0)` copy of `Box`.

r? @alexcrichton
@bors

This comment has been minimized.

Contributor

bors commented Jan 4, 2018

💔 Test failed - status-appveyor

@kennytm

This comment has been minimized.

Member

kennytm commented Jan 4, 2018

The backtrace tests failed on i686-pc-windows-msvc, maybe legit. These two tests are passing on x86_64-pc-windows-msvc and i686-pc-windows-gnu.

failures:
---- [run-pass] run-pass\backtrace-debuginfo.rs stdout ----
	
error: test run failed!
status: exit code: 101
command: PATH="C:\projects\rust\build\i686-pc-windows-msvc\stage2\lib\rustlib\i686-pc-windows-msvc\lib;C:\Program Files (x86)\Windows Kits\10\bin\x86;C:\Program Files (x86)\Windows Kits\10\bin\10.0.14393.0\x86;C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64;C:\projects\rust\build\i686-pc-windows-msvc\stage0-tools\i686-pc-windows-msvc\release\deps;C:\projects\rust\build\i686-pc-windows-msvc\stage0-sysroot\lib\rustlib\i686-pc-windows-msvc\lib;C:\Program Files (x86)\Inno Setup 5;C:\Python27;C:\msys64\mingw32\bin;C:\msys64\usr\bin;C:\Perl\site\bin;C:\Perl\bin;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Program Files\7-Zip;C:\Program Files\Microsoft\Web Platform Installer;C:\Tools\GitVersion;C:\Tools\PsTools;C:\Program Files\Git LFS;C:\Program Files (x86)\Subversion\bin;C:\Program Files\Microsoft SQL Server\120\Tools\Binn;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\110\Tools\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\Tools\Binn;C:\Program Files\Microsoft SQL Server\120\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\Tools\Binn\ManagementStudio;C:\Tools\WebDriver;C:\Program Files (x86)\Microsoft SDKs\TypeScript\1.4;C:\Program Files (x86)\Microsoft Visual Studio 12.0\Common7\IDE\PrivateAssemblies;C:\Program Files (x86)\Microsoft SDKs\Azure\CLI\wbin;C:\Ruby193\bin;C:\Tools\NUnit\bin;C:\Tools\xUnit;C:\Tools\MSpec;C:\Tools\Coverity\bin;C:\Program Files (x86)\CMake\bin;C:\go\bin;C:\Program Files\Java\jdk1.8.0\bin;C:\Python27;C:\Program Files\nodejs;C:\Program Files (x86)\iojs;C:\Program Files\iojs;C:\Users\appveyor\AppData\Roaming\npm;C:\Program Files\Microsoft SQL Server\130\Tools\Binn;C:\Program Files (x86)\MSBuild\14.0\Bin;C:\Tools\NuGet;C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\IDE\CommonExtensions\Microsoft\TestWindow;C:\Program Files\Microsoft DNX\Dnvm;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\130\Tools\Binn;C:\Program Files (x86)\Microsoft SQL Server\130\Tools\Binn;C:\Program Files (x86)\Microsoft SQL Server\130\DTS\Binn;C:\Program Files\Microsoft SQL Server\130\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\110\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\DTS\Binn;C:\Program Files (x86)\Apache\Maven\bin;C:\Python27\Scripts;C:\Tools\NUnit3;C:\Program Files\Mercurial;C:\Program Files\LLVM\bin;C:\Program Files\dotnet;C:\Program Files\erl8.3\bin;C:\Tools\curl\bin;C:\Program Files\Amazon\AWSCLI;C:\Program Files (x86)\Microsoft SQL Server\140\DTS\Binn;C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\IDE\Extensions\Microsoft\SQLDB\DAC\140;C:\Program Files\Git\cmd;C:\Program Files\Git\usr\bin;C:\Tools\vcpkg;C:\Program Files\Microsoft Service Fabric\bin\Fabric\Fabric.Code;C:\Program Files\Microsoft SDKs\Service Fabric\Tools\ServiceFabricLocalClusterManager;C:\Program Files (x86)\Yarn\bin;C:\ProgramData\chocolatey\bin;C:\Program Files (x86)\Microsoft SQL Server\140\Tools\Binn;C:\Program Files\Microsoft SQL Server\140\Tools\Binn;C:\Program Files\Microsoft SQL Server\140\DTS\Binn;C:\Program Files (x86)\nodejs;C:\Users\appveyor\AppData\Local\Yarn\bin;C:\Users\appveyor\AppData\Roaming\npm;C:\Program Files\AppVeyor\BuildAgent;C:\projects\rust;C:\projects\rust\handle" "C:\\projects\\rust\\build\\i686-pc-windows-msvc\\test\\run-pass\\backtrace-debuginfo.stage2-i686-pc-windows-msvc.exe"
stdout:
------------------------------------------
------------------------------------------
stderr:
------------------------------------------
thread 'main' panicked at 'trace does not match position list: test case 0
thread 'main' panicked at 'explicit panic', C:\projects\rust\src/test\run-pass\backtrace-debuginfo.rs:77:5
stack backtrace:
   0: 0x7471399f - std::panicking::Location::column::h2aaafe34e8f24eb6
   1: 0x74710f1f - <&'a T as core::fmt::Debug>::fmt::hfffa8a776ccdaf7b
   2: 0x74713f71 - std::panicking::rust_panic_with_hook::h94e3079805c14424
   3:   0x5affff - <unknown>
---
backtrace-debuginfo.rs:166
', C:\projects\rust\src/test\run-pass\backtrace-debuginfo.rs:132:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
------------------------------------------
thread '[run-pass] run-pass\backtrace-debuginfo.rs' panicked at 'explicit panic', src\tools\compiletest\src\runtest.rs:2776:8
note: Run with `RUST_BACKTRACE=1` for a backtrace.
---- [run-pass] run-pass\backtrace.rs stdout ----
	
error: test run failed!
status: exit code: 101
command: PATH="C:\projects\rust\build\i686-pc-windows-msvc\stage2\lib\rustlib\i686-pc-windows-msvc\lib;C:\Program Files (x86)\Windows Kits\10\bin\x86;C:\Program Files (x86)\Windows Kits\10\bin\10.0.14393.0\x86;C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64;C:\projects\rust\build\i686-pc-windows-msvc\stage0-tools\i686-pc-windows-msvc\release\deps;C:\projects\rust\build\i686-pc-windows-msvc\stage0-sysroot\lib\rustlib\i686-pc-windows-msvc\lib;C:\Program Files (x86)\Inno Setup 5;C:\Python27;C:\msys64\mingw32\bin;C:\msys64\usr\bin;C:\Perl\site\bin;C:\Perl\bin;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Program Files\7-Zip;C:\Program Files\Microsoft\Web Platform Installer;C:\Tools\GitVersion;C:\Tools\PsTools;C:\Program Files\Git LFS;C:\Program Files (x86)\Subversion\bin;C:\Program Files\Microsoft SQL Server\120\Tools\Binn;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\110\Tools\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\Tools\Binn;C:\Program Files\Microsoft SQL Server\120\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\Tools\Binn\ManagementStudio;C:\Tools\WebDriver;C:\Program Files (x86)\Microsoft SDKs\TypeScript\1.4;C:\Program Files (x86)\Microsoft Visual Studio 12.0\Common7\IDE\PrivateAssemblies;C:\Program Files (x86)\Microsoft SDKs\Azure\CLI\wbin;C:\Ruby193\bin;C:\Tools\NUnit\bin;C:\Tools\xUnit;C:\Tools\MSpec;C:\Tools\Coverity\bin;C:\Program Files (x86)\CMake\bin;C:\go\bin;C:\Program Files\Java\jdk1.8.0\bin;C:\Python27;C:\Program Files\nodejs;C:\Program Files (x86)\iojs;C:\Program Files\iojs;C:\Users\appveyor\AppData\Roaming\npm;C:\Program Files\Microsoft SQL Server\130\Tools\Binn;C:\Program Files (x86)\MSBuild\14.0\Bin;C:\Tools\NuGet;C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\IDE\CommonExtensions\Microsoft\TestWindow;C:\Program Files\Microsoft DNX\Dnvm;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\130\Tools\Binn;C:\Program Files (x86)\Microsoft SQL Server\130\Tools\Binn;C:\Program Files (x86)\Microsoft SQL Server\130\DTS\Binn;C:\Program Files\Microsoft SQL Server\130\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\110\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\DTS\Binn;C:\Program Files (x86)\Apache\Maven\bin;C:\Python27\Scripts;C:\Tools\NUnit3;C:\Program Files\Mercurial;C:\Program Files\LLVM\bin;C:\Program Files\dotnet;C:\Program Files\erl8.3\bin;C:\Tools\curl\bin;C:\Program Files\Amazon\AWSCLI;C:\Program Files (x86)\Microsoft SQL Server\140\DTS\Binn;C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\IDE\Extensions\Microsoft\SQLDB\DAC\140;C:\Program Files\Git\cmd;C:\Program Files\Git\usr\bin;C:\Tools\vcpkg;C:\Program Files\Microsoft Service Fabric\bin\Fabric\Fabric.Code;C:\Program Files\Microsoft SDKs\Service Fabric\Tools\ServiceFabricLocalClusterManager;C:\Program Files (x86)\Yarn\bin;C:\ProgramData\chocolatey\bin;C:\Program Files (x86)\Microsoft SQL Server\140\Tools\Binn;C:\Program Files\Microsoft SQL Server\140\Tools\Binn;C:\Program Files\Microsoft SQL Server\140\DTS\Binn;C:\Program Files (x86)\nodejs;C:\Users\appveyor\AppData\Local\Yarn\bin;C:\Users\appveyor\AppData\Roaming\npm;C:\Program Files\AppVeyor\BuildAgent;C:\projects\rust;C:\projects\rust\handle" "C:\\projects\\rust\\build\\i686-pc-windows-msvc\\test\\run-pass\\backtrace.stage2-i686-pc-windows-msvc.exe"
stdout:
------------------------------------------
------------------------------------------
stderr:
------------------------------------------
thread 'main' panicked at 'bad output: thread 'main' panicked at 'explicit panic', C:\projects\rust\src/test\run-pass\backtrace.rs:25:9
stack backtrace:
   0: std::panicking::Location::column
   1: <&'a T as core::fmt::Debug>::fmt
   2: std::panicking::rust_panic_with_hook
   3: <unknown>
', C:\projects\rust\src/test\run-pass\backtrace.rs:60:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
------------------------------------------
thread '[run-pass] run-pass\backtrace.rs' panicked at 'explicit panic', src\tools\compiletest\src\runtest.rs:2776:8
failures:
    [run-pass] run-pass\backtrace-debuginfo.rs
    [run-pass] run-pass\backtrace.rs
test result: FAILED. 2857 passed; 2 failed; 22 ignored; 0 measured; 0 filtered out
thread 'main' panicked at 'Some tests failed', src\tools\compiletest\src\main.rs:476:21
@eddyb

This comment has been minimized.

Member

eddyb commented Jan 4, 2018

@alexcrichton IIRC, 32-bit x86 with MSVC uses a different unwinding scheme than GNU.
Is it possible it's sensitive to type debuginfo? AFAICT, that's the only relevant change.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Jan 4, 2018

Hm maybe? It's true that the unwinding scheme of i686 MSVC is totally unique to that target, and I believe the method we acquire backtraces with does indeed require debug info (but unwinding doesn't require deubginfo). I wouldn't know where to start looking unfortunately though.

@eddyb eddyb force-pushed the eddyb:future-box branch from 99bf699 to 0ee960f Jan 5, 2018

@bors

This comment has been minimized.

Contributor

bors commented Jan 5, 2018

📌 Commit 0ee960f has been approved by nikomatsakis

@bors

This comment has been minimized.

Contributor

bors commented Jan 5, 2018

⌛️ Testing commit 0ee960f with merge 3dabf28...

bors added a commit that referenced this pull request Jan 5, 2018

Auto merge of #47043 - eddyb:future-box, r=nikomatsakis
Future-proof the compiler for Box<T, A>.

@Ericson2314 is playing around with `Box<T, A = Heap>` and there is still some special-casing in the compiler around `Box`, some of which is removed by this PR.
Ideally we can merge this before the beta cutoff to avoid needing a `cfg(stage0)` copy of `Box`.

r? @alexcrichton
@bors

This comment has been minimized.

Contributor

bors commented Jan 5, 2018

💔 Test failed - status-appveyor

@eddyb eddyb force-pushed the eddyb:future-box branch from 0ee960f to 5043534 Jan 6, 2018

@eddyb

This comment has been minimized.

Member

eddyb commented Jan 6, 2018

@bors r=nikomatsakis (now with the LLVM pointer types for Box<T: Sized> restored)

@bors

This comment has been minimized.

Contributor

bors commented Jan 6, 2018

📌 Commit 5043534 has been approved by nikomatsakis

@bors

This comment has been minimized.

Contributor

bors commented Jan 6, 2018

⌛️ Testing commit 5043534 with merge 52ccf67...

bors added a commit that referenced this pull request Jan 6, 2018

Auto merge of #47043 - eddyb:future-box, r=nikomatsakis
Future-proof the compiler for Box<T, A>.

@Ericson2314 is playing around with `Box<T, A = Heap>` and there is still some special-casing in the compiler around `Box`, some of which is removed by this PR.
Ideally we can merge this before the beta cutoff to avoid needing a `cfg(stage0)` copy of `Box`.

r? @alexcrichton
@bors

This comment has been minimized.

Contributor

bors commented Jan 6, 2018

💔 Test failed - status-appveyor

@eddyb

This comment has been minimized.

Member

eddyb commented Jan 6, 2018

Oddly enough, with each try the backtraces get longer, but they're still mostly <unknown>.

@shepmaster

This comment has been minimized.

Member

shepmaster commented Jan 13, 2018

Ping from triage, @eddyb! Any luck figuring out the build issues?

@eddyb

This comment has been minimized.

Member

eddyb commented Jan 13, 2018

@shepmaster Nope, although I could try to take a fresh look and poke at it some more.

@bors

This comment has been minimized.

Contributor

bors commented Jan 16, 2018

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

@carols10cents

This comment has been minimized.

Member

carols10cents commented Jan 22, 2018

Triage ping for you @eddyb ! Looks like there are some merge conflicts for you too.

@kennytm

This comment has been minimized.

Member

kennytm commented Jan 31, 2018

Since this PR has been inactive for more than 2 weeks, I'm closing it to keep the queue clean. Feel free to reopen anytime once the build issues have been figured out.

@kennytm kennytm closed this Jan 31, 2018

bors added a commit that referenced this pull request Apr 20, 2018

Auto merge of #50097 - glandium:box_free, r=<try>
Partial future-proofing for Box<T, A>

In some ways, this is similar to @eddyb's PR #47043 that went stale, but doesn't cover everything. Notably, this still leaves Box internalized as a pointer in places, so practically speaking, only ZSTs can be practically added to the Box type with the changes here (the compiler ICEs otherwise).

No change to liballoc is introduced here, that's left for the future because I want to test that further first, but this puts things in place in a way that hopefully will make things easier (notably, a compiler built with these changes can still build the current liballoc code *and* future changes to it).

bors added a commit that referenced this pull request Apr 27, 2018

Auto merge of #50097 - glandium:box_free, r=nikomatsakis
Partial future-proofing for Box<T, A>

In some ways, this is similar to @eddyb's PR #47043 that went stale, but doesn't cover everything. Notably, this still leaves Box internalized as a pointer in places, so practically speaking, only ZSTs can be practically added to the Box type with the changes here (the compiler ICEs otherwise).

The Box type is not changed here, that's left for the future because I want to test that further first, but this puts things in place in a way that hopefully will make things easier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment