◐ Shell
reader mode source ↗
Skip to content

gh-132132: Upgrade LLVM on tail calling CI#132098

Merged
Fidget-Spinner merged 17 commits into
python:mainfrom
Fidget-Spinner:fix-double-redeclaration
Apr 5, 2025
Merged

gh-132132: Upgrade LLVM on tail calling CI#132098
Fidget-Spinner merged 17 commits into
python:mainfrom
Fidget-Spinner:fix-double-redeclaration

Conversation

@Fidget-Spinner

@Fidget-Spinner Fidget-Spinner commented Apr 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

No description provided.

@bedevere-app bedevere-app Bot mentioned this pull request Apr 4, 2025
Closed
@Fidget-Spinner

Fidget-Spinner commented Apr 4, 2025

Copy link
Copy Markdown
Member Author

@chris-eibl can I take some of your time to request a review please :)?

@Fidget-Spinner Fidget-Spinner changed the title gh-131591: Fix double redeclaration on MSVC+clang-cl Apr 4, 2025
@chris-eibl

chris-eibl commented Apr 4, 2025

Copy link
Copy Markdown
Member

Oh no, it's back again - this was already a back and forth (#130040).

I think, your fix back then (exclude __clang__)

#if defined(MS_WINDOWS) && !defined(__clang__)
#define _GENERATE_DEBUG_SECTION_WINDOWS(name)                       \
   _Pragma(Py_STRINGIFY(section(Py_STRINGIFY(name), read, write))) \
   __declspec(allocate(Py_STRINGIFY(name)))

should stay: clangcl might not understand __declspec(allocate(...)).

But then, no GENERATE_DEBUG_SECTION is in effect, because

#if defined(__linux__) && (defined(__GNUC__) || defined(__clang__))
#define _GENERATE_DEBUG_SECTION_LINUX(name) \
   __attribute__((section("." Py_STRINGIFY(name))))               \
   __attribute__((used))

will never fire due to __linux__. Maybe rewrite that to

#if (defined(__linux__) && defined(__GNUC__)) || (defined(MS_WINDOWS) && defined(__clang__))
#define _GENERATE_DEBUG_SECTION_LINUX(name) \
   __attribute__((section("." Py_STRINGIFY(name))))               \
   __attribute__((used))

clang defines __GNUC__, so I dropped the first __clang__

@chris-eibl

chris-eibl commented Apr 4, 2025

Copy link
Copy Markdown
Member

Ah - while I was typing you already had the same idea :)

@Fidget-Spinner

Fidget-Spinner commented Apr 4, 2025

Copy link
Copy Markdown
Member Author

Well I have absolutely no clue what's going on with Windows here. Will sleep on it and let someone else take over. In the meantime, will just use this PR to upgrade the LLVM version.

@chris-eibl chris-eibl mentioned this pull request Apr 5, 2025
Merged
@chris-eibl

chris-eibl commented Apr 5, 2025

Copy link
Copy Markdown
Member

@Fidget-Spinner: I've created #132112 to fix GENERATE_DEBUG_SECTION for clangcl on Windows. CI is green. Feel free to incorporate it in this PR if you want - I'll then just close it.

@Fidget-Spinner Fidget-Spinner changed the title gh-131591: Upgrade LLVM on tail calling, and fix MSVC Apr 5, 2025
@Fidget-Spinner Fidget-Spinner changed the title Upgrade LLVM on tail calling CI Apr 5, 2025
@Fidget-Spinner

Fidget-Spinner commented Apr 5, 2025

Copy link
Copy Markdown
Member Author

@chris-eibl can you please send me an email (it's listed on my GH profile), I want to ask some questions.

@chris-eibl

chris-eibl commented Apr 5, 2025

Copy link
Copy Markdown
Member

Email sent :)

@Fidget-Spinner Fidget-Spinner merged commit 92fb949 into python:main Apr 5, 2025
@Fidget-Spinner Fidget-Spinner deleted the fix-double-redeclaration branch April 5, 2025 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chris-eibl chris-eibl chris-eibl left review comments

@ezio-melotti ezio-melotti Awaiting requested review from ezio-melotti ezio-melotti is a code owner

@hugovk hugovk Awaiting requested review from hugovk hugovk is a code owner

@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

+1 more reviewer

@taegyunkim taegyunkim taegyunkim left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Fidget-Spinner @chris-eibl @taegyunkim @riteshwadkar