[PATCH] D116179: [InstrProf][NFC] Do not assume size of counter type

[Ellis Hoag via Phabricator via llvm-commits]

ellis marked an inline comment as done.
ellis added inline comments.


================
Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:159
     for (unsigned I = 0; I < NC; I++)
-      DstCounters[I] += SrcCounters[I];
+      ((uint64_t *)DstCounters)[I] += ((uint64_t *)SrcCounters)[I];
 
----------------
kyulee wrote:
> This also assumes the counter type is a 64 bit integer. Should we just use the counter element size like `NC * __llvm_profile_counter_entry_size()` for the loop bound?
> It appears this is actually just a memcpy, and I guess the compiler will emit it for you.
Yes it does assume 64 bits, but that is the only mode when this diff lands. In the next diff the behavior changes based on the mode.

Since these values are being accumulated, I'm not sure it could be just a memcpy?


================
Comment at: llvm/test/tools/llvm-profdata/malformed-ptr-to-counter-array.test:21
 RUN: printf '\10\0\0\0\0\0\0\0' >> %t.profraw
 RUN: printf '\0\0\6\0\1\0\0\0' >> %t.profraw
 RUN: printf '\0\0\6\0\2\0\0\0' >> %t.profraw
----------------
This is the `CountersDelta` value and it was too small for the comment below to be true. Set the least significant byte to 8 to make `CounterOffset = 1`.


================
Comment at: llvm/test/tools/llvm-profdata/malformed-ptr-to-counter-array.test:56
 RUN: not llvm-profdata merge -o /dev/null %t.profraw 2>&1 | FileCheck %s
-CHECK: warning: {{.+}}: malformed instrumentation profile data: number of counters 3 is greater than the maximum number of counters 2
+CHECK: warning: {{.+}}: malformed instrumentation profile data: number of counters 2 is greater than the maximum number of counters 0
 CHECK: error: no profile can be merged
----------------
kyulee wrote:
> Why is the behavior changed?
> My understanding is that now you check # of counters against the maximum # of counters read from the header which appears 0.
> What happens if we fix it in the header as 2 so that # of counters are okay because of type size truncation, but still off by 1?
> 
The error message in the original code is actually talking about the total number of counters. There are two total counters but the reader expected too read a third counter.

The new message is talking about number of counters for a function. This function has two counters, but based on the offset there can only be zero more counters. On second thought this is quite confusing....

For this latest revision, there are really only two things to check for a function's counter offset.
1. Make sure it is positive
2. Make sure this function's last counter is inside the counters section
I've updated the error checking code to reflect this. Let me know what you think and sorry for so many revisions on this part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116179