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

LLVM Code Coverage Instrumentation #1088

Open
wants to merge 70 commits into
base: master
Choose a base branch
from

Conversation

corbanvilla
Copy link
Contributor

@corbanvilla corbanvilla commented Feb 10, 2024

Overview

Related Issue

Description

This PR includes all of the necessary code to introduce code coverage instrumentation into Rusty. It's a large PR with still lots of work to be done, but provides a proof of concept and reference point for some necessary discussions.

Code Coverage Instrumentation Process

  1. CodeGen::new(...): Create a CoverageInstrumentationBuilder
  2. CodeGen::generate_llvm_index(...): add increment function to llvm_index.
  3. CodeGen::generate(...):
    • Generate the coverage mapping header in the module.
    • Generate function records in the module, along with Profile-guided optimization (PGO) variables for each function.
      • Even though we don't use PGO in the compilation, these variables are used by the coverage instrumentation pass.
      • This process includes running a DFS algorithm through each function and creating coverage counters and mapping regions throughout control flow.
  4. CodeGen::generate(...) -> PouGenerator::generate_implementation(...):
    • Add a counter increment right before the function body begins to execute.
    • Add counter increments when entering control flow regions.
  5. CodeGen::generate(...): call the LLVM instrumentation coverage pass (instrprof) to run on the IR.
    • From my understanding, this pass needs to run even when the target output is IR (the pass can't be run later by clang, for example). This is because it replaces the increment calls with other instructions, and the increment function calls are not actually callable.

Overview of Changes

  • Add two new sub-packages, derived from the rust compiler:

    • compiler/rustc_llvm: this contains cpp files that include the necessary classes and functions from the LLVM project, through rust-friendly wrappers. This is necessary because many of the code coverage features are not exposed through the LLVM C-API, and have to be called through this.
      • build.rs: taken directly from the rust compiler.
    • compiler/rustc_llvm_coverage: this package contains files and type definitions from the rust compiler, which have been modified to use Inkwell's type system:
      • interface.rs: function interfaces taken from the rust compiler, adapted to Inkwell typing (taken from rustc version 1.64, the last version to support LLVM 14).
      • lib.rs: includes slightly higher-level abstractions for interfacing with Inkwell in a more convenient way.
      • ffi.rs: exposes the rustc_llvm functions and links against the binary produced.
      • types.rs: coverage instrumentation type definitions, consolidated from different parts of the rust compiler.
  • Create new CoverageInstrumentationBuilder (instrument.rs), which is used similar to DebugBuilder.

    • CoverageInstrumentationBuilder tries to encapsulate as much of the coverage instrumentation process as possible, to minimize changes to the Rusty. It's called throughout the code generation process at various stages.
  • Temporarily uses a forked version of Inkwell, which exposes a global value constructor. This was fixed in Inkwell here.

Discussion Points

  • rustc_llvm and rustc_llvm_coverage don't necessarily fit into Rusty, and it would probably make more sense to integrate them into Inkwell, or into an extension of Inkwell (since the functions they reference are kind of undocumented and not exposed in the LLVM C-API). @ghaith would definitely be interested to know your thoughts on this.
  • There is probably a more seamless way of integrating the calls to CoverageInstrumentationBuilder throughout the codegen process.

TODOs

  • Update to the latest version of Rusty.
  • Add command-line flags for optionally adding coverage instrumentation.
  • Resolve in-code TODOs.
  • I also added the AddressSanitizer features into this, though I'll refactor them out to their own PR.
  • Test name mangling issues.
  • Cleanup the build.rs script in llvm_rustc
  • Unit tests for code coverage

Examples

Screenshot 2024-01-31 at 10 51 16 PM
Screenshot 2024-01-31 at 10 50 51 PM

@riederm
Copy link
Collaborator

riederm commented Feb 12, 2024

hey @corbanvilla,

first of all, thx for your contribution. This PR is pretty big, and also the topics are quite new to rusty, so I guess we should discuss further on how we best integrate these things. I'm afraid if we keep such a big PR pending for too long it will diverge more and more.

I'll see if I can talk to the other maintainers soon but I really like that you already suggested some discussion points. ghaith already contributed into inkwell in the past, it should be failry streight forward to decide if this can be suggested to the inkwell project right away. I'll poke ghaith today when I see him.

thx again! we'll keep in touch

@ghaith
Copy link
Collaborator

ghaith commented Feb 12, 2024

Hello @corbanvilla,

Thank you for the PR. I didn't get the chance to look into it in details, but from your comments and first glance I just have a few small points.

  • Regarding the new C++ libs, I think they would fit better as their own projects/crates since they are thematically not RuSTy related.
  • About the possibility of merging them into inkwell, I think it's worth a try but IIRC the developer was focused on implementing a safe wrapper around the C api and did not want C++. This might have changed, or I might have misunderstood.
  • Regarding the code, I would suggest you take a look at the Debug trait and enum to see how we solved calling the debug without having if let blocks everywhere. This might work well for the instrumentation as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants