Stabilize -Zinstrument-mcount

[pmur]

Stabilization report

Summary

Stabilize the boolean -Zinstrument-mcount command line option into -Cinstrument-mcount.
This option is similar to the gcc or clang -pg option to support instrumented profiling. This option
inserts a special function call into each function's prologue in the crate being compiler. The name of
the function is target specific. A user would then link a library which implements this function to
collect profiling data.

A common use is to compile a binary for instrumented profiling using gprof.
ld supports a special option (also called -pg) to link the necessary runtime support for
instrumented profiling.

This option was introduced several years ago, and quietly broke for a period of time due to
refactoring within LLVM, but has since been fixed with an end-to-end test to verify (at least on
x86-64) functions are instrumented.

And all other contributors who have tested, reported, or fixed bugs since 2018.

cc @rust-lang/compiler

What is stabilized

The command line option -Cinstrument-mcount. More generally, the ability to instrument function entry points in way compatible with other widely adopted toolchains (gcc, clang) for instrumenting function calls.

What isn't stabilized

None.

Design

Reference

This feature annotates LLVM IR functions with instrument-function-entry-inlined=target_mcount_name where target_mcount_name is target specific.

RFC history

This option was likely considered too trivial for an RFC.

Answers to unresolved questions

None yet.

Post-RFC changes

None.

Key points

Nightly extensions

There is not part of this feature which will remain unstable.

Doors closed

This option has minimal impact on other options.

Feedback

Call for testing

No. This is a very narrowly scoped option. In the years since this was added there is now sufficient testing to verify the compiler feature
works end-to-end on major targets.

Nightly use

Do any known nightly users use this feature? Counting instances of #![feature(FEATURE_NAME)] on GitHub with grep might be informative.

No. It was suggested this might be useful for Rust For Linux, but it is not yet used.

Implementation

Major parts

• @quark-zju's PR#57220 add -Z instrument-mcount

Coverage

Various PR's over the years have added support for target specific mcount function names which verify the correct function attributes are applied, and several tier 1 targets verify the mcount function is called after LLVM code generation.

• @clubby789's PR#145884 test to verify LLVM actually instruments functions
• @JohnTitor's PR#59506 support for target specific mcount function name
• @ChoKyuWon's PR#113814 fix ARM32 mcount instrinsic

Outstanding bugs

There are no outstanding bugs.

Outstanding FIXMEs

None.

Tool changes

This requires no tooling changes. Though, the utility of this option could be increased by addition cargo support to build instrumented binaries. That would required a stabilized means to build and instrumented std and apply appropriate linker flags. That discussion is out of this scope.

Breaking changes

None.

History and Acknoledgments

The following contributors have implemented, tested, and stabilized this feature:

• @clubby789's PR#145884 test to verify LLVM actually instruments functions
• @JohnTitor's PR#59506 support for target specific mcount function name
• @ChoKyuWon's PR#113814 fix ARM32 mcount instrinsic
• @quark-zju's PR#57220 add -Z instrument-mcount

And any others I have failed to uncover in the long history of this option.

Open items

• The linux kernel also makes use of attributes to inhibit tracing on selected functions. Should a Rust attribute be added to support inhibiting instrumentation? e.g

#[instrument_mcount(no)]
fn foo() {
    // ...
}

• Is there any objection to the name of this option? mcount seems to be a historical artifact from BSD, nor is the instrumentation function named this.
• End-to-end testing should be expanded to all tier1 targets
• There is a lack of documentation about the target specific profiling ABI. How much documentation should Rust provide?
• Are the target specific calls inserted by LLVM sufficiently stable to make this a stable option?

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

⚠️ Warning ⚠️

• This PR is based on an upstream commit that is older than 28 days.

  It's recommended to update your branch according to the rustc-dev-guide.

[Mark-Simulacrum]

A few questions:

• Where does the name mcount come from? Edit: I see you mention this is a historical BSD artifact, but in other toolchains it's not named mcount AFAICT? I'd prefer a more self-contained name personally, e.g., inject-prelude-calls or perhaps -Cinstrument-profiler?
• Is there a reason we want to call a platform-specific function, instead of calling a standard (perhaps Rust-mangled?) name? It doesn't seem obvious that this should be platform specific in general.
  • And if it is, then we ought to have tests on at least all tier 1 targets IMO to verify this behaves reasonably. Right now it looks like only x86_64-unknown-linux-gnu and tier 2 x86_64-apple-darwin are covered based on Test instrument-mcount codegen #145884?
• A user would then link a library which implements this function to collect profiling data. Do we have documentation somewhere for what the ABI of that function is supposed to be?
  • And since getting that ABI wrong is out of our control, does this need to be in our bucket of unsafe codegen options (or have unsafe in the name)?

I think these all reduce to a desire to have the rustc docs either be self-contained or at least link to documentation for how a user might use this without needing to dig through LLVM.

[pmur]

• Where does the name mcount come from? Edit: I see you mention this is a historical BSD artifact, but in other toolchains it's not named mcount AFAICT? I'd prefer a more self-contained name personally, e.g., inject-prelude-calls or perhaps -Cinstrument-profiler?

Yes, I think this is named after the BSD profiler function. Opening the door for a more generic option, I think we want to consider where similar derivatives fit in, like gcc/clang's -mfentry for x86 and s390, or -mprofile-kernel (powerpc).

As for naming, I think it is tied to the effective[1] ABI of the target for interoperability. The calling conventions are typically non-standard and handled by the codegen backend, but there are enough moving parts that it cannot be changed on a whim.

As for documenting the above, arm is the only target which documents this interface. I could try to document the tier1 and some tier2 targets, and provide hints on how to determine this.

[1] The ABI presumably introduced by gcc/gprof, defined officially (e.g arm's __gnu_mcount_nc), or otherwise modified by target specific compiler options (powerpc's -mprofile-kernel)

[workingjubilee]

@Zalathar Do you recognize this instrumentation option? Can you weigh in on its merit?

I recall you removed -Zprofile in #131829 because the gcov support complicated our support of LLVM's similar instrumentation, but while my understanding is that gcov and llvm-cov are themselves distinct (even if functionally similar from the "end user" perspective), I'm less sure about how gprof and llvm's profdata tooling relate. Or fail to, for that matter.

Call for testing

No. This is a very narrowly scoped option. In the years since this was added there is now sufficient testing to verify the compiler feature works end-to-end on major targets.

Nightly use

...
No. It was suggested this might be useful for Rust For Linux, but it is not yet used.

These two as mere assertions don't fill me with confidence. The original PR added zero tests and we have only seen two tests added since? And we only have both of those tests for x86-64 targets, not even AArch64 also? And they are codegen-only, not functional?

If there was a big outsider usage, that would add weight, but I frankly cannot square that knowledge with

there is now sufficient testing to verify the compiler feature works end-to-end on major targets.

"Major" targets is not well-defined. We could assume "all tier 1 targets" at least. I believe our testing does not adequately cover that.

[Zalathar]

@Zalathar Do you recognize this instrumentation option? Can you weigh in on its merit?

It doesn't ring any bells; this might be the first time I've seen it.

At a glance, this looks like the sort of long-neglected flag that is already on thin ice as an unstable feature.

[petrochenkov]

I'm not sure what to do with this, I don't want to start an FCP given the concerns above.
Let's start with nominating for the compiler team.
Upd: later, github has issues.

[pmur]

This is not ready. I was interested in this from the kernel usage point of view. Sorry for the noise, but thank you all for the feedback. I think there is still value for an option to instrument functions, I'll create an RFC.

As for the named purpose, this seems broken for building a self-contained Rust binary. For some (all?) targets, a different set of crt libraries need linked. On glibc, you need to replace linking the crt crt1.* with gcrt1.*. Is there a friendly way to do this today?

[petrochenkov]

Okay, marking as waiting on author then.
@rustbot author

For some (all?) targets, a different set of crt libraries need linked.

See compiler\rustc_target\src\spec\crt_objects.rs and how the functions from it are used.
gcrt1.* will require something similar.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[pmur]

This is not ready. I was interested in this from the kernel usage point of view. Sorry for the noise, but thank you all for the feedback. I think there is still value for an option to instrument functions, I'll create an RFC.

I have created an RFC to have this discussion.