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

[Kyungwoo Lee via Phabricator via llvm-commits]

kyulee accepted this revision.
kyulee added a comment.
This revision is now accepted and ready to land.

LGTM



================
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];
 
----------------
ellis wrote:
> 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?
Ah. I thought it's a copy not an accumulation. Given the current counter type is 64 bit only, this looks okay.


================
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
----------------
ellis wrote:
> 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.
Yeah. I think in your initial diff I thought/commented this way -- either the start is positive or the last offset is within the the section. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116179