Only instantiate inline- and const-fns if they are referenced (again). #45575

michaelwoerister · 2017-10-27T12:37:00Z

It seems that we have regressed on not translating #[inline] functions unless they are actually used. This should bring back this optimization. I also added a regression test this time so it doesn't happen again accidentally.

Fixes #40392.

r? @alexcrichton

UPDATE & PSA

This patch makes translation very lazy -- in general this is a good thing (we don't want the compiler to do unnecessary work) but it has two consequences:

Some error messages are only generated when an item is actually translated. Consequently, this patch will lead to more cases where the compiler will only start emitting errors when the erroneous function is actually used. This has always been true to some extend (e.g. when passing generic values to an intrinsic) but since this is something user-facing it's worth mentioning.
When writing tests, one has to make sure that the functions in question are actually generated. In other words, it must not be dead code. This can usually be achieved by either
1. making sure the function is exported from the resulting binary or
2. by making sure the function is called from something that is exported (or main()).

Note that it depends on the crate type what functions are exported:

For rlibs and dylibs everything that is reachable from the outside is exported.
For executables, cdylibs, and staticlibs, items are only exported if they are additionally #[no_mangle] or have an #[export_name].

The commits in this PR contain many examples of how tests can be updated to comply to the new requirements.

alexcrichton · 2017-10-27T16:14:58Z

Looks great to me! r=me with passing tests

kennytm · 2017-10-27T18:23:39Z

CI is still failing. These tests compiled successfully while they should not:

[00:51:51]     [compile-fail] compile-fail/E0534.rs
[00:51:51]     [compile-fail] compile-fail/bad-intrinsic-monomorphization.rs
[00:51:51]     [compile-fail] compile-fail/dupe-symbols-2.rs
[00:51:51]     [compile-fail] compile-fail/invalid-inline.rs
[00:51:51]     [compile-fail] compile-fail/issue-22638.rs
[00:51:51]     [compile-fail] compile-fail/non-interger-atomic.rs

alexcrichton · 2017-10-31T13:46:35Z

This looks great to me, thanks @michaelwoerister! I'm a little hesitant to personally r+ due to the change in semantics here. We chatted on IRC and it sounded like you're going to wait for discussion amongst the @rust-lang/compiler team, so I'll tag the PR appropriately for that.

eddyb · 2017-10-31T16:32:57Z

Some error messages are only generated when an item is actually translated.

I should note that this mainly has to do with unstable features (e.g. direct uses of intrinsics).
AFAIK, there are only two other ways to get an error (not a warning) during codegen:

a type with a size in bits that cannot be represented in i64 (LLVM limitation), or more generally, with a size in bytes that cannot be represented in the target platform's usize
resource limits, typically recursion either during trait matching or the function monomorphizations themselves

We could turn these into warnings and generate runtime panics instead, in fact the warnings could be done ahead-of-time and distinct from codegen anyway.

michaelwoerister · 2017-11-01T15:39:55Z

We could turn these into warnings and generate runtime panics instead, in fact the warnings could be done ahead-of-time and distinct from codegen anyway.

Yes, I think this should be the preferred way to do it anyway.

What does the rest of @rust-lang/compiler say? Can we land this? I think the general goal of being as lazy as possible is worth pursuing.

arielb1 · 2017-11-02T15:36:26Z

src/test/compile-fail/dupe-symbols-2.rs

    #[no_mangle]
    pub extern fn fail() {
    }
 }

-mod b {
+pub mod b {
    #[no_mangle]
    pub extern fn fail() {


Shouldn't private no_mangle functions always be compiled in?

Seems like it would be good if that were true.

arielb1 · 2017-11-02T15:37:17Z

src/test/compile-fail/invalid-inline.rs

@@ -21,4 +21,8 @@ fn b() {
 fn c() {


Shouldn't we check the inline attribute for functions that aren't translated? I don't want people to create functions with an inline attribute that explode when a child crate finally instantiates them.

arielb1 · 2017-11-02T15:41:42Z

I'm not sure this is a difference in kind, merely a difference in degree: even today you can write things that depend on reachability and blow up only when a downstream crate uses them, for example:

#![feature(core_intrinsics)]

use std::intrinsics;

#[inline(fail)]
pub fn foo<T>(_t: T) {
    unsafe {
        intrinsics::atomic_load(1 as *const [u8; 1048576]);
    }
}

#[inline(always)]
pub fn bar() {
    unsafe {
        intrinsics::atomic_load(1 as *const [u8; 1048576]);
    }
}

fn main() {}

I still think we want to sanitize attributes during type-checking, but intrinsics (which are unstable and intentionally error on monomorphization time) and resource limits (which are always a problem) should be OK.

michaelwoerister · 2017-11-03T13:53:55Z

Discussed in the @rust-lang/compiler team meeting yesterday and the conclusion was to proceed with this. The main concern was that #[inline] attributes would not be validated for functions that are not called. This should be simple to fix and I opened #45738 to keep track of that.

@bors r=nikomatsakis

bors · 2017-11-03T13:53:56Z

📌 Commit 8d4c92f has been approved by nikomatsakis

bors · 2017-11-04T09:31:20Z

⌛ Testing commit 8d4c92f80d97bc0e42d935ce7d23f6e3083a4937 with merge df38d994283db81c5176dc109d1894cd5232625f...

bors · 2017-11-04T10:24:16Z

💔 Test failed - status-travis

kennytm · 2017-11-04T10:39:51Z

Test failed, test/mir-opt/match_false_edges/rustc.node17.SimplifyBranches-initial.before.mir does not exist:

[00:48:30] failures:
[00:48:30] 
[00:48:30] ---- [mir-opt] mir-opt/match_false_edges.rs stdout ----
[00:48:30] 	thread '[mir-opt] mir-opt/match_false_edges.rs' panicked at 'Output file `/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt/match_false_edges/rustc.node17.SimplifyBranches-initial.before.mir` from test does not exist', /checkout/src/tools/compiletest/src/runtest.rs:2290:12
[00:48:30] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:48:30]
[00:48:30] 
[00:48:30] failures:
[00:48:30]     [mir-opt] mir-opt/match_false_edges.rs

michaelwoerister · 2017-11-07T13:05:57Z

So I think these new failures are because #45710 got merged in between. The custom allocator implementations in these test cases fail linking because (I assume) the respective functions are not instantiated because they are not considered exported? Does that sound right, @alexcrichton?

I don't quite understand where these functions are supposed to come from (__rg_alloc, __rg_oom, __rg_grow_in_place, etc).

alexcrichton · 2017-11-07T14:54:14Z

@michaelwoerister oh dear sorry about that!

The #[global_allocator] attribute is where they come from, the expand module will turn such a static into something like:

static FOO: MyAllocator = ...;

mod some_internal_name {
    #[std_special_symbol]
    #[no_mangle]
    fn __rg_alloc(...) -> ... {
        FOO.some_method(...)
    }

    ...
}

So the symbols are injected by the compiler and their attributes are generated here. IIRC the linkage = "external" puts them on the path towards reachability and then rustc_std_internal_symbol was a change to make them hidden visibility instead of public.

michaelwoerister · 2017-11-07T14:59:19Z

Ah ok, the collector does not see the functions generated in librustc_trans/allocator.rs, and since we are creating an executable, these seemingly unreferenced SymbolExportLevel::Rust functions will not be instantiated.

michaelwoerister · 2017-11-07T15:02:56Z

@alexcrichton Do you think that there's a case where a #[rustc_std_internal_symbol] should not be instantiated? If not I think this could be solved by treating such functions the same as regularly exported functions in the collector.

alexcrichton · 2017-11-07T15:05:36Z

@michaelwoerister nah I think everything tagged with that should be unconditionally instantiated. They're all tiny anyway and will get gc'd by the linker if they're not used.

michaelwoerister · 2017-11-07T15:13:20Z

@alexcrichton 👍

michaelwoerister · 2017-11-07T21:17:07Z

OK, travis looks good. Let's give it another try.

@bors r=nikomatsakis

bors · 2017-11-07T21:17:08Z

📌 Commit d141abf has been approved by nikomatsakis

bors · 2017-11-08T04:07:02Z

⌛ Testing commit d141abf with merge 6dc72035c92f954c39240fbe4eeec544ef159331...

bors · 2017-11-08T05:04:08Z

💔 Test failed - status-travis

kennytm · 2017-11-08T06:29:53Z

dist-i586-gnu-i686-musl failed at codegen/fastcall-inreg.rs.

[00:51:57] ---- [codegen] codegen/fastcall-inreg.rs stdout ----
[00:51:57] 	
[00:51:57] error: verification with 'FileCheck' failed
[00:51:57] status: exit code: 1
[00:51:57] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--input-file" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/fastcall-inreg.ll" "/checkout/src/test/codegen/fastcall-inreg.rs"
[00:51:57] stdout:
[00:51:57] ------------------------------------------
[00:51:57] 
[00:51:57] ------------------------------------------
[00:51:57] stderr:
[00:51:57] ------------------------------------------
[00:51:57] /checkout/src/test/codegen/fastcall-inreg.rs:63:12: error: expected string not found in input
[00:51:57]  // CHECK: @f1(i32 inreg %arg0, i32 inreg %arg1, i32 %arg2)
[00:51:57]            ^
[00:51:57] /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/fastcall-inreg.ll:1:1: note: scanning from here
[00:51:57] ; ModuleID = 'fastcall_inreg0-8787f43e282added376259c1adb08b80.rs'
[00:51:57] ^
[00:51:57] /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/fastcall-inreg.ll:3:35: note: possible intended match here
[00:51:57] target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
[00:51:57]                                   ^
[00:51:57] 
[00:51:57] ------------------------------------------
[00:51:57] 
[00:51:57] thread '[codegen] codegen/fastcall-inreg.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2498:8
[00:51:57] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:51:57] 
[00:51:57] 
[00:51:57] failures:
[00:51:57]     [codegen] codegen/fastcall-inreg.rs
[00:51:57] 
[00:51:57] test result: �[31mFAILED�(B�[m. 47 passed; 1 failed; 5 ignored; 0 measured; 0 filtered out

…tantiated.

michaelwoerister · 2017-11-08T10:11:08Z

Updated test case.

@bors r=nikomatsakis

bors · 2017-11-08T10:11:08Z

📌 Commit 081ef8e has been approved by nikomatsakis

bors · 2017-11-08T12:23:41Z

⌛ Testing commit 081ef8e with merge 4bb96f6...

@alexcrichton

…ikomatsakis Only instantiate inline- and const-fns if they are referenced (again). It seems that we have regressed on not translating `#[inline]` functions unless they are actually used. This should bring back this optimization. I also added a regression test this time so it doesn't happen again accidentally. Fixes #40392. r? @alexcrichton UPDATE & PSA --------------------- This patch **makes translation very lazy** -- in general this is a good thing (we don't want the compiler to do unnecessary work) but it has two consequences: 1. Some error messages are only generated when an item is actually translated. Consequently, this patch will lead to more cases where the compiler will only start emitting errors when the erroneous function is actually used. This has always been true to some extend (e.g. when passing generic values to an intrinsic) but since this is something user-facing it's worth mentioning. 2. When writing tests, one has to make sure that the functions in question are actually generated. In other words, it must not be dead code. This can usually be achieved by either 1. making sure the function is exported from the resulting binary or 2. by making sure the function is called from something that is exported (or `main()`). Note that it depends on the crate type what functions are exported: 1. For rlibs and dylibs everything that is reachable from the outside is exported. 2. For executables, cdylibs, and staticlibs, items are only exported if they are additionally `#[no_mangle]` or have an `#[export_name]`. The commits in this PR contain many examples of how tests can be updated to comply to the new requirements.

bors · 2017-11-08T14:53:58Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 4bb96f6 to master...

michaelwoerister · 2017-11-08T15:47:53Z

Wow, cool :)

rust-highfive assigned alexcrichton Oct 27, 2017

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 27, 2017

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 27, 2017

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2017

michaelwoerister force-pushed the tweak-inline-trans-items branch from 4f785a5 to 8d4c92f Compare October 30, 2017 17:59

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2017

alexcrichton added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2017

kennytm added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 31, 2017

arielb1 reviewed Nov 2, 2017

View reviewed changes

michaelwoerister mentioned this pull request Nov 3, 2017

#[inline] attributes should be validated before translation. #45738

Closed

shepmaster 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 3, 2017

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 3, 2017

Fix some rebasing fallout.

8fcf3e3

Always treat #[rustc_std_internal_symbol]s as root TransItems.

d141abf

michaelwoerister force-pushed the tweak-inline-trans-items branch from 678be7d to d141abf Compare November 7, 2017 15:42

Update fastcall-inreg codegen test so that functions actually get ins…

081ef8e

…tantiated.

kennytm 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 8, 2017

bors merged commit 081ef8e into rust-lang:master Nov 8, 2017

bors mentioned this pull request Nov 8, 2017

Move collector to librustc_mir::monomorphize #45525

Merged

This was referenced Nov 21, 2017

Binaries built with 1.21.0 are 5x the size of those from 1.20.0 #46150

Closed

Firefox build failure for win32 with Rust 1.22 (exit code: 3221225477) #46045

Closed

alexcrichton mentioned this pull request Dec 3, 2017

Dead code not linked in beta or nightly #46467

Closed

japaric mentioned this pull request Jun 21, 2018

rust_begin_unwind not emitted when crate-type = bin #51671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only instantiate inline- and const-fns if they are referenced (again). #45575

Only instantiate inline- and const-fns if they are referenced (again). #45575

michaelwoerister commented Oct 27, 2017 •

edited

Loading

alexcrichton commented Oct 27, 2017

kennytm commented Oct 27, 2017

alexcrichton commented Oct 31, 2017

eddyb commented Oct 31, 2017 •

edited

Loading

michaelwoerister commented Nov 1, 2017

arielb1 Nov 2, 2017

nikomatsakis Nov 2, 2017

arielb1 Nov 2, 2017

arielb1 commented Nov 2, 2017 •

edited

Loading

michaelwoerister commented Nov 3, 2017

bors commented Nov 3, 2017

bors commented Nov 4, 2017

bors commented Nov 4, 2017

kennytm commented Nov 4, 2017

michaelwoerister commented Nov 7, 2017

alexcrichton commented Nov 7, 2017

michaelwoerister commented Nov 7, 2017

michaelwoerister commented Nov 7, 2017

alexcrichton commented Nov 7, 2017

michaelwoerister commented Nov 7, 2017

michaelwoerister commented Nov 7, 2017

bors commented Nov 7, 2017

bors commented Nov 8, 2017

bors commented Nov 8, 2017

kennytm commented Nov 8, 2017

michaelwoerister commented Nov 8, 2017

bors commented Nov 8, 2017

bors commented Nov 8, 2017

bors commented Nov 8, 2017

michaelwoerister commented Nov 8, 2017

Only instantiate inline- and const-fns if they are referenced (again). #45575

Only instantiate inline- and const-fns if they are referenced (again). #45575

Conversation

michaelwoerister commented Oct 27, 2017 • edited Loading

UPDATE & PSA

alexcrichton commented Oct 27, 2017

kennytm commented Oct 27, 2017

alexcrichton commented Oct 31, 2017

eddyb commented Oct 31, 2017 • edited Loading

michaelwoerister commented Nov 1, 2017

arielb1 Nov 2, 2017

Choose a reason for hiding this comment

nikomatsakis Nov 2, 2017

Choose a reason for hiding this comment

arielb1 Nov 2, 2017

Choose a reason for hiding this comment

arielb1 commented Nov 2, 2017 • edited Loading

michaelwoerister commented Nov 3, 2017

bors commented Nov 3, 2017

bors commented Nov 4, 2017

bors commented Nov 4, 2017

kennytm commented Nov 4, 2017

michaelwoerister commented Nov 7, 2017

alexcrichton commented Nov 7, 2017

michaelwoerister commented Nov 7, 2017

michaelwoerister commented Nov 7, 2017

alexcrichton commented Nov 7, 2017

michaelwoerister commented Nov 7, 2017

michaelwoerister commented Nov 7, 2017

bors commented Nov 7, 2017

bors commented Nov 8, 2017

bors commented Nov 8, 2017

kennytm commented Nov 8, 2017

michaelwoerister commented Nov 8, 2017

bors commented Nov 8, 2017

bors commented Nov 8, 2017

bors commented Nov 8, 2017

michaelwoerister commented Nov 8, 2017

michaelwoerister commented Oct 27, 2017 •

edited

Loading

eddyb commented Oct 31, 2017 •

edited

Loading

arielb1 commented Nov 2, 2017 •

edited

Loading