GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-101525: Use only safe identical code folding with BOLT by geofft · Pull Request #134642 · python/cpython

Conversation

@geofft

@geofft geofft commented May 25, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

"Identical code folding" (ICF) is the feature of an optimizer to find that two functions have the same code and that they can therefore be deduplicated in the binary. While this is usually safe, it can cause observable behavior differences if the program relies on the fact that the two functions have different addresses.

CPython relies on this in (at least) Objects/typeobject.c, which defines two functions wrap_binaryfunc() and wrap_binaryfunc_l() with the same implementation, and stores their addresses in the slotdefs array. If these two functions have the same address, update_one_slot() in that file will fill in slots it shouldn't, causing, for instances, classes defined in Python that inherit from some built-in types to misbehave.

As of LLVM 20 (llvm/llvm-project#116275), BOLT has a "safe ICF" mode, where it looks to see if there are any uses of a function symbol outside function calls (e.g., relocations in data sections) and skips ICF on such functions. The intent is that this avoids observable behavior differences but still saves storage as much as possible.

This version is about two months old at the time of writing. To support older LLVM versions, we have to turn off ICF entirely.

This problem was previously noticed for Windows/MSVC in #53093 (and again in #24098), where the default behavior of PGO is to enable ICF (which they expand to "identical COMDAT folding") and we had to turn it off.

  • Issue: Make BOLT as stable feature #101525

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@geofft
Use only safe identical code folding with BOLT
91fc5ae
"Identical code folding" (ICF) is the feature of an optimizer to find that two
functions have the same code and that they can therefore be deduplicated
in the binary. While this is usually safe, it can cause observable
behavior differences if the program relies on the fact that the two
functions have different addresses.

CPython relies on this in (at least) Objects/typeobject.c, which defines
two functions wrap_binaryfunc() and wrap_binaryfunc_l() with the same
implementation, and stores their addresses in the slotdefs array. If
these two functions have the same address, update_one_slot() in that
file will fill in slots it shouldn't, causing, for instances,
classes defined in Python that inherit from some built-in types to
misbehave.

As of LLVM 20 (llvm/llvm-project#116275), BOLT has a "safe ICF" mode,
where it looks to see if there are any uses of a function symbol outside
function calls (e.g., relocations in data sections) and skips ICF on
such functions. The intent is that this avoids observable behavior
differences but still saves storage as much as possible.

This version is about two months old at the time of writing. To support
older LLVM versions, we have to turn off ICF entirely.

This problem was previously noticed for Windows/MSVC in python#53093 (and
again in python#24098), where the default behavior of PGO is to enable ICF
(which they expand to "identical COMDAT folding") and we had to turn it
off.
@geofft
geofft requested review from corona10 and erlend-aasland as code owners May 25, 2025 01:56
@bedevere-app bedevere-app Bot added the awaiting review label May 25, 2025
@geofft

geofft commented May 25, 2025

Copy link
Copy Markdown
Contributor Author

cc @zanieb
fyi @kmod who originally added -icf=1 to the BOLT flags

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced May 25, 2025
Make BOLT as stable feature #101525
Open
Disable unsafe identical code folding in BOLT astral-sh/python-build-standalone#622
Merged
@corona10 corona10 self-assigned this May 25, 2025
@erlend-aasland
erlend-aasland removed their request for review May 25, 2025 12:24
@corona10 corona10 changed the title Use only safe identical code folding with BOLT gh-101525: Use only safe identical code folding with BOLT May 26, 2025
@corona10 corona10 added the skip news label May 26, 2025
emmatyping
emmatyping reviewed May 26, 2025
View reviewed changes
Comment thread configure.ac Resolved

Uh oh!

There was an error while loading. Please reload this page.

geofft added a commit to astral-sh/python-build-standalone that referenced this pull request May 30, 2025
@geofft
Disable unsafe identical code folding in BOLT (#622)
cddb9c1
astral-sh/uv#13610 reported a misbehavior that is the result of a
subclass of str incorrectly having its ->tp_as_number->nb_add slot
filled in with the value of PyUnicode_Type->tp_as_sequence->sq_concat.
There are some times when this is an appropriate thing to do iwhen
subclassing, but this is not one of them. The logic to prevent it in
this case relies on two helper functions in the file, wrap_binaryfunc
and wrap_binaryfunc_l, having different addresses, even though they
contain identical code.

For some reason BOLT does not do this optimization in the shared library
(even though those are static functions and not exported), so we only
started seeing this in the static build.

BOLT in LLVM 20+ supports "safe" code folding, which uses heuristics about
relocations to determine whether a function's address is used in any way other
than a call. This seems to be enough to fix the issue. Add a patch to switch
to -icf=safe, submitted upstream as python/cpython#134642
@corona10 corona10 added needs backport to 3.13 only security fixes needs backport to 3.14 bugs and security fixes labels Jun 1, 2025
corona10
corona10 approved these changes Jun 1, 2025
View reviewed changes

@corona10 corona10 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 digging this issue, Now BOLT build be more stable :)
LGTM.

I like this change and it's real issue: astral-sh/python-build-standalone#622
But I am not sure this is best autoconf that we add, I will wait @erlend-aasland 's review also.
Erlend, the change looks good. Can you review, focusing on the autoconf code itself?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jun 1, 2025
@corona10 corona10 assigned erlend-aasland Jun 1, 2025
erlend-aasland
erlend-aasland reviewed Jun 2, 2025
View reviewed changes
Comment thread configure.ac Resolved

Uh oh!

There was an error while loading. Please reload this page.

@zanieb zanieb mentioned this pull request Nov 24, 2025
Upstream patch patch-configure-bolt-icf-safe.patch astral-sh/python-build-standalone#878
Closed
@github-actions

github-actions Bot commented Apr 25, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 25, 2026
@emmatyping

emmatyping commented May 1, 2026

Copy link
Copy Markdown
Member

@geofft do you want to pick this back up? I believe applying Erlend's suggestion should be sufficient to move this forward.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@utkonos utkonos mentioned this pull request May 12, 2026
BOLT-WARNING: specifying numeric value "1" for option -icf is deprecated #149708
Open
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label May 15, 2026
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.15 bugs and security fixes label May 30, 2026
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 90 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 28, 2026
@zanieb

zanieb commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

@jjhelmus maybe you should pick this up?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Sep 3, 2026
@jjhelmus

jjhelmus commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

@geofft Are you okay if I pick up this work? I can create a new PR from my fork or add commits to your branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@geofft @erlend-aasland
Use WITH_SAVE_ENV and avoid unnecessary brackets
c28b7ec
Co-authored-by: Erlend E. Aasland <[email protected]>
@geofft
geofft requested review from AA-Turner and itamaro as code owners September 11, 2026 18:29
@geofft
geofft marked this pull request as draft September 11, 2026 18:30
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 11, 2026
geofft added 2 commits September 11, 2026 14:33
@geofft
Merge remote-tracking branch 'origin/main' into safe-icf
8c0e9c1
@geofft
autoreconf
4a90c16
@geofft
geofft marked this pull request as ready for review September 11, 2026 18:39
@bedevere-app bedevere-app Bot added the awaiting review label Sep 11, 2026
@geofft

geofft commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Merged with main and applied Erlend's suggestion, I think this is ready for review now, sorry for the delay! (Also in general feel free to pick up any of my branches)

@erlend-aasland, I assume that I used the [[]] syntax because I copied from a few other spots in configure.ac that use AC_LANG_PROGRAM([[]], [[]]). Should all of those also be AC_LANG_PROGRAM([], [])?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@geofft geofft mentioned this pull request Sep 11, 2026
gh-148832: Use -icf=0 in BOLT_APPLY_FLAGS #148833
Merged
@geofft
Add blurb
7be8273
@geofft

geofft commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Also I added a blurb because #148833 which switched to -icf=0 (which landed in the meantime) added a blurb. Originally I figured this was an internal / non-default feature and didn't add a blurb at all. I'm not sure what's expected here, if I ought to edit or delete the other blurb I can do that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zaniebot zaniebot mentioned this pull request Oct 21, 2026
Enable safe identical code folding when BOLT supports it zanieb/cpython#21
Draft
itamaro
itamaro requested changes Nov 7, 2026
View reviewed changes

@itamaro itamaro left a comment

Copy link
Copy Markdown
Contributor

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.

this looks good, except for the issue with the CFLAGS and LDFLAGS (see inline comment). also need to merge main.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread configure.ac
Comment on lines +2240 to +2241
CFLAGS="$CFLAGS_NODIST"
LDFLAGS="$LDFLAGS_NODIST"

@itamaro itamaro Nov 7, 2026

Copy link
Copy Markdown
Contributor

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.

this discards the existing CFLAGS and LDFLAGS - they should be preserved when adding the _NODIST flags.

Suggested change
CFLAGS="$CFLAGS_NODIST"
LDFLAGS="$LDFLAGS_NODIST"
CFLAGS="$CFLAGS $CFLAGS_NODIST"
LDFLAGS="$LDFLAGS $LDFLAGS_NODIST"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting review labels Nov 7, 2026
@bedevere-app

bedevere-app Bot commented Nov 7, 2026

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emmatyping emmatyping emmatyping left review comments
@erlend-aasland erlend-aasland erlend-aasland left review comments
@itamaro itamaro itamaro requested changes
@corona10 corona10 corona10 approved these changes
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@corona10 corona10

@erlend-aasland erlend-aasland

Labels

awaiting changes needs backport to 3.13 only security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 bugs and security fixes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@geofft @emmatyping @zanieb @jjhelmus @itamaro @corona10 @erlend-aasland @serhiy-storchaka
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.