FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-131591: Fix GENERATE_DEBUG_SECTION for clangcl on Windows by chris-eibl · Pull Request #132112 · python/cpython · GitHub

/ cpython Public

gh-131591: Fix GENERATE_DEBUG_SECTION for clangcl on Windows - #132112

Merged
Fidget-Spinner merged 4 commits into
python:mainfrom
chris-eibl:fix_clangcl_GENERATE_DEBUG_SECTION
Apr 5, 2025
ConversationCommits (4)ChecksFiles changed
Merged

gh-131591: Fix GENERATE_DEBUG_SECTION for clangcl on Windows#132112
Fidget-Spinner merged 4 commits into
python:mainfrom
chris-eibl:fix_clangcl_GENERATE_DEBUG_SECTION

Conversation

chris-eibl commented Apr 5, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

See #132098 where the discussion started.

I think this a skip news?

Maybe a skip issue, too? Unsure, whether to link with #131591.

bedevere-app Bot mentioned this pull request Apr 5, 2025
Closed
chris-eibl commented Apr 5, 2025
View reviewed changes
Comment thread Include/internal/pycore_debug_offsets.h
#if defined(MS_WINDOWS) && !defined(__clang__)
// Please note that section names are truncated to eight bytes
// on Windows!
#if defined(MS_WINDOWS)

chris-eibl Apr 5, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Fidget-Spinner: clang-cl actually understands the __declspec and now we have the same section name without the . like for MSVC.

chris-eibl commented Apr 5, 2025
View reviewed changes
Comment thread Modules/_asynciomodule.c
} Py_AsyncioModuleDebugOffsets;

GENERATE_DEBUG_SECTION(AsyncioDebug, Py_AsyncioModuleDebugOffsets AsyncioDebug)
GENERATE_DEBUG_SECTION(AsyncioDebug, Py_AsyncioModuleDebugOffsets _AsyncioDebug)

chris-eibl Apr 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually, this is the reason why clangcl failed with

error: symbol 'AsyncioDebug' is already defined

https://github.com/python/cpython/actions/runs/13288425017/job/37102791006#step:4:243

This also "syncs" with

cpython/Python/pylifecycle.c

Line 112 in ac14d4a

GENERATE_DEBUG_SECTION(PyRuntime, _PyRuntimeState _PyRuntime)

where _PyRuntime has a leading underscore, too.

chris-eibl mentioned this pull request Apr 5, 2025
Merged
hugovk removed their request for review April 5, 2025 11:55

Fidget-Spinner commented Apr 5, 2025

Copy link
Copy Markdown
Member

We need a news entry please, because the debug section is now an externally facing thing. And we changed the name for _AsyncioDebug.

chris-eibl commented Apr 5, 2025

Copy link
Copy Markdown
Member Author

The debug section name did not change, and the _AsyncioDebug is only an internal variable name?

Otherwise, the test cases would have failed - they only do the lookup per section name.

But sure, I can add a news entry. Can you suggest a wording?

Fidget-Spinner commented Apr 5, 2025

Copy link
Copy Markdown
Member

The debug section name did not change, and the _AsyncioDebug is only an internal variable name?

Just to clarify: there are no changes to the name of the symbol in the binary? If there aren't any changes, we can skip the news entry.

chris-eibl commented Apr 5, 2025

Copy link
Copy Markdown
Member Author

IMHO, the only change in the Windows clang-cl binary is, that now the PyRuntim and AsyncioD sections are emitted, like MSVC does. For all other platforms, nothing should have changed.

So I could mention something like "GENERATE_DEBUG_SECTION now emits the debug sections for clang-cl on Windows."

In case of Windows, the symbol names are in the pdb files, in case of Linux in the ELF files (unless stripped). I don't know whether this shall be mentioned, but strictly speaking - sure you are correct: the symbol name has changed.

Whether (and how spelled) that shall go into a news entry I am unsure about.

Fidget-Spinner commented Apr 5, 2025

Copy link
Copy Markdown
Member

Ok if there's no change on existing platforms we can skip news.

Fidget-Spinner approved these changes Apr 5, 2025
View reviewed changes

Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks!

Fidget-Spinner merged commit d827d4d into python:main Apr 5, 2025
chris-eibl deleted the fix_clangcl_GENERATE_DEBUG_SECTION branch April 6, 2025 07:00
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Fidget-Spinner Fidget-Spinner approved these changes
1st1 Awaiting requested review from 1st1 1st1 is a code owner
asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner
willingc Awaiting requested review from willingc willingc is a code owner
ezio-melotti Awaiting requested review from ezio-melotti
AA-Turner Awaiting requested review from AA-Turner
ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL