[profile] Fix bounds checks in profile merging #118782
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-pgo Author: Nikita Popov (nikic) ChangesThese bounds checks work on the result of the pointer addition -- but the pointer addition already asserts that no overflow may occur, so the checks are optimized away after #118472. Avoid this by performing the addition in a way that permits overflow. Full diff: https://github.com/llvm/llvm-project/pull/118782.diff 1 Files Affected:
diff --git a/compiler-rt/lib/profile/InstrProfilingMerge.c b/compiler-rt/lib/profile/InstrProfilingMerge.c
index 7cf1679811eb07..77fc69110c73ea 100644
--- a/compiler-rt/lib/profile/InstrProfilingMerge.c
+++ b/compiler-rt/lib/profile/InstrProfilingMerge.c
@@ -127,6 +127,12 @@ 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) {
@@ -154,9 +160,10 @@ int __llvm_profile_merge_from_buffer(const char *ProfileData,
SrcCountersStart = (char *)SrcDataEnd;
SrcCountersEnd = SrcCountersStart +
Header->NumCounters * __llvm_profile_counter_entry_size();
- SrcBitmapStart = SrcCountersEnd + __llvm_profile_get_num_padding_bytes(
- SrcCountersEnd - SrcCountersStart);
- SrcNameStart = SrcBitmapStart + Header->NumBitmapBytes;
+ SrcBitmapStart = ptr_add_with_overflow(
+ SrcCountersEnd,
+ __llvm_profile_get_num_padding_bytes(SrcCountersEnd - SrcCountersStart));
+ SrcNameStart = ptr_add_with_overflow(SrcBitmapStart, Header->NumBitmapBytes);
SrcValueProfDataStart =
SrcNameStart + getDistanceFromCounterToValueProf(Header);
if (SrcNameStart < SrcCountersStart || SrcNameStart < SrcBitmapStart)
@@ -200,8 +207,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.
- char *SrcCounters =
- SrcCountersStart + ((uintptr_t)SrcData->CounterPtr - CountersDelta);
+ char *SrcCounters = ptr_add_with_overflow(
+ SrcCountersStart, (uintptr_t)SrcData->CounterPtr - CountersDelta);
// CountersDelta needs to be decreased as we advance to the next data
// record.
CountersDelta -= sizeof(*SrcData);
|
chapuni
approved these changes
Dec 5, 2024
These bounds checks work on the result of the pointer addition -- but the pointer addition already asserts that no overflow may occur, so the checks are optimized away after llvm#118472. Avoid this by performing the addition in a way that permits overflow.
nikic
force-pushed
the
profile-bounds-check
branch
from
44d55b7 to
b6d1b81
Compare
I'm not sure I get what you mean here -- are you suggesting we should do everything in uintptr rather than const char and then only cast to a pointer type when doing accesses? |
|
I will apply both patches and validate in about 1h |
vitalybuka
approved these changes
Dec 5, 2024
I agree with @chapuni, it seems cleaner to make the type of |
nikic
added a commit
to nikic/llvm-project
that referenced
this pull request
Dec 6, 2024
Based on the feedback from llvm#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.
|
I've created a followup to switch most of the arithmetic to uintptr_t here: #118944 |
nikic
added a commit
that referenced
this pull request
Dec 6, 2024
The profile runtime test failure this caused has been addressed in: #118782 ----- Unsigned icmp of gep nuw folds to unsigned icmp of offsets. Unsigned icmp of gep nusw nuw folds to unsigned samesign icmp of offsets. Proofs: https://alive2.llvm.org/ce/z/VEwQY8
nikic
added a commit
that referenced
this pull request
Dec 9, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These bounds checks work on the result of the pointer addition -- but the pointer addition already asserts that no overflow may occur, so the checks are optimized away after #118472. Avoid this by performing the addition in a way that permits overflow.