gh-131591: Fix GENERATE_DEBUG_SECTION for clangcl on Windows #132112
Conversation
chris-eibl
requested review from
1st1,
asvetlov,
kumaraditya303,
willingc,
ezio-melotti,
hugovk and
AA-Turner
as code owners
| #if defined(MS_WINDOWS) && !defined(__clang__) | ||
| // Please note that section names are truncated to eight bytes | ||
| // on Windows! | ||
| #if defined(MS_WINDOWS) |
There was a problem hiding this comment.
@Fidget-Spinner: clang-cl actually understands the __declspec and now we have the same section name without the . like for MSVC.
| @@ -116,7 +116,7 @@ typedef struct _Py_AsyncioModuleDebugOffsets { | |||
| } asyncio_thread_state; | |||
| } Py_AsyncioModuleDebugOffsets; | |||
|
|
|||
| GENERATE_DEBUG_SECTION(AsyncioDebug, Py_AsyncioModuleDebugOffsets AsyncioDebug) | |||
| GENERATE_DEBUG_SECTION(AsyncioDebug, Py_AsyncioModuleDebugOffsets _AsyncioDebug) | |||
There was a problem hiding this comment.
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
Line 112 in ac14d4a
where
_PyRuntime has a leading underscore, too.
This reverts commit fc18d45.
|
We need a news entry please, because the debug section is now an externally facing thing. And we changed the name for |
|
The debug section name did not change, and the 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? |
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. |
|
IMHO, the only change in the Windows clang-cl binary is, that now the So I could mention something like " In case of Windows, the symbol names are in the Whether (and how spelled) that shall go into a news entry I am unsure about. |
|
Ok if there's no change on existing platforms we can skip news. |
See #132098 where the discussion started.
I think this a skip news?
Maybe a skip issue, too? Unsure, whether to link with #131591.