[PATCH] D114566: [InstrProf] Add Correlator class to read debug info
Kyungwoo Lee via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Dec 5 11:39:38 PST 2021
kyulee added a subscriber: clayborg.
kyulee added a comment.
Thanks for fixing MachO.
It appears there is still an issue but it seems in dwarflinker (dsymutil). As below with `clang -g -mllvm -disable-vp=true -mllvm -debug-info-correlate=true -fprofile-generate -Oz -mllvm -disable-preinline`, when the function is fully inlined, `dsymutil` seems to optimize this static data in the final dSYM.
I was trying to use `-keep-function-for-static` to keep this, but still has a missing location info (DW_AT_location) at dSYM, and thus we cannot correlate `foo` and thus miss the coverage for it.
But I think missing location info for the function static variables seems a known issue in dsymutil, and we may follow this up. cc @clayborg
static
int foo() {
return 1;
}
int main() {
return foo();
}
================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:36
+/// bundle otherwise return the input path.
+static Expected<std::string> expandBundle(const StringRef InputPath) {
+ std::vector<std::string> BundlePaths;
----------------
Hmm. This code seems repeated 3 times in dwarfdump and gsymutil, and now here.
I think this helper might be moved to some dsym related support file. But certainly this can be followed up.
================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:205
+ const auto &ParentDie = Die.getParent();
+ if (!Die || !ParentDie || Die.isNULL())
+ return false;
----------------
Die or ParentDie are references. Can they be null?
================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:219
+template <class IntPtrT>
+void DwarfInstrProfCorrelator<IntPtrT>::correlateProfileDataImpl() {
+ auto maybeAddProbe = [&](DWARFDie Die) {
----------------
Can you add a high-level (example) dwarf layout for this function to match as the comment?
================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:227
+ // TODO: Warn or fail when the function address is unavailable.
+ uint64_t FunctionPtr =
+ dwarf::toAddress(Die.getParent().find(dwarf::DW_AT_low_pc))
----------------
Can we use `getLowAndHighPC` or make one `getLow`?
I think 0 address (no existent of function address) is a valid case when the function is completely inlined (and thus the body is elided) -- only this static profile data remains.
================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:419
+ return error(instrprof_error::unexpected_debug_info_for_correlation);
+ if (Correlator) {
+ Data = Correlator->getDataPointer();
----------------
Can we set these value for Correlator once instead of overwriting them, which may be out-of-bound or invalid?
I guess DataSize and NameSize etc are 0, and we can assert these values under this flag.
================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:955
};
auto Annotations = DB.getOrCreateArray({
MDNode::get(Ctx, FunctionNameAnnotation),
----------------
I was thinking whether we need another INSTR_PROF_DATA or a similar one for DebugInfoCorrelate in the header and include them in order so that we can ensure sync these data in order in both a writer, here and a reader, correlateProfileDataImpl.
================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:151
+static std::unique_ptr<MemoryBuffer>
+getInputFileBuf(const StringRef &InputFile) {
+ if (InputFile == "")
----------------
Why is this moved? Is this relevant to this change?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114566/new/
https://reviews.llvm.org/D114566
More information about the llvm-commits
mailing list