[PATCH] D157632: [Profile] Allow online merging with debug info correlation.

[Ellis Hoag via Phabricator via cfe-commits]

ellis added a comment.

Thanks for working on this!



================
Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:103-104
 COMPILER_RT_VISIBILITY
 int __llvm_profile_merge_from_buffer(const char *ProfileData,
                                      uint64_t ProfileSize) {
   __llvm_profile_data *SrcDataStart, *SrcDataEnd, *SrcData, *DstData;
----------------
I just realized that I need to throw an error here for temporal profiles since online merging won't work for that case.


================
Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:129
+  // enabled.
+  if (Header->DataSize == 0) {
+    if (!(__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE)) {
----------------
Since we don't have the data section, we need to be confident that existing profile comes from the same binary (so that the counter section is identical). Can we add some extra checks here? I'm thinking we can verify that some fields in the headers match and that the variant flags are identical.


================
Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:134-136
+    for (SrcCounter = SrcCountersStart,
+        DstCounter = __llvm_profile_begin_counters();
+         SrcCounter < SrcCountersEnd;) {
----------------
Can you add a check to make sure src and dst have the same number of counters (`SrcCountersEnd - SrcCountersStart`)?


================
Comment at: compiler-rt/test/profile/Darwin/instrprof-debug-info-correlate.c:26
+// RUN: %clang_pgogen -o %t -g -mllvm --debug-info-correlate -mllvm --disable-vp=true %S/../Inputs/instrprof-debug-info-correlate-main.cpp %S/../Inputs/instrprof-debug-info-correlate-foo.cpp
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/%m.proflite %run %t
+// RUN: llvm-profdata merge -o %t.profdata --debug-info=%t.dSYM %t.profdir/
----------------
We need to run this line twice to correctly test `__llvm_profile_merge_from_buffer()`


================
Comment at: compiler-rt/test/profile/Linux/instrprof-debug-info-correlate.c:35
+
+// RUN: diff <(llvm-profdata show %t.normal.profdata) <(llvm-profdata show %t.profdata)
+
----------------
I know this is outside the scope of this patch, but I don't think simply `llvm-profdata show` is sufficient to compare profiles. I'll try to fix this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632