Build instruction profiler runtime as part of compiler-rt #38608
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
One thing I'm not sure about is the old buildsystem. This PR only updates rustbuild. I haven't even looked into enabling this with the old buildsystem. |
|
See https://users.rust-lang.org/t/howto-generating-a-branch-coverage-report/8524 for what can be done with stock rustc after this is merged. |
|
@est31 Not all the intrinsics have been ported to Rust so the compiler-builtins crate still compiles some C code to fill the gaps. We could do the same here: continue compiling the C files added here and, in the future, port them to Rust. |
|
Thanks for the PR! I've long wanted to integrate the various runtimes of compiler-rt into rustc in a more first-class fashion. I've been particularly interested in sanitizers like ASAN and TSAN historically for unsafe code, but cross-platform profiling would also be awesome! I think, though, that we may want to keep this library separate from the standard compiler-builtins library. That is, I think we'd want to ensure that you can pull in compiler-bulitins without the profiling runtime, and then the profiling runtime is only linked in when necessary. There's been some efforts to do this in the past but unfortunately haven't gotten far off the ground. I'd prefer to not have us on the hook for implementing these intrinsics in Rust, as well. Although compiler-rt has a very stable ABI and set of symbols, I suspect that the profiling runtime probably doesn't? I wouldn't want to have to track upstream LLVM changes whenever we update LLVM :( |
Why is this needed at all?
I think you will have to, unless you're fine with rustc being stuck with an inferior profiler. |
|
Yes |
This is something I would normally get very much behind, but...
... the main reason why I so eagerly submitted this PR is that it is my understanding is that the compiler-rt profiling runtine works on any platform with libc. If I am proven wrong with regards to this then we should definitely go your way. |
|
That said--would you like me to separate it into perhaps a |
Do I understand you correctly here? In other words, is the problem that...
I don't see how this is not true right now. For example, someone can reference |
|
Yes I believe the theory is that compiler-rt and all various libraries in that project work on all platforms, but in reality that's been very far from the truth. Just getting the libraries to build is a nightmare, much less actually working correctly. Until it's actually tested, I'd hazard a guess that it only works well on x86_64 linux, and probably OSX as well. I think that there's basically an open design space here. I do think that we'd want a separate library like profiler-builtins but exactly how you request that from the compiler is something I'm not so sure about. This is likely to be a platform-specific tool and the compiler needs to cope with that somehow as well. I haven't personally thought through all of the design questions here, but you can find some prior art on old PRs.
We can't remove symbols like |
I am confused as to how you expect the profiler runtime to function. Its job is to increment a counter then load a file, update a field and save it back. What exactly in it is so platform-specific? LLVM doesn't attempt to be compatible with any existing tools, apparently not even gcov, so you are supposed to use it with llvm-cov only, which uses the same representation on any platform... |
|
I see you reference sanitizers; the profiler runtime of course has a huge difference from sanitizers, which indeed have to use many platform-specific tricks to run. |
|
I am not privvy to the internals of LLVM profiling runtime, I have just had historical experience that "cross platform libraries" like compiler-rt are anything but. If it's not platform-specific then that's great! Again though my preference is to not have this come by default in compiler-rt. At the very least using this should require some form of feature gate as it's entering as unstable, and then we can flesh out bugs and exactly how it works before stabilizing. I feel that if we want to land this we also want documentation one way or another. From this PR I don't even know how to make use of the support added. |
Ack. How about this plan:
I wrote a fairly complete guide for this in https://users.rust-lang.org/t/howto-generating-a-branch-coverage-report/8524. I'll of course introduce it into the Rust documentation if someone points me to where it should live. |
|
Yeah that sounds like a good approach to me, modulo probably -Z instead of -C. We can add documentation either in-tree, in tests, or in the nightly portion of the book. It just needs to go somewhere. |
|
Why |
|
-Z == unstable |
|
ping @whitequark, any update on moving these to unstable features? |
|
Will update soon. I've changed my overall plans somewhat as LLVM's built-in profiler has a much better output than gcov+lcov and is portable, but I don't yet know how it works. |
|
ping @whitequark, any updates with your new plans? |
|
@alexcrichton How do I add libprofiler_builtins in the build system? Looks like everything else is a dependency of libstd, but we're specifically trying to avoid that. |
|
I've found this in /// The various "modes" of invoking Cargo.
///
/// These entries currently correspond to the various output directories of the
/// build system, with each mod generating output in a different directory.
#[derive(Clone, Copy)]
pub enum Mode {
/// This cargo is going to build the standard library, placing output in the
/// "stageN-std" directory.
Libstd,
/// This cargo is going to build libtest, placing output in the
/// "stageN-test" directory.
Libtest,
/// This cargo is going to build librustc and compiler libraries, placing
/// output in the "stageN-rustc" directory.
Librustc,
/// This cargo is going to some build tool, placing output in the
/// "stageN-tools" directory.
Tool,
}but none of the modes seem fit. Should I just add a new one? |
|
Ideally it'd be in a crate rather than the build system, but is it not possible to do so? |
|
@alexcrichton I'm confused. Do you mean it would be in a crate completely external to rustc? |
|
Ideally, yeah, it'd follow the pattern of compiler-builtins, the sanitizer runtimes, panic runtimes, allocators, etc. If necessary the compiler can have a small amount of logic, but all runtime support goes into a dedicated crate. |
Okay. This can only be done with LLVM 4.0+ but since @alexcrichton says it's fine I can do that. Bikeshed: is |
When -Z profile is passed, the GCDAProfiling LLVM pass is added to the pipeline, which uses debug information to instrument the IR. After compiling with -Z profile, the $(OUT_DIR)/$(CRATE_NAME).gcno file is created, containing initial profiling information. After running the program built, the $(OUT_DIR)/$(CRATE_NAME).gcda file is created, containing branch counters. The created *.gcno and *.gcda files can be processed using the "llvm-cov gcov" and "lcov" tools. The profiling data LLVM generates does not faithfully follow the GCC's format for *.gcno and *.gcda files, and so it will probably not work with other tools (such as gcov itself) that consume these files.
|
Sounds reasonable to me! |
alexcrichton
added
S-waiting-on-author
Actually, I've noticed the files generated by LLVM have a different version than the files generated by GCC. So I can also use the version to identify them. |
|
Looks like there was a legitimate test failure? My brain isn't pattern-matching it to one of the spurious failures, at least: @whitequark is this a real failure? |
|
@carols10cents Yes; I haven't gated the test on the profiler being enabled. I'll fix that once I get around to the rest of the issues. |
|
☔ The latest upstream changes (presumably #41709) made this pull request unmergeable. Please resolve the merge conflicts. |
|
friendly ping @whitequark, just want to keep this on your radar! |
|
@carols10cents I'm ill and temporarily unable to work. I'll go back to this PR right once I clear the higher priority parts of my backlog. |
Mark-Simulacrum
added
T-dev-tools
|
Ok this has been inactive for a bit so I'm going to close to help clear out the queue, but @whitequark please feel free to resubmit if you get the chance! (hope you're feeling better!) |
|
@alexcrichton - do we have a tracking bug for this work that I can link to? It's one of the Stylo asks. |
|
@alexcrichton - thanks! |
Build instruction profiler runtime as part of compiler-rt r? @alexcrichton This is #38608 with some fixes. Still missing: - [x] testing with profiler enabled on some builders (on which ones? Should I add the option to some of the already existing configurations, or create a new configuration?); - [x] enabling distribution (on which builders?); - [x] documentation.
Build instruction profiler runtime as part of compiler-rt r? @alexcrichton This is #38608 with some fixes. Still missing: - [x] testing with profiler enabled on some builders (on which ones? Should I add the option to some of the already existing configurations, or create a new configuration?); - [x] enabling distribution (on which builders?); - [x] documentation.
Build instruction profiler runtime as part of compiler-rt r? @alexcrichton This is #38608 with some fixes. Still missing: - [x] testing with profiler enabled on some builders (on which ones? Should I add the option to some of the already existing configurations, or create a new configuration?); - [x] enabling distribution (on which builders?); - [x] documentation.
This is the only change that is needed to the core compiler in order to produce (mostly) usable branch coverage information. This PR will address rust-lang/rfcs#646 (note that the generated information is not really compatible with gcov, one has to use llvm-cov to work with it).
I believe that it is safe to unconditionally enable the profiler builtins in compiler-rt as they are written to work in any hosted environment--MSVC/Win32, GNU/Win32, Apple, Linux, FreeBSD, plus a catch-all for anything else based on POSIX.
The
InstrProfilingPlatform{Darwin,Linux,Other}.cfiles are unconditionally built because they define away the entire contents on platforms other than the supported one.