Add EntryExistInstrumenterPass for -Z instrument-mcount to the pipeline manually for LLVM >= 13.

[luqmana]

Fixes #92109

[rust-highfive]

r? @fee1-dead

(rust-highfive has picked a reviewer for you, use r? to override)

[fee1-dead]

r? @nikic

(I'm not familiar with LLVM)

[nikic]

As we only set instrument-function-entry-inlined, I assume the pre-inline pass is not strictly necessary and just added for the sake of completeness?

[tmiasko]

I don't have opinion whether we should land this for now, but I think the issue is ultimately on the LLVM side: llvm/llvm-project#52853

[luqmana]

As we only set instrument-function-entry-inlined, I assume the pre-inline pass is not strictly necessary and just added for the sake of completeness?

Yea. Mostly trying to keep the same behaviour that was previously handled in LLVM. More-or-less doing the same sorta thing in clang (as per https://reviews.llvm.org/D97608)

[luqmana]

I don't have opinion whether we should land this for now, but I think the issue is ultimately on the LLVM side: llvm/llvm-project#52853

I think that's the slightly different issue of clang producing misleading results. Rust simply stopped inserting the mcount calls at all, which is what this PR was trying to address. Although, reading that it seems like we might as have the same issue.

[tmiasko]

I don't have opinion whether we should land this for now, but I think the issue is ultimately on the LLVM side: llvm/llvm-project#52853

I think that's the slightly different issue of clang producing misleading results. Rust simply stopped inserting the mcount calls at all, which is what this PR was trying to address. Although, reading that it seems like we might as have the same issue.

To my understanding fixing that issue would obviate the need for changes on the Rust side, and obsolete any changes made here, so it might be a preferable direction. Especially that Rust implementation would otherwise have exactly the same problem.

Sorry for just linking the issue without offering an explanation.

[luqmana]

I don't have opinion whether we should land this for now, but I think the issue is ultimately on the LLVM side: llvm/llvm-project#52853

I think that's the slightly different issue of clang producing misleading results. Rust simply stopped inserting the mcount calls at all, which is what this PR was trying to address. Although, reading that it seems like we might as have the same issue.

To my understanding fixing that issue would obviate the need for changes on the Rust side, and obsolete any changes made here, so it might be a preferable direction. Especially that Rust implementation would otherwise have exactly the same problem.

Sorry for just linking the issue without offering an explanation.

Oh no worries. I did think on first read they meant making those changes on the Clang side but if it involves just doing the work from the backend again, that works too. I guess the question then is is it worth getting something working in the meantime since it has been broken for a couple months now.

[pnkfelix]

I think we can and should push ahead with the strategy of adding a -Z flag for the short term to cover this functionality.

But I also think it makes a lot of sense to include a pointer to llvm/llvm-project#52853, either in a comment in the code itself, or in the help text for -Z instrument-mcount, so that people at least have some hint that the results they get may suffer from misleading end results.

r+ from me after you make a change along those lines, @luqmana .

[apiraino]

Switching to waiting on author to incorporate changes. Feel free to request a review when ready, thanks!

@rustbot label -S-waiting-on-review +S-waiting-on-author

[bors]

☔ The latest upstream changes (presumably #99792) made this pull request unmergeable. Please resolve the merge conflicts.

[cuviper]

FYI, #100460 made LLVM 13 the minimum, which should simplify some things here.

[JohnCSimon]

ping from triage:
@luqmana

Can you please post your status on this PR? There are reviewer comments.

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

[mati865]

ping @luqmana

[Dylan-DPC]

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks