-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[Clang] Add release note for pointer overflow optimization change #122462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-clang Author: Nikita Popov (nikic) ChangesAdd a release note for optimization change related to pointer overflow checks. I've put this in the breaking changes section to give it the best chance of being seen. Full diff: https://github.com/llvm/llvm-project/pull/122462.diff 1 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 511a28c5554bbb..aea5eb2a04ac63 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -58,6 +58,26 @@ code bases.
containing strict-aliasing violations. The new default behavior can be
disabled using ``-fno-pointer-tbaa``.
+- Clang will now more aggressively use undefined behavior on pointer addition
+ overflow for optimization purposes. For example, a check like
+ ``ptr + unsigned_offset < ptr`` will now optimize to ``false``, because
+ ``ptr + unsigned_offset`` will cause undefined behavior if it overflows (or
+ advances past the end of the object).
+
+ Previously, ``ptr + unsigned_offset < ptr`` was optimized (by both Clang and
+ GCC) to ``(ssize_t)unsigned_offset < 0``. This also results in an incorrect
+ overflow check, but in a way that is less apparent when only testing with
+ pointers in the low half of the address space.
+
+ To avoid pointer addition overflow, it is necessary to perform the addition
+ on integers, for example using
+ ``(uintptr_t)ptr + unsigned_offset < (uintptr_t)ptr``.
+
+ Undefined behavior due to pointer addition overflow can be reliably detected
+ using ``-fsanitize=pointer-overflow``. It is also possible to use
+ ``-fno-strict-overflow`` to opt-in to a language dialect where signed integer
+ and pointer overflow are well-defined.
+
C/C++ Language Potentially Breaking Changes
-------------------------------------------
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for the release note!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up on this!
clang/docs/ReleaseNotes.rst
Outdated
To avoid pointer addition overflow, it is necessary to perform the addition | ||
on integers, for example using | ||
``(uintptr_t)ptr + unsigned_offset < (uintptr_t)ptr``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In many cases it's possible to rewrite the check in the integer domain instead, checking that unsigned_offset
is less then some ptr diff. Should we mention something like that here, so that people don't immediately just add the uintptr_t casts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the case you have in mind here where people have a ptr
and end_ptr
and write something like ptr + offset < end_ptr
, which can be rewritten as offset < end_ptr - ptr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking of code like if (ptr + offset >= end_ptr || ptr + offset < ptr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an extra example on how to rewrite the check. Does that look good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you!
``-fno-strict-overflow`` to opt-in to a language dialect where signed integer | ||
and pointer overflow are well-defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this flag vs. -fwrapv (which we went with in Chromium). Is there a difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Clang, -fno-strict-overflow
and -fwrapv
are the same. With GCC, -fwrapv
only controls signed integer overflow, while pointer overflow uses a separate -fwrapv-pointer
flag. That's why I'm recommending -fno-strict-overflow
here, as it will work on both compilers with the same semantics.
I'm also considering adding -fwrapv-pointer
to Clang, in which case we could recommend -fwrapv-pointer
as the minimally intrusive option here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks for explaining!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here's the PR for -fwrapv-pointer
: #122486
@@ -58,6 +58,26 @@ code bases. | |||
containing strict-aliasing violations. The new default behavior can be | |||
disabled using ``-fno-pointer-tbaa``. | |||
|
|||
- Clang will now more aggressively use undefined behavior on pointer addition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a motivating example to show this is not about pedantry but is useful for removing redundant checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's hard to add an example that is small and simple enough for a release note. After all, the distinguishing feature between a desirable and an undesirable optimization in this context is just how many layers of abstraction there are. (This is Schrödinger's programmer: Doesn't want the compiler to optimize trivial cases because its "obviously wrong". Does want it to optimize complex cases.)
E.g. one of the motivating cases I have lying around is https://godbolt.org/z/3Y5bTxzqq, and that is both too large for a release note and too reduced to be really meaningful :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current version lgtm by the way
Maybe we should also put a blurb in the main llvm release notes and link to this one.
…vm#122462) Add a release note for optimization change related to pointer overflow checks. I've put this in the breaking changes section to give it the best chance of being seen.
Add a release note for optimization change related to pointer overflow checks. I've put this in the breaking changes section to give it the best chance of being seen.