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-132097: remove unnecessary clinic casts to `PyCFunction` and others by picnixz · Pull Request #131665 · python/cpython

Conversation

@picnixz

@picnixz picnixz commented Mar 24, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: UBSan: runtime undefined behaviours when using -fsanitize=undefined -fno-sanitize-recover #132097

Sorry, something went wrong.

Uh oh!

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

All reactions
picnixz added 3 commits March 24, 2025 12:54
@picnixz
do not add a PyCFunction cast for METH_NOARGS, METH_O and METH_VARARGS
074b2ae
@picnixz
update clinic hard-coded tests
65b14c1
@picnixz
update autogenerated content
4e2153a
@bedevere-app bedevere-app Bot mentioned this pull request Mar 24, 2025
UBSan: Calling a function through pointer to incorrect function type is undefined behavior #111178
Closed
@picnixz picnixz added the skip news label Mar 24, 2025
picnixz added 4 commits April 3, 2025 09:41
@picnixz
Merge remote-tracking branch 'upstream/main' into fix/ubsan/ac-redund…
502f5cb
…ant-casts-111178
@picnixz
remove auto-genmerated getter/setter casts
857e476
@picnixz
sync
f1b1dc9
@picnixz
Merge remote-tracking branch 'upstream/main' into fix/ubsan/ac-redund…
24a02bc
…ant-casts-111178
@picnixz

picnixz commented Apr 4, 2025

Copy link
Copy Markdown
Member Author

@vstinner This one is purely cosmetic but it affects lots of files. It's about removing redundant casts. I've removed some redundant casts with non-clinic files but this one just removes the casts generated by clinic.

I'm not entirely sure I want to merge this for the following reason:

  • let's say clinic incorrectly generates a signature, and the signature then becomes incompatible. With this change, this would affect the PR with a bad clinic signature. However, this means that we need to fix clinic in the meantime.
  • Without removing the explicit casts, we can at least make other PRs relying on clinic not affected by subtle UBs that may or may not be impactful.
  • For WASM, UBs are strictly checked: "Bad fpcasts are an issue in WebAssembly. WASM's indirect_call has strict function signature checks. Argument count, types, and return type must match." (this is a comment for _PyCFunction_TrampolineCall in pycore_object.h (which is also duplicated in pycore_emscripten_trampoline.h).
  • So technically, while it's a UB for all platforms, it's much more annoying on Emscripten. So maybe it's better that UBs are always detected at compile time.

I'm going to first address the fastcall issue before continuing here so take your time. I won't commit if you're not happy with this change or if you prefer that we hold it until we see a real issue.

All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz added 2 commits April 5, 2025 11:42
@picnixz
Merge remote-tracking branch 'upstream/main' into fix/ubsan/ac-redund…
44b7246
…ant-casts-111178
@picnixz
clinic
8d2fb6d
@picnixz picnixz changed the title gh-111178: remove unnecessary clinic casts to PyCFunction gh-111178: remove unnecessary clinic casts to PyCFunction and others Apr 5, 2025
@picnixz picnixz changed the title gh-111178: remove unnecessary clinic casts to PyCFunction and others gh-131665: remove unnecessary clinic casts to PyCFunction and others Apr 5, 2025
@picnixz picnixz changed the title gh-131665: remove unnecessary clinic casts to PyCFunction and others gh-132097: remove unnecessary clinic casts to PyCFunction and others Apr 5, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Apr 5, 2025
UBSan: runtime undefined behaviours when using -fsanitize=undefined -fno-sanitize-recover #132097
Closed
picnixz added 3 commits April 5, 2025 12:46
@picnixz
clinic
af0ee39
@picnixz
Merge remote-tracking branch 'upstream/main' into fix/ubsan/ac-redund…
1f20b58
…ant-casts-111178
@picnixz
Merge remote-tracking branch 'upstream/main' into fix/ubsan/ac-redund…
21b3667
…ant-casts-111178
@picnixz

picnixz commented Apr 20, 2025

Copy link
Copy Markdown
Member Author

I more or less added explicit casts for WASM in #132406 so what's left here is just removing redundant casts. If someone wants to push for that, we'll do it but it will create conflicts in many generated files (which, thankfully, are easy to fix with make clinic). That being said, it's not that useful for now. If I were to be alone on the project, I would have pushed this one. But I'm not and I eventually don't see a real value that this PR brings. So I will close this.

All reactions

Sorry, something went wrong.

Uh oh!

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

@picnixz picnixz closed this Apr 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland will be requested when the pull request is marked ready for review erlend-aasland is a code owner
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner will be requested when the pull request is marked ready for review AA-Turner is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

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.

1 participant

@picnixz
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.