Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from
Apr 5, 2025

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Apr 4, 2025

No description provided.

@Fidget-Spinner
Copy link
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 gh-131591: Upgrade LLVM on tail calling, and fix MSVC Apr 4, 2025
@chris-eibl
Copy link
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
Copy link
Member

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

const char* secname = "_PyRuntime";
#else
const char* secname = "PyRuntime";
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Use secname instead of "PyRuntime" below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah oops 🤦 thanks 3AM things.

Comment on lines +117 to +118
export PATH="/usr/local/opt/llvm/bin:$PATH"
export PATH="/opt/homebrew/opt/llvm/bin:$PATH"

Choose a reason for hiding this comment

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

wonder whether
eval "$(brew shellenv)"
is just enough for both intel and arm mac

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. Let me try that later.

@Fidget-Spinner
Copy link
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
Copy link
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 Upgrade LLVM on tail calling CI Apr 5, 2025
@Fidget-Spinner Fidget-Spinner changed the title Upgrade LLVM on tail calling CI gh-132132: Upgrade LLVM on tail calling CI Apr 5, 2025
@Fidget-Spinner
Copy link
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
Copy link
Member

Email sent :)

@Fidget-Spinner Fidget-Spinner merged commit 92fb949 into python:main Apr 5, 2025
44 checks passed
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants