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

[Ellis Hoag via Phabricator via llvm-commits]

ellis added a comment.

In D116179#3211807 <https://reviews.llvm.org/D116179#3211807>, @kyulee wrote:

> Given this change must be NFC, but subtle due to pointer arithmetic. I guess you might run more through tests with some large programs with PGO.

Thanks for the review! Instead of running tests on larger programs, I think it's more important to run



================
Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:26
   uint64_t Version = __llvm_profile_get_version();
-  uint64_t CounterSize = (uint64_t)(__llvm_profile_end_counters() -
-                                    __llvm_profile_begin_counters());
+  uint64_t NumCounters = (uint64_t)(__llvm_profile_end_counters() -
+                                    __llvm_profile_begin_counters()) /
----------------
kyulee wrote:
> `CounterSize` means `NumCounters` similar to `DataSize` which should mean `NumData`.
> You rename this for `Counter` not for `Data`. But down the road below, Header still uses `CounterSize` and `DataSize`.
> Should we keep `CounterSize` same as before?
I've decided to go ahead change the diff so that `Num{Counters,Data}` and `{Counters,Data}Size` have the expected meaning.


================
Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:74
+          Header->DataSize * sizeof(__llvm_profile_data) + Header->NamesSize +
+          Header->CountersSize * __llvm_profile_counter_entry_size())
     return 1;
----------------
By the way, this change has different behavior because it's actually a bug fix. `Header->CounterSize` (which is the number of counters) needs to be scaled by the entry size.


================
Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:157
+    if (SrcCounters < SrcCountersStart || SrcCounters >= SrcNameStart ||
+        (SrcCounters + NC) > SrcNameStart)
       return 1;
----------------
kyulee wrote:
> `SrcCounters + NC` may be differently evaluated in between `uint64_t *SrcCounters` vs `char *SrcCounters`. 
Thanks for the catch!


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:463
+  ptrdiff_t CounterBaseOffset =
+      swap(Data->CounterPtr) - (Correlator ? 0 : CountersDelta);
+  if (CounterBaseOffset < 0)
----------------
kyulee wrote:
> `CountersDelta` should be 0 when being read in the header for the Correlator?
> Do we keep this as an invariant? I guess `advance()` above in the main loop may not update this value for this case.
`CountersDelta` is now a constant zero when there is a `Correlator` now that I've changed `advanceData()`.


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:470
+  ptrdiff_t MaxCounterOffset = NamesStart - CountersStart;
+  if (!Correlator && CounterBaseOffset > MaxCounterOffset)
+    return error(instrprof_error::malformed,
----------------
kyulee wrote:
> It's a bit confusing `!Correlator` is repeated in here and below conditions.
> BTW, should it be `CounterBaseOffset >= MaxCounterOffset`? I think `MaxCounterOffset` is not really a valid offset for a counter since it's just the starting offset of the name section that follows.
Thanks, I made a few fixes to this area. I'm now tracking `CountersEnd` so that we can correctly compute `MaxNumCounters` and we no longer care if we have a correlator. Also, the two checks are helpful to make sure `CountersEnd - (CountersStart + CounterBaseOffset)` doesn't underflow


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116179