[profile] Perform pointer arithmetic in uintptr_t

[nikic]

Based on the feedback from #118782, this switches most of the pointer arithmetic in __llvm_profile_merge_from_buffer to work on uintptr_t instead of const char *, only casting back to a pointer when performing actual accesses.

This ensures that all the arithmetic is performed without any assumptions about pointer overflow.

[llvmbot]

@llvm/pr-subscribers-pgo

Author: Nikita Popov (nikic)

Changes

Based on the feedback from #118782, this switches most of the pointer arithmetic in __llvm_profile_merge_from_buffer to work on uintptr_t instead of const char *, only casting back to a pointer when performing actual accesses.

This ensures that all the arithmetic is performed without any assumptions about pointer overflow.

---

Full diff: https://github.com/llvm/llvm-project/pull/118944.diff

1 Files Affected:

• (modified) compiler-rt/lib/profile/InstrProfilingMerge.c (+29-38)

diff --git a/compiler-rt/lib/profile/InstrProfilingMerge.c b/compiler-rt/lib/profile/InstrProfilingMerge.c
index a42fa74d1788eb..92721c4fd55ea2 100644
--- a/compiler-rt/lib/profile/InstrProfilingMerge.c
+++ b/compiler-rt/lib/profile/InstrProfilingMerge.c
@@ -127,12 +127,6 @@ getDistanceFromCounterToValueProf(const __llvm_profile_header *const Header) {
          PaddingBytesAfterVNamesSize;
 }
 
-// Add offset to pointer without assuming that the addition does not overflow.
-// This allows performing bounds checks by checking the result of the addition.
-static const char *ptr_add_with_overflow(const char *p, size_t offset) {
-  return (const char *)((uintptr_t)p + offset);
-}
-
 COMPILER_RT_VISIBILITY
 int __llvm_profile_merge_from_buffer(const char *ProfileData,
                                      uint64_t ProfileSize) {
@@ -143,28 +137,23 @@ int __llvm_profile_merge_from_buffer(const char *ProfileData,
     return 1;
   }
 
-  __llvm_profile_data *SrcDataStart, *SrcDataEnd, *SrcData, *DstData;
   __llvm_profile_header *Header = (__llvm_profile_header *)ProfileData;
-  char *SrcCountersStart, *DstCounter;
-  const char *SrcCountersEnd, *SrcCounter;
-  const char *SrcBitmapStart;
-  const char *SrcNameStart;
-  const char *SrcValueProfDataStart, *SrcValueProfData;
   uintptr_t CountersDelta = Header->CountersDelta;
   uintptr_t BitmapDelta = Header->BitmapDelta;
 
-  SrcDataStart =
+  __llvm_profile_data *SrcDataStart =
       (__llvm_profile_data *)(ProfileData + sizeof(__llvm_profile_header) +
                               Header->BinaryIdsSize);
-  SrcDataEnd = SrcDataStart + Header->NumData;
-  SrcCountersStart = (char *)SrcDataEnd;
-  SrcCountersEnd = SrcCountersStart +
-                   Header->NumCounters * __llvm_profile_counter_entry_size();
-  SrcBitmapStart = ptr_add_with_overflow(
-      SrcCountersEnd,
-      __llvm_profile_get_num_padding_bytes(SrcCountersEnd - SrcCountersStart));
-  SrcNameStart = ptr_add_with_overflow(SrcBitmapStart, Header->NumBitmapBytes);
-  SrcValueProfDataStart =
+  __llvm_profile_data *SrcDataEnd = SrcDataStart + Header->NumData;
+  uintptr_t SrcCountersStart = (uintptr_t)SrcDataEnd;
+  uintptr_t SrcCountersEnd =
+      SrcCountersStart +
+      Header->NumCounters * __llvm_profile_counter_entry_size();
+  uintptr_t SrcBitmapStart =
+      SrcCountersEnd +
+      __llvm_profile_get_num_padding_bytes(SrcCountersEnd - SrcCountersStart);
+  uintptr_t SrcNameStart = SrcBitmapStart + Header->NumBitmapBytes;
+  uintptr_t SrcValueProfDataStart =
       SrcNameStart + getDistanceFromCounterToValueProf(Header);
   if (SrcNameStart < SrcCountersStart || SrcNameStart < SrcBitmapStart)
     return 1;
@@ -172,13 +161,13 @@ int __llvm_profile_merge_from_buffer(const char *ProfileData,
   // Merge counters by iterating the entire counter section when data section is
   // empty due to correlation.
   if (Header->NumData == 0) {
-    for (SrcCounter = SrcCountersStart,
-        DstCounter = __llvm_profile_begin_counters();
+    for (uintptr_t SrcCounter = SrcCountersStart,
+                   DstCounter = (uintptr_t)__llvm_profile_begin_counters();
          SrcCounter < SrcCountersEnd;) {
       if (__llvm_profile_get_version() & VARIANT_MASK_BYTE_COVERAGE) {
-        *DstCounter &= *SrcCounter;
+        *(char *)DstCounter &= *(const char *)SrcCounter;
       } else {
-        *(uint64_t *)DstCounter += *(uint64_t *)SrcCounter;
+        *(uint64_t *)DstCounter += *(const uint64_t *)SrcCounter;
       }
       SrcCounter += __llvm_profile_counter_entry_size();
       DstCounter += __llvm_profile_counter_entry_size();
@@ -186,6 +175,8 @@ int __llvm_profile_merge_from_buffer(const char *ProfileData,
     return 0;
   }
 
+  __llvm_profile_data *SrcData, *DstData;
+  uintptr_t SrcValueProfData;
   for (SrcData = SrcDataStart,
       DstData = (__llvm_profile_data *)__llvm_profile_begin_data(),
       SrcValueProfData = SrcValueProfDataStart;
@@ -194,10 +185,10 @@ int __llvm_profile_merge_from_buffer(const char *ProfileData,
     // address of the data to the start address of the counter. On WIN64,
     // CounterPtr is a truncated 32-bit value due to COFF limitation. Sign
     // extend CounterPtr to get the original value.
-    char *DstCounters =
-        (char *)((uintptr_t)DstData + signextIfWin64(DstData->CounterPtr));
-    char *DstBitmap =
-        (char *)((uintptr_t)DstData + signextIfWin64(DstData->BitmapPtr));
+    uintptr_t DstCounters =
+        (uintptr_t)DstData + signextIfWin64(DstData->CounterPtr);
+    uintptr_t DstBitmap =
+        (uintptr_t)DstData + signextIfWin64(DstData->BitmapPtr);
     unsigned NVK = 0;
 
     // SrcData is a serialized representation of the memory image. We need to
@@ -207,8 +198,8 @@ int __llvm_profile_merge_from_buffer(const char *ProfileData,
     // CountersDelta computes the offset into the in-buffer counter section.
     //
     // On WIN64, CountersDelta is truncated as well, so no need for signext.
-    const char *SrcCounters = ptr_add_with_overflow(
-        SrcCountersStart, (uintptr_t)SrcData->CounterPtr - CountersDelta);
+    uintptr_t SrcCounters =
+        SrcCountersStart + ((uintptr_t)SrcData->CounterPtr - CountersDelta);
     // CountersDelta needs to be decreased as we advance to the next data
     // record.
     CountersDelta -= sizeof(*SrcData);
@@ -221,14 +212,14 @@ int __llvm_profile_merge_from_buffer(const char *ProfileData,
     for (unsigned I = 0; I < NC; I++) {
       if (__llvm_profile_get_version() & VARIANT_MASK_BYTE_COVERAGE) {
         // A value of zero signifies the function is covered.
-        DstCounters[I] &= SrcCounters[I];
+        ((char *)DstCounters)[I] &= ((const char *)SrcCounters)[I];
       } else {
-        ((uint64_t *)DstCounters)[I] += ((uint64_t *)SrcCounters)[I];
+        ((uint64_t *)DstCounters)[I] += ((const uint64_t *)SrcCounters)[I];
       }
     }
 
-    const char *SrcBitmap = ptr_add_with_overflow(
-        SrcBitmapStart, (uintptr_t)SrcData->BitmapPtr - BitmapDelta);
+    uintptr_t SrcBitmap =
+        SrcBitmapStart + ((uintptr_t)SrcData->BitmapPtr - BitmapDelta);
     // BitmapDelta also needs to be decreased as we advance to the next data
     // record.
     BitmapDelta -= sizeof(*SrcData);
@@ -239,7 +230,7 @@ int __llvm_profile_merge_from_buffer(const char *ProfileData,
         return 1;
       // Merge Src and Dst Bitmap bytes by simply ORing them together.
       for (unsigned I = 0; I < NB; I++)
-        DstBitmap[I] |= SrcBitmap[I];
+        ((char *)DstBitmap)[I] |= ((const char *)SrcBitmap)[I];
     }
 
     /* Now merge value profile data. */
@@ -252,7 +243,7 @@ int __llvm_profile_merge_from_buffer(const char *ProfileData,
     if (!NVK)
       continue;
 
-    if (SrcValueProfData >= ProfileData + ProfileSize)
+    if (SrcValueProfData >= (uintptr_t)ProfileData + ProfileSize)
       return 1;
     VPMergeHook((ValueProfData *)SrcValueProfData, DstData);
     SrcValueProfData =

[ellishg]

I don't think we need the explicit cast, do we? Same for DstCounter on line 165.

Suggested change

	uintptr_t SrcCountersStart = (uintptr_t)SrcDataEnd;
	uintptr_t SrcCountersStart = SrcDataEnd;

[nikic]

SrcDataEnd is a __llvm_profile_data * so it does need a cast. I only switched the parts using (const) char * to uintptr_t.