This is an archive of the discontinued LLVM Phabricator instance.

[InstrProfiling] No runtime hook for unused funcs
ClosedPublic

Authored by gulfem on Mar 23 2022, 11:22 AM.

Details

Summary

CoverageMappingModuleGen generates a coverage mapping record
even for unused functions with internal linkage, e.g.
static int foo() { return 100; }
Clang frontend eliminates such functions, but InstrProfiling pass
still pulls in profile runtime since there is a coverage record.
Fuchsia uses runtime counter relocation, and pulling in profile
runtime for unused functions causes a linker error:
undefined hidden symbol: __llvm_profile_counter_bias.
Since 389dc94d4be7, we do not hook profile runtime for the binaries
that none of its translation units have been instrumented in Fuchsia.
This patch extends that for the instrumented binaries that
consist of only unused functions.

Diff Detail

Event Timeline

gulfem created this revision.Mar 23 2022, 11:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2022, 11:22 AM
gulfem requested review of this revision.Mar 23 2022, 11:22 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 23 2022, 11:22 AM
phosek accepted this revision.Mar 23 2022, 11:57 PM

LGTM

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
569–573

This could be combined into a single statement.

This revision is now accepted and ready to land.Mar 23 2022, 11:57 PM
gulfem updated this revision to Diff 417945.Mar 24 2022, 8:59 AM

Addressed phosek's comment.

Should this be fixed in Clang (not generating coverage record)? Adding the logic here is a little confusing.

Should this be fixed in Clang (not generating coverage record)? Adding the logic here is a little confusing.

@davidxl,
I originally did that (not generating coverage records) because I think it is a cleaner way.
But, after reading https://reviews.llvm.org/D43794, it's my understanding is that we might be generating coverage record for unused functions for TAPI.
@vsk can clarify that. If there is no such restriction anymore, I can change that.

vsk added a subscriber: cishida.Mar 24 2022, 12:09 PM

Sorry for ay delayed replies - I've switched teams at Apple and find it difficult to keep up with llvm reviews.

it's my understanding is that we might be generating coverage record for unused functions for TAPI.

Coverage function records are emitted for unused functions because the tooling needs to know which file/line ranges require a "0" execution count.
CC'ing @cishida for tapi expertise. My (possibly outdated) understanding is that for tapi, we always emit the runtime hook because it analyzes compilation flags to determine the expected exported symbol set.

Sorry for ay delayed replies - I've switched teams at Apple and find it difficult to keep up with llvm reviews.

it's my understanding is that we might be generating coverage record for unused functions for TAPI.

Coverage function records are emitted for unused functions because the tooling needs to know which file/line ranges require a "0" execution count.

Thanks for the background @vsk. That means that we need to generate a coverage record even for unused functions.
Do you have any objection to the current solution (not pulling in profile runtime for such cases)?

vsk added a comment.Mar 24 2022, 5:03 PM

So long as it doesn't change behavior expected by tapi on Darwin, I think it's OK. I doubt any other platforms are similarly affected.

So long as it doesn't change behavior expected by tapi on Darwin, I think it's OK. I doubt any other platforms are similarly affected.

I don't think it should impact TAPI because it relies on needsRuntimeHookUnconditionally function, which only returns false for Fuchsia.
So, this patch only affects the behavior of pulling in profile runtime for unused functions in Fuchsia.

bool needsRuntimeHookUnconditionally(const Triple &TT) {
  // On Fuchsia, we only need runtime hook if any counters are present.
  if (TT.isOSFuchsia())
    return false;

  return true;
}
This revision was landed with ongoing or failed builds.Mar 25 2022, 10:03 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
569
gulfem added inline comments.Mar 25 2022, 10:28 AM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
569

Thanks for the tip @MaskRay, and I'm going to fix that in a following patch.

davidxl added inline comments.Mar 25 2022, 11:44 AM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
569

It would be helpful to add a comment in the code reflecting the review discussions.