Skip to content

[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

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Jan 10, 2025

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.

@nikic nikic requested a review from AaronBallman January 10, 2025 14:18
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Jan 10, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 10, 2025

@llvm/pr-subscribers-clang

Author: Nikita Popov (nikic)

Changes

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.


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

1 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+20)
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
 -------------------------------------------
 

Copy link
Collaborator

@AaronBallman AaronBallman left a 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!

Copy link
Collaborator

@zmodem zmodem left a 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!

Comment on lines 72 to 74
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``.
Copy link
Collaborator

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?

Copy link
Contributor Author

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?

Copy link
Collaborator

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)

Copy link
Contributor Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you!

Comment on lines +78 to +79
``-fno-strict-overflow`` to opt-in to a language dialect where signed integer
and pointer overflow are well-defined.
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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!

Copy link
Contributor Author

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
Copy link
Collaborator

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?

Copy link
Contributor Author

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 :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

Copy link
Collaborator

@zmodem zmodem left a 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.

@nikic nikic merged commit c2979c5 into llvm:main Jan 13, 2025
6 of 8 checks passed
@nikic nikic deleted the pointer-overflow-release-note branch January 13, 2025 10:24
kazutakahirata pushed a commit to kazutakahirata/llvm-project that referenced this pull request Jan 13, 2025
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants