[PATCH] D114566: [InstrProf] Add Correlator class to read debug info

[Kyungwoo Lee via Phabricator via llvm-commits]

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