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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.


================
Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:129
+  // enabled.
+  if (Header->DataSize == 0) {
+    if (!(__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE)) {
----------------
ellis wrote:
> 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.
The [[ https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/profile/InstrProfiling.h#L105-L107 | comment ]] says the caller of this function is responsible for the check. And we [[ https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/profile/InstrProfilingMerge.c#L48 | verifies ]] that fields in both headers math before merging at doProfileMerging.


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

I use in-memory `__llvm_profile_counter_entry_size()` to calculate in-file SrcCountersEnd, because the header only tells number of counters.


================
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/
----------------
ellis wrote:
> We need to run this line twice to correctly test `__llvm_profile_merge_from_buffer()`
Added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157632