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

[Kyungwoo Lee via Phabricator via llvm-commits]

kyulee added inline comments.


================
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()) /
----------------
`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?


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


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:463
+  ptrdiff_t CounterBaseOffset =
+      swap(Data->CounterPtr) - (Correlator ? 0 : CountersDelta);
+  if (CounterBaseOffset < 0)
----------------
`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.


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:470
+  ptrdiff_t MaxCounterOffset = NamesStart - CountersStart;
+  if (!Correlator && CounterBaseOffset > MaxCounterOffset)
+    return error(instrprof_error::malformed,
----------------
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.


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:479
+      CounterBaseOffset + NumCounters * getCounterTypeSize();
+  if (!Correlator && LastCounterBaseOffset > MaxCounterOffset)
+    return error(instrprof_error::malformed,
----------------
I think the same here. `LastCounterBaseOffset >= MaxCounterOffset`?
And also, looking at these two conditions, the latter condition (checking the end address) seems sufficient instead of checking if both the start and end address are out of bound.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116179