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

Instrumentation (-Zinstrument-mcount) broken due to missing LLVM-pass since LLVM13 update #92109

Open
tlambertz opened this issue Dec 19, 2021 · 9 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. P-medium Medium priority regression-untriaged Untriaged performance or correctness regression. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tlambertz
Copy link

Issue

I tried to compile a generic hello-world binary with the -Zinstrument-mcount compiler option.

I expected every function-entry to have a call to mcount().
Instead, there are no such calls inserted.

Triage

I have triaged the bug, and it first occurred in nightly-2021-08-22, the first version with LLVM13.

The LLVM-IR still contains the "instrument-function-entry-inlined"="mcount" attributes, but the corrosponding llvm-pass is not run anymore.

rustc +nightly-2021-08-21 -O -Zinstrument-mcount main.rs --emit asm -Z print-llvm-passes -Z no-parallel-llvm 2>&1 | grep mcount
    Instrument function entry/exit with calls to e.g. mcount() (pre inlining)
      Instrument function entry/exit with calls to e.g. mcount() (post inlining)
rustc +nightly-2021-08-22 -O -Zinstrument-mcount main.rs --emit asm -Z print-llvm-passes -Z no-parallel-llvm 2>&1 | grep mcount
[empty]

Verbose Versions:

❯ rustc +nightly-2021-08-21 --version --verbose
rustc 1.56.0-nightly (a0035916e 2021-08-20)
binary: rustc
commit-hash: a0035916e01d8e644ccd44554c57f0874cef8c8c
commit-date: 2021-08-20
host: x86_64-unknown-linux-gnu
release: 1.56.0-nightly
LLVM version: 12.0.1
❯ rustc +nightly-2021-08-22 --version --verbose
rustc 1.56.0-nightly (d3e2578c3 2021-08-21)
binary: rustc
commit-hash: d3e2578c31688619ddc0a10ddf8543bf4ebcba5b
commit-date: 2021-08-21
host: x86_64-unknown-linux-gnu
release: 1.56.0-nightly
LLVM version: 13.0.0
@tlambertz tlambertz added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Dec 19, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 19, 2021
@tlambertz
Copy link
Author

Manually adding the passes works with all nightly versions I have tested (including the current one):

rustc +nightly -Zinstrument-mcount -C passes="ee-instrument post-inline-ee-instrument" main.rs

For 2021-08-22 it also prints said passes. Sometime in between 09-01 and 10-01 the pass is not printed anymore, not sure why.

❯ rustc +nightly-2021-09-01 -Zinstrument-mcount -C passes="ee-instrument post-inline-ee-instrument" main.rs --emit asm -Z print-llvm-passes -Z no-parallel-llvm 2>&1 | grep mcount
      Instrument function entry/exit with calls to e.g. mcount() (pre inlining)
      Instrument function entry/exit with calls to e.g. mcount() (post inlining)
❯ rustc +nightly-2021-10-01 -Zinstrument-mcount -C passes="ee-instrument post-inline-ee-instrument" main.rs --emit asm -Z print-llvm-passes -Z no-parallel-llvm 2>&1 | grep mcount
[empty]

@tmiasko
Copy link
Contributor

tmiasko commented Dec 22, 2021

The responsibility for hooking in EntryExitInstrumenterPass moved from LLVM to Clang in https://reviews.llvm.org/D97608.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 23, 2021
@inquisitivecrystal inquisitivecrystal added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Jan 7, 2022
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 15, 2022
@mvtec-bergdolll
Copy link

Using RUSTFLAGS="-Z instrument-mcount -C passes=ee-instrument<post-inline>" I noticed that inlined functions may call mcount again, leading to skewed statistics, eg:

image

I'm not sure if clang generates different code, but I would have expected an inlined function to not contribute to the call count of its parent function.

@quark-zju
Copy link
Contributor

quark-zju commented Dec 16, 2022

I'm not sure if clang generates different code, but I would have expected an inlined function to not contribute to the call count of its parent function.

In 31a5066, must_not_eliminate_frame_pointers returns true for mcount to work. It seems (from the linked gprof doc in the comment) keeping the frame pointer is enough to keep mcount work properly. Did you find cases where functions must not be inlined?

EDIT:

Actually from https://reviews.llvm.org/D22666 I think you're right that mcount shouldn't appear in inlined functions. It seems this is an ordering issue of LLVM passes then.

@tmiasko
Copy link
Contributor

tmiasko commented Dec 16, 2022

I reported the inlining / pass ordering issue upstream llvm/llvm-project#52853 (but never got around to fixing it).

@quark-zju
Copy link
Contributor

I reported the inlining / pass ordering issue upstream llvm/llvm-project#52853 (but never got around to fixing it).

Thanks for the context! According to that thread, it seems completely disabling LTO is a valid workaround. I can verify that.

More specifically, If I build my project using:

RUSTCFLAGS='-Zinstrument-mcount -Cpasses=ee-instrument<post-inline> -Clinker-plugin-lto=off -Clto=off'

Then uftrace would be able to trace the program without warnings or crashes. Without the lto flags, uftrace would print lots of warnings and could segfault.

aeubanks pushed a commit to llvm/llvm-project that referenced this issue May 31, 2024
…pelines (#92171)

Move EntryExitInstrumenter(PostInlining=true) to as late as possible and
EntryExitInstrumenter(PostInlining=false) to an early pre-inlining stage
(but skip for ThinLTO post-link).

This should fix the issues reported in
rust-lang/rust#92109 and
#52853. These are caused
by https://reviews.llvm.org/D97608.
@alehander92
Copy link

llvm/llvm-project#92171 this should be fixed now?

@pasko
Copy link

pasko commented Dec 3, 2024

llvm/llvm-project#92171 this should be fixed now?

I believe it should be fixed, but I did not test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. P-medium Medium priority regression-untriaged Untriaged performance or correctness regression. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
9 participants