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

-Z call-metadata: add call metadata to LLVM IR #59777

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@japaric
Copy link
Member

japaric commented Apr 7, 2019

This PR implements eRFC #59412 in spirit but there are some
differences between the implementation and the original proposal.

What

Here's a summary of what the -Z call-metadata feature does:

  1. Each function that:
  • may be used as a function pointer,
  • it's a method of a trait that may be dispatched dynamically,
  • or may be called by trait object drop glue

receives a single !rust metadata node when translated into LLVM IR (turns
out you can't attach several !rust metadata nodes as I originally thought).
The node describes in which of those categories the function falls into (it may
belong to more than one category).

As an example consider this piece of code:

fn foo() -> i32 {
    0
}

trait Bar<T> {
    fn bar(&self) -> T;
}

struct Baz;

impl Bar<bool> for Baz {
    fn bar(&self) -> bool {
        true
    }
}

impl Bar<i32> for Baz {
    fn bar(&self) -> i32 {
        0
    }
}

struct Quux;

impl Bar<i32> for Quux {
    fn bar(&self) -> i32 {
        1
    }
}

fn main() {
    let x: fn() -> i32 = foo;
    x();
    let y: fn(&Baz) -> bool = Baz::bar;

    let z: Box<dyn Bar<bool>> = Box::new(Baz);
    let a = z.bar();
    drop(z);
    let mut w: &dyn Bar<i32> = &Quux;
    w = &Baz;
    let b = w.bar();
}

Using the -Z call-metadata flag produces the following (unoptimized) IR:

NOTE mangled names and debug metadata have been omitted to keep the IR short

; core::ptr::real_drop_in_place
define internal void @_(%Baz* nonnull align 1) unnamed_addr #4 !rust !180 {
  ; ..
}

; core::ptr::real_drop_in_place
define internal void @_(%Quux* nonnull align 1) unnamed_addr #4 !rust !190 {
  ; ..
}

; hello::foo
define internal i32 @_() unnamed_addr #4 !rust !363 {
  ; ..
}

; <hello::Baz as hello::Bar<bool>>::bar
define internal zeroext i1 @_(%Baz* noalias nonnull readonly align 1) unnamed_addr #4 !rust !370 {
  ; ..
}

; <hello::Baz as hello::Bar<i32>>::bar
define internal i32 @_(%Baz* noalias nonnull readonly align 1) unnamed_addr #4 !rust !377 {
  ; ..
}

; <hello::Quux as hello::Bar<i32>>::bar
define internal i32 @_(%Quux* noalias nonnull readonly align 1) unnamed_addr #4 !rust !377 {
  ; ..
}

!180 = !{!"drop", !"Bar<i32>", !"Bar<bool>"}
!190 = !{!"drop", !"Bar<i32>"}
!363 = !{!"fn", !"fn() -> i32"}
!370 = !{!"dyn", !"Bar<bool>", !"bar", !"fn", !"fn(&Baz) -> bool"}
!377 = !{!"dyn", !"Bar<i32>", !"bar"}

Functions that may be called via a function pointer receive metadata of the
form: !"fn" !"fn() -> i32", where the second node is the signature of the
function. Example: !363

Trait methods that may be called via dynamic dispatch receive metadata of the
form: !"dyn" !"Bar<bool>" !"bar", where the second node is a concrete trait
and the third node is the name of the method. Example: !377

Note that a single function can receive both "fn" and "dyn" metadata as
it's the case of <Baz as Bar<i32>::bar in the example above (!370).

Destructors that may be invoked by trait object drop glue receive metadata of
the form: !"drop" !"Bar<i32>", where the second node is a concrete trait.
Example: !180

A single destructor may be invoked by different trait objects (because a type
can implement several traits); in that case the "drop" metadata will contain
one node for each trait, as it's the case of Baz's destructor in the above
example (!190).

  1. Indirect function calls will receive metadata at call site if they belong to
    any of these groups:
  • Function pointer calls,
  • Dynamic dispatch, or
  • Trait object drop glue

Using our previous example, this is the part of the IR that shows the call site
metadata added by the -Z call-metadata flag:

; hello::main
define internal void @() unnamed_addr #0 {
  ; ..

  ; `x()`
  %1 = call i32 %0(), !rust !363

  ; ..

  ; `let a = z.bar();`
  %14 = invoke zeroext i1 %13({}* align 1 %8)
          to label %bb4 unwind label %cleanup, !db!rust !430

  ; ..

  ; `let b = w.bar();`
  %37 = invoke i32 %36({}* align 1 %31)
          to label %bb6 unwind label %cleanup, !rust !377

  ; ..

  ; `drop(z)`
  call void @_ZN4core3ptr18real_drop_in_place17h9dde6f5cdf273fbdE(..) #26

  ; ..
}

; this is `Box<dyn Bar<bool>>`'s destructor
; core::ptr::real_drop_in_place
define internal void @_ZN4core3ptr18real_drop_in_place17h9dde6f5cdf273fbdE(..) {
  ; ..

  ; `drop_in_place::<dyn Bar<bool>>(_)`
  invoke void %8({}* align 1 %3)
          to label %bb3 unwind label %cleanup, !rust !211

  ; ..
}

!211 = !{!"drop", !"Bar<bool>"}
!363 = !{!"fn", !"fn() -> i32"}
!377 = !{!"dyn", !"Bar<i32>", !"bar"}
!430 = !{!"dyn", !"Bar<bool>", !"bar"}

A function invocation may only belong to a single category. The syntax used for
call site metadata is as follows:

  • !"fn" "fn() -> i32" for function pointer calls. The second node is the
    signature of the function being invoked. Example: !363

  • !"dyn" !"Bar<bool>" !"bar" for dynamic dispatch. The second node is a
    concrete trait and the third node is the name of the method being dispatched.
    Examples: !377 and !430

  • !"drop" !"Bar" for drop glue. The second node is a concrete trait. In
    the case of call site "drop" metadata only a single trait will listed. Example: !211

How

And here's a high level description of how the flag is implemented.

In the MIR monomorphize collector pass we take note of:

  • Functions converted into function pointers. These occur in two places: in
    non-const context (e.g. let x: fn() = foo), and in const-context (static X: fn() = foo). These hit two different code paths in the compiler (while
    walking the MIR of functions and when looking into MIR values of static variables) but in
    both cases we keep track of which Instances (rustc type used to track
    concrete functions) get converted into a function pointer in a FxHashSet.

  • Traits that can get converted into trait objects. Again these occur in two
    places: non-const context (e.g. let x: &dyn Foo = &Bar) and in const-context
    (static X: &'static dyn Foo = &Bar). As in the previous bullet these hit two
    different code paths in the compiler but in both cases we keep track of all
    methods (Instances) that belong to these traits
    in a FxHashSet.

  • Drop glue of trait objects. While performing bullet 2 we also note down the
    types that implement traits that get converted into trait objects. In this
    case we store in the information in a FxHashMap that goes from the
    implementer destructor (the Instance e.g. real_drop_inplace(&mut Bar))
    to the set of concrete traits (ExistentialTraitRefs) implemented by the
    implementer type (only the ones that get converted into trait objects).

(Now I realize that for the second bullet it would use less memory to keep
a set of ExistentialTraitRefs (traits) instead of a set of Instances (trait
methods)).

Then in the LLVM codegen pass:

  • When declaring functions (lowering them to define items in LLVM IR) we
    attach "define"-level metadata if the function (Instance) is in one of the
    sets / maps we previously collected. The functions get either "drop"
    metadata, a mixture of "dyn" and "fn" metadata or no !rust metadata.

  • When lowering function invocations to call / invoke instructions we look up
    the callee (Instance) in the sets / maps we previously collected and if
    there's a match we add either "fn", "dyn" or "drop" metadata.

TODO / questions

This doesn't fully work cross crate. Call site metadata is always complete, even
if the function being invoked comes from a different crate; and in the case of
LTO, invocations in functions defined in dependencies will also get call site
metadata. The "define"-level metadata, however, is incomplete; only functions
defined (or monomorphize) in the top crate get this kind of metadata.

To get complete "define"-level metadata I think we would have to store the call
metadata ("is this function converted into function pointer?", "is this trait
converted into a trait object?", etc.) in the rlib metadata (like we do for
types and traits). However, that would mean either (a) always building and
storing the call metadata, which would impact everyone's compile times and
increase rlib size, or (b) only storing the metadata when the -Z call-metadata
flag is used, which would worsen end-users' experience as they would need to
recompile core / std to get full information. If we do (b) we could compile
the std facade with -Z call-metadata so end users don't need to recompile it
themselves (which requires a tool like Xargo).

My gut feeling (and hope) is that always building and storing the extra maps /
sets won't have much impact on compile time given that it's the existing MIR
pass plus a few extra trait lookups and map/set insertions per indirect function
call. So perhaps we can always build and store the call metadata?

r? @eddyb or @oli-obk

japaric added some commits Apr 7, 2019

group metadata nodes in tuples
!1 = !{!"dyn", !"Trait", !"method", !"fn", "fn(&Type) -> bool"}

becomes

!1 = !{!2, !3}
!2 = !{!"dyn", !"Trait", !"method"}
!3 = !{!"fn", !"fn(&Type) -> bool"}

and

!1 = !{!"drop", !"Trait1", !"drop", !"Trait2", !"drop", !"Trait3"}

becomes

!1 = !{!2, !3, !4}
!2 = !{!"drop", !"Trait1"}
!3 = !{!"drop", !"Trait2"}
!4 = !{!"drop", !"Trait3"}

@japaric japaric force-pushed the japaric:call-metadata branch from 10e52a6 to cca6108 Apr 7, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Apr 7, 2019

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:1a2699b8:start=1554674655326883541,finish=1554674657473277304,duration=2146393763
$ 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
Setting environment variables from .travis.yml
---

[00:04:00] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:00] tidy error: /checkout/src/librustc_codegen_ssa/mir/block.rs:335: line longer than 100 chars
[00:04:00] tidy error: /checkout/src/librustc/query/mod.rs:896: line longer than 100 chars
[00:04:02] some tidy checks failed
[00:04:02] 
[00:04:02] 
[00:04:02] 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:02] 
[00:04:02] 
[00:04:02] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:02] Build completed unsuccessfully in 0:00:46
[00:04:02] Build completed unsuccessfully in 0:00:46
[00:04:02] make: *** [tidy] Error 1
[00:04:02] Makefile:67: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1146bcc7
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Apr  7 22:08:29 UTC 2019
---
travis_time:end:095a71bc:start=1554674910437990162,finish=1554674910444834247,duration=6844085
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:2a3de7c6
$ 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:13b5feb4
travis_time:start:13b5feb4
$ 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:00844cac
$ 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)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Apr 8, 2019

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:00575f98:start=1554678670603606213,finish=1554678672912711043,duration=2309104830
$ 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
Setting environment variables from .travis.yml
---
[01:13:15] .................................................................................................... 1000/2961
[01:13:30] .................................................................................................... 1100/2961
[01:13:40] .................................................................................................... 1200/2961
[01:13:52] .................................................................................................... 1300/2961
[01:14:05] .....F..................F........................................................................... 1400/2961
[01:14:28] .................................................................................i.................. 1600/2961
[01:14:43] .................................................................................................... 1700/2961
[01:14:58] .................................................................................................... 1800/2961
[01:15:09] .................................................................................................... 1900/2961
---
[01:16:29] .................................................................................................... 2300/2961
[01:16:48] .............................................ii..................................................... 2400/2961
[01:17:04] .................................................................................................... 2500/2961
[01:17:31] .................................................................................................... 2600/2961
[01:17:50] .......................................F............................................................ 2700/2961
[01:18:13] .................................................................................................... 2900/2961
[01:18:23] .............................................................
[01:18:23] failures:
[01:18:23] 
[01:18:23] 
[01:18:23] ---- [run-pass] run-pass/issues/issue-25515.rs stdout ----
[01:18:23] 
[01:18:23] error: test compilation failed although it shouldn't!
[01:18:23] status: exit code: 101
[01:18:23] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/issues/issue-25515.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/issues/issue-25515/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/issues/issue-25515/auxiliary"
[01:18:23] ------------------------------------------
[01:18:23] 
[01:18:23] ------------------------------------------
[01:18:23] stderr:
[01:18:23] stderr:
[01:18:23] ------------------------------------------
[01:18:23] {"message":"src/librustc_codegen_ssa/mir/block.rs:336: trait object has no principal","code":null,"level":"error: internal compiler error","spans":[],"children":[],"rendered":"error: internal compiler error: src/librustc_codegen_ssa/mir/block.rs:336: trait object has no principal\n\n"}
[01:18:23] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:18:23] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
[01:18:23] 
[01:18:23] note: the compiler unexpectedly panicked. this is a bug.
[01:18:23] note: the compiler unexpectedly panicked. this is a bug.
[01:18:23] 
[01:18:23] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[01:18:23] 
[01:18:23] note: rustc 1.35.0-dev running on x86_64-unknown-linux-gnu
[01:18:23] 
[01:18:23] note: compiler flags: -Z threads=1 -Z ui-testing -Z unstable-options -C prefer-dynamic -C rpath
[01:18:23] 
[01:18:23] ------------------------------------------
[01:18:23] 
[01:18:23] thread '[run-pass] run-pass/issues/issue-25515.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3425:9
[01:18:23] thread '[run-pass] run-pass/issues/issue-25515.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3425:9
[01:18:23] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:18:23] 
[01:18:23] ---- [run-pass] run-pass/issues/issue-26709.rs stdout ----
[01:18:23] 
[01:18:23] error: test compilation failed although it shouldn't!
[01:18:23] status: exit code: 101
[01:18:23] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/issues/issue-26709.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/issues/issue-26709/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/issues/issue-26709/auxiliary"
[01:18:23] ------------------------------------------
[01:18:23] 
[01:18:23] ------------------------------------------
[01:18:23] stderr:
[01:18:23] stderr:
[01:18:23] ------------------------------------------
[01:18:23] {"message":"src/librustc_codegen_ssa/mir/block.rs:336: trait object has no principal","code":null,"level":"error: internal compiler error","spans":[],"children":[],"rendered":"error: internal compiler error: src/librustc_codegen_ssa/mir/block.rs:336: trait object has no principal\n\n"}
[01:18:23] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:18:23] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:18:23] thread '<unnamed>' panicked at 'Metadata module not compiled?', src/libcore/option.rs:1034:5
[01:18:23] 
[01:18:23] note: the compiler unexpectedly panicked. this is a bug.
[01:18:23] 
[01:18:23] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[01:18:23] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[01:18:23] 
[01:18:23] note: rustc 1.35.0-dev running on x86_64-unknown-linux-gnu
[01:18:23] 
[01:18:23] note: compiler flags: -Z threads=1 -Z ui-testing -Z unstable-options -C prefer-dynamic -C rpath
[01:18:23] 
[01:18:23] ------------------------------------------
[01:18:23] 
[01:18:23] thread '[run-pass] run-pass/issues/issue-26709.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3425:9
[01:18:23] thread '[run-pass] run-pass/issues/issue-26709.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3425:9
[01:18:23] 
[01:18:23] ---- [run-pass] run-pass/traits/principal-less-trait-objects.rs stdout ----
[01:18:23] 
[01:18:23] error: test compilation failed although it shouldn't!
[01:18:23] status: exit code: 101
[01:18:23] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/traits/principal-less-trait-objects.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/traits/principal-less-trait-objects/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/traits/principal-less-trait-objects/auxiliary"
[01:18:23] ------------------------------------------
[01:18:23] 
[01:18:23] ------------------------------------------
[01:18:23] stderr:
[01:18:23] stderr:
[01:18:23] ------------------------------------------
[01:18:23] {"message":"src/librustc_codegen_ssa/mir/block.rs:336: trait object has no principal","code":null,"level":"error: internal compiler error","spans":[],"children":[],"rendered":"error: internal compiler error: src/librustc_codegen_ssa/mir/block.rs:336: trait object has no principal\n\n"}
[01:18:23] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:18:23] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:18:23] thread '<unnamed>' panicked at 'Metadata module not compiled?', src/libcore/option.rs:1034:5
[01:18:23] 
[01:18:23] note: the compiler unexpectedly panicked. this is a bug.
[01:18:23] 
[01:18:23] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[01:18:23] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[01:18:23] 
[01:18:23] note: rustc 1.35.0-dev running on x86_64-unknown-linux-gnu
[01:18:23] 
[01:18:23] note: compiler flags: -Z threads=1 -Z ui-testing -Z unstable-options -C prefer-dynamic -C rpath
[01:18:23] 
[01:18:23] ------------------------------------------
[01:18:23] 
[01:18:23] thread '[run-pass] run-pass/traits/principal-less-trait-objects.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3425:9
---
[01:18:23] 
[01:18:23] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:516:22
[01:18:23] 
[01:18:23] 
[01:18:23] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:18:23] 
[01:18:23] 
[01:18:23] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:18:23] Build completed unsuccessfully in 0:12:11
[01:18:23] Build completed unsuccessfully in 0:12:11
[01:18:23] make: *** [check] Error 1
[01:18:23] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0ebcda7c
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Mon Apr  8 00:29:47 UTC 2019

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)

@oli-obk
Copy link
Contributor

oli-obk left a comment

If you unconditionally enable the flag, we can test it out on perfbot.

If we do (b) we could compile
the std facade with -Z call-metadata so end users don't need to recompile it
themselves (which requires a tool like Xargo).

We should try this out, both compilation speed of libstd as well as the dist size. Although I'm not sure what an ok regression would be

query collect_and_partition_mono_items(_: CrateNum)
-> (Arc<DefIdSet>, Arc<Vec<Arc<CodegenUnit<'tcx>>>>) {
query collect_and_partition_mono_items(_: CrateNum) -> (
Arc<DefIdSet>,

This comment has been minimized.

Copy link
@oli-obk

oli-obk Apr 8, 2019

Contributor

This has collected quite the list of output arguments. I think it would be better to use a struct with named fields here

}

AssociatedItemContainer::ImplContainer(impl_def_id) => {
if let Some(trait_ref) = tcx.impl_trait_ref(impl_def_id) {

This comment has been minimized.

Copy link
@oli-obk

oli-obk Apr 8, 2019

Contributor

use the ? operator here

@@ -54,6 +55,38 @@ impl<'a, 'tcx> Instance<'tcx> {
)
}

pub fn trait_ref_and_method(

This comment has been minimized.

Copy link
@oli-obk

oli-obk Apr 8, 2019

Contributor

Please document this method

@@ -282,21 +285,36 @@ impl<'tcx> InliningMap<'tcx> {
}
}

pub fn collect_crate_mono_items<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

This comment has been minimized.

Copy link
@oli-obk

oli-obk Apr 8, 2019

Contributor

I don't like how this file has been affected by the change. It causes some significant readability overhead that I feel is irrelevant to what the collector actually does. Implementation wise I think I'd be fine with it if you created a new type that has all the logic and datastructures in a single place (in another file preferrably). If there are just some calls of the call_metadata.foo() sort flying around this file that seems much better to me.

@@ -122,6 +123,14 @@ pub enum ModuleKind {
Allocator,
}

#[derive(Copy, Clone, Debug, PartialEq)]
pub enum CallKind<'tcx> {

This comment has been minimized.

Copy link
@oli-obk

oli-obk Apr 8, 2019

Contributor

Document this type. It should be clear both here and at the use sites that this is solely for debug info and does not actually affect codegen

dyn_: Option<(ExistentialTraitRef<'tcx>, Symbol)>,
fn_: Option<FnSig<'tcx>>,
llfn: &'ll Value) {
unsafe {

This comment has been minimized.

Copy link
@oli-obk

oli-obk Apr 8, 2019

Contributor

the llvm::* functions here should have safe wrappers on CodegenCx instead of requiring this code to use unsafe so freely.

japaric added some commits Apr 7, 2019

partial fix: separately include the principal trait in `drop_glue`
it wasn't being included in the case it has zero methods

this commit only fixes the non-const case
tweak how "drop" metadata is generated
don't create a tuple if there's a single node (e.g. `!{!1}}`)

don't emit an empty tuple (i.e. `{}`)
@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 11, 2019

We should probably also wait on a change like this: #59412 (comment).

@japaric japaric force-pushed the japaric:call-metadata branch from 3ef7e54 to a0a2544 Apr 16, 2019

@japaric

This comment has been minimized.

Copy link
Member Author

japaric commented Apr 16, 2019

I have added the rustc_metadata encoding / decoding (it's still missing a small
bit) and refactored the implementation according to the review comments but now
I have realized that there's a fundamental problem with the approach of this PR. The
problem is easier to understand with an example so here's one:

Consider these two crates:

// crate: a

pub fn foo() {
    // ..
}
// crate: b

fn main() {
    let f: fn() = a::foo();

    // ..
}

In this example we want to attach "fn" metadata to a::foo the problem is that
we codegen that item when we codegen the crate a (and store the LLVM bitcode
in liba.rlib (*)) but only learn about the "fn" metadata when we analyze
crate b.

(*) For some reason I though that generating LLVM IR / bitcode only happened at
the very end when doing (fat) LTO but now I see that rustc always produces
(some) LLVM bitcode when it compiles a crate and fat LTO is just merging the
LLVM bitcode from all crates.

I'm a bit stumped now. Some ideas:

a. Is it possible to override the define metadata of items defined in other
LLVM bitcode modules from a single LLVM bitcode module (i.e. from the top
crate)?

b. Would it be feasible to add new define metadata after merging all LLVM
bitcode modules into a single one when doing fat LTO? Is there LLVM API
for bitcode manipulation?

c. Discard the idea of define metadata and instead emit all that information
as some separate (JSON?) file. Meaning that tools would need to parse that
file and the callsite metadata in the LLVM IR. This would be a shame because
I think having all the metadata in the LLVM IR is pretty elegant and easy to
work with.

d. Something else?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Apr 16, 2019

d. Something else?

Am I understanding it correctly, that you'll need to know all the call metadata when codegening to llvm? If so, we could delay codegen to the final crate (so basically MIR-only rlibs).

@japaric

This comment has been minimized.

Copy link
Member Author

japaric commented Apr 16, 2019

@oli-obk that's correct.

we could delay codegen to the final crate (so basically MIR-only rlibs).

can we implement that in this PR? Or do you mean to say that this is blocked on MIR-only rlibs?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Apr 16, 2019

Or do you mean to say that this is blocked on MIR-only rlibs?

Only the "delay codegen to the final crate" solution is blocked on MIR-only rlibs. I don't remember what the blockers were. IIRC it was just that the codegen was slower or worse than with the current setup.

I don't know of another way to do this nicely.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 17, 2019

@japaric I think there's a flag you can use on nightly (-Z always-encode-mir maybe?) which has that behavior, but I could be wrong.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Apr 17, 2019

That flag will only encode the MIR for everything. It doesn't change the way llvm is invoked.

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.