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

[Ellis Hoag via Phabricator via llvm-commits]

ellis marked 8 inline comments as done.
ellis added inline comments.


================
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;
----------------
kyulee wrote:
> 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.
Absolutely! I'll follow up in a separate diff.


================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:205
+  const auto &ParentDie = Die.getParent();
+  if (!Die || !ParentDie || Die.isNULL())
+    return false;
----------------
kyulee wrote:
> Die or ParentDie are references. Can they be null?
`DWARFDie` has a bool operator that just calls `.isValid()`. It's probably safer and more clear to call `.isValid()` directly.


================
Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:219
+template <class IntPtrT>
+void DwarfInstrProfCorrelator<IntPtrT>::correlateProfileDataImpl() {
+  auto maybeAddProbe = [&](DWARFDie Die) {
----------------
kyulee wrote:
> Can you add a high-level (example) dwarf layout for this function to match as the comment?
Good idea. I've added a comment in the header.


================
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))
----------------
kyulee wrote:
> 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.
IIUC `FunctionPtr` is only used for value profiling to match callsite dispatches to their callees. If the function is completely inlined then there won't be value profiling for that callee.

I've changed this so that if we can't find an address then we emit a warning so we can investigate and possible fixup debug info. So for the `.profdata` file, an address of `0` means either "we can't find the address" or "the function is not concrete (it is inlined)".


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:419
+    return error(instrprof_error::unexpected_debug_info_for_correlation);
+  if (Correlator) {
+    Data = Correlator->getDataPointer();
----------------
kyulee wrote:
> 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.
Yeah I've change this up a bit to only set them once. It's confusing because `DataSize` is zero but there is actual data in the `Correlator`.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:955
       };
       auto Annotations = DB.getOrCreateArray({
           MDNode::get(Ctx, FunctionNameAnnotation),
----------------
kyulee wrote:
> 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.
I don't think adding a macro in `InstrProfData.inc` is the right solution because it makes it unnecessary complicated and hard to read. IIUC the worry is that someone may change the writer and forget to change the reader. If this code is changed without the right reader change then the compiler-rt test will break and hopefully the change won't land. I would like to leave this how it is, but I'm open to discussion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114566/new/

https://reviews.llvm.org/D114566