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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
