Skip to content

gh-131876: extract _hashlib helpers into a separate directory - #136995

Merged
picnixz merged 11 commits into
python:mainfrom
picnixz:refactor/hashlib/extract-helpers-135341
Jul 28, 2025
Merged

gh-131876: extract _hashlib helpers into a separate directory#136995
picnixz merged 11 commits into
python:mainfrom
picnixz:refactor/hashlib/extract-helpers-135341

Conversation

@picnixz

@picnixzpicnixz commented Jul 22, 2025

Copy link
Copy Markdown
Member

@picnixzpicnixz changed the title gh-135341: extract _hashlib helpers into a separate directorygh-131876: extract _hashlib helpers into a separate directoryJul 22, 2025
@picnixz
picnixzforce-pushed the refactor/hashlib/extract-helpers-135341 branch from 93fdbd3 to 59b0cdaCompareJuly 22, 2025 13:23
@picnixzpicnixz added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 22, 2025
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @picnixz for commit 59b0cda 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136995%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 22, 2025
@picnixz
picnixz marked this pull request as ready for review July 25, 2025 14:00
@picnixz

picnixz commented Jul 27, 2025

Copy link
Copy Markdown
MemberAuthor

I'll first merge the fortication PR, then merge this one where the macros are rewritten are pure functions when possible. I actually wanted to check whether using an exported function vs a static inline function could change a lot the performance (if possible I don't want to make the code significantly slower because I used non-static inline functions).

@gpshead

Copy link
Copy Markdown
Member

for these code paths I doubt inline even matters. in general these days I assume the compiler will figure that out for static things, at least in pgo and lto builds.

@picnixz

picnixz commented Jul 28, 2025

Copy link
Copy Markdown
MemberAuthor

for these code paths I doubt inline even matters. in general these days I assume the compiler will figure that out for static things, at least in pgo and lto builds.

Actually, I was also a bit worried about the exported symbol. I also try to see if I can just live with extern and not with PyAPI_FUNC actually and if everything links correctly, then I'll be able to move the function definitions into a .c file instead.

Otherwise, I'll use static inline for both instead of using PyAPI_FUNC.

EDIT: everything works well with extern

@picnixz

Copy link
Copy Markdown
MemberAuthor

Hum. Windows. Ok I don't know how to correct the project so using PyAPI_FUNC should be enough. I think we should find a better way to manage cryptographic modules on Windows as I feel it's confusing (I don't really know what to exactly do...)

@picnixz

Copy link
Copy Markdown
MemberAuthor

!buildbot FIPS only

@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @picnixz for commit a0fa849 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136995%2Fmerge

The command will test the builders whose names match following regular expression: FIPS only

The builders matched are:

  • AMD64 RHEL8 FIPS Only Blake2 Builtin Hash PR
  • AMD64 CentOS9 FIPS Only Blake2 Builtin Hash PR

@picnixz
picnixz marked this pull request as ready for review July 28, 2025 09:17
@picnixz
picnixz merged commit 45138d3 into python:mainJul 28, 2025
45 checks passed
@picnixz
picnixz deleted the refactor/hashlib/extract-helpers-135341 branch July 28, 2025 09:28
@picnixz

Copy link
Copy Markdown
MemberAuthor

I don't think I can backport this as I think it's an ABI change?

@freakboy3742

Copy link
Copy Markdown
Contributor

Not sure why this hasn't flagged, but this seems to have caused a big problem for the Emscripten buildbot: https://buildbot.python.org/#/builders/1810/builds/234

@hoodmane

Copy link
Copy Markdown
Contributor

Problem is we pass libhashlib.a to the linker twice which wasm-ld doesn't like.

@zooba

Copy link
Copy Markdown
Member

There shouldn't be any need for PyAPI_FUNC in internal headers - it's for public exported API (that needs to be used by apps loading python315.dll).

This seems like it should be a fully internal change, just refactoring sources?

@picnixz

Copy link
Copy Markdown
MemberAuthor

Oh so it did cause an issue. I actually tried using extern but the Windows build failed so I wasn't sure about it.

@picnixz

picnixz commented Aug 1, 2025

Copy link
Copy Markdown
MemberAuthor

I will take care of this by making everything static inline just to prevent polluting main for a moment. Or I'll directly try to see how I can correctly use extern (I think the problem lies with the MSVC projects where I get duplicated symbols)

@zooba could you help me here setting up the projects?

@picnixz

picnixz commented Aug 1, 2025

Copy link
Copy Markdown
MemberAuthor

I tried something but I can only rely on the CI to test the build: #137301. I think the issue is that we have a hashlib project but we don't have HACL* projects. Is it sufficient for me to actually only specify the files to compile in the hashlib project (I don't know if it's possible to disable hashlib on Windows)? would they be picked up by the "parent" project correctly? EDIT: well apparently not. Let's discuss what I should do on the new PR.

picnixz added a commit to picnixz/cpython that referenced this pull request Aug 1, 2025
gpshead pushed a commit that referenced this pull request Aug 1, 2025
…ate directory (#136995) (#137307)
Revert "gh-131876: extract `_hashlib` helpers into a separate directory (#136995)"
This reverts commit 45138d3.
@bedevere-bot

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 CentOS9 NoGIL Refleaks 3.x (tier-1) has failed when building commit fe0e921.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1610/builds/1839) and take a look at the build logs.
  4. Check if the failure is related to this commit (fe0e921) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1610/builds/1839

Failed tests:

  • test_free_threading

Summary of the results of the build (if available):

==

Click to see traceback logs
remote: Enumerating objects: 7, done. remote: Counting objects: 14% (1/7) remote: Counting objects: 28% (2/7) remote: Counting objects: 42% (3/7) remote: Counting objects: 57% (4/7) remote: Counting objects: 71% (5/7) remote: Counting objects: 85% (6/7) remote: Counting objects: 100% (7/7) remote: Counting objects: 100% (7/7), done. remote: Compressing objects: 14% (1/7) remote: Compressing objects: 28% (2/7) remote: Compressing objects: 42% (3/7) remote: Compressing objects: 57% (4/7) remote: Compressing objects: 71% (5/7) remote: Compressing objects: 85% (6/7) remote: Compressing objects: 100% (7/7) remote: Compressing objects: 100% (7/7), done. remote: Total 7 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) 
From https://github.com/python/cpython
* branch main -> FETCH_HEAD
Note: switching to 'fe0e921817a7f96c62c91085884ab910859328ce'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:
git switch -c <new-branch-name>
Or undo this operation with:
git switch -
Turn off this advice by setting config variable advice.detachedHead to false
HEAD is now at fe0e921817a gh-131876: Revert "gh-131876: extract `_hashlib` helpers into a separate directory (#136995) (#137307)
Switched to and reset branch 'main'
configure: WARNING: no system libmpdec found; falling back to pure-Python version for the decimal modulemake: *** [Makefile:2486: buildbottest] Error 2

picnixz added a commit to picnixz/cpython that referenced this pull request Aug 2, 2025
…python#136995)
The `Modules/hashlib.h` helper file is now removed and split into multiple files:
* `Modules/_hashlib/hashlib_buffer.[ch]` -- Utilities for getting a buffer view and handling buffer inputs.
* `Modules/_hashlib/hashlib_fetch.h` -- Utilities used when fetching a message digest from a digest-like identifier.
Currently, this file only contains common error messages as the fetching API is not yet implemented.
* `Modules/_hashlib/hashlib_mutex.h` -- Utilities for managing the lock on cryptographic hash objects.
picnixz added a commit to picnixz/cpython that referenced this pull request Aug 2, 2025
…python#136995)
The `Modules/hashlib.h` helper file is now removed and split into multiple files:
* `Modules/_hashlib/hashlib_buffer.[ch]` -- Utilities for getting a buffer view and handling buffer inputs.
* `Modules/_hashlib/hashlib_fetch.h` -- Utilities used when fetching a message digest from a digest-like identifier.
Currently, this file only contains common error messages as the fetching API is not yet implemented.
* `Modules/_hashlib/hashlib_mutex.h` -- Utilities for managing the lock on cryptographic hash objects.
picnixz added a commit to picnixz/cpython that referenced this pull request Aug 2, 2025
…python#136995)
The `Modules/hashlib.h` helper file is now removed and split into multiple files:
* `Modules/_hashlib/hashlib_buffer.[ch]` -- Utilities for getting a buffer view and handling buffer inputs.
* `Modules/_hashlib/hashlib_fetch.h` -- Utilities used when fetching a message digest from a digest-like identifier.
Currently, this file only contains common error messages as the fetching API is not yet implemented.
* `Modules/_hashlib/hashlib_mutex.h` -- Utilities for managing the lock on cryptographic hash objects.
picnixz added a commit to picnixz/cpython that referenced this pull request Aug 2, 2025
…python#136995)
The `Modules/hashlib.h` helper file is now removed and split into multiple files:
* `Modules/_hashlib/hashlib_buffer.h` -- Utilities for getting a buffer view and handling buffer inputs.
* `Modules/_hashlib/hashlib_fetch.h` -- Utilities used when fetching a message digest from a digest-like identifier.
Currently, this file only contains common error messages as the fetching API is not yet implemented.
* `Modules/_hashlib/hashlib_mutex.h` -- Utilities for managing the lock on cryptographic hash objects.
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
…python#136995)
The `Modules/hashlib.h` helper file is now removed and split into multiple files:
* `Modules/_hashlib/hashlib_buffer.[ch]` -- Utilities for getting a buffer view and handling buffer inputs.
* `Modules/_hashlib/hashlib_fetch.h` -- Utilities used when fetching a message digest from a digest-like identifier.
Currently, this file only contains common error messages as the fetching API is not yet implemented.
* `Modules/_hashlib/hashlib_mutex.h` -- Utilities for managing the lock on cryptographic hash objects.
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
…into a separate directory (python#136995) (python#137307)
Revert "pythongh-131876: extract `_hashlib` helpers into a separate directory (python#136995)"
This reverts commit 45138d3.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@picnixz@bedevere-bot@gpshead@freakboy3742@hoodmane@zooba