-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Fix wrong attribute ordering in the runtime again #62460
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
Conversation
This is required because of a change in clang 15, where attributes need to come after `extern "C"`: https://reviews.llvm.org/D126061 llvm/llvm-project#56077 Fixes swiftlang#61468
I just noticed that this was reverted again in #62337 last week, but only on the rebranch, not on main. |
Ah, I've been meaning to look into why this was failing even with @compnerd's changes. Thanks for finding that! Is building the runtime with a release clang something that we support? I would prefer the fix to be moving |
I don't think it's the default, but these headers are also included when building the Swift compiler itself, so they break that build with a host clang 15 too. Moving |
When you say "I don't think it's the default", what do you mean there? I had a quick look at all includes of
So the only problematic one is |
I mean that the runtime is built by our forked clang 13 by default, but there is an option to build it with the same host clang used to build the Swift compiler, which could be clang 15.
No, that was the file that wasn't building for me. If you'd rather modify the Swift compiler to not use these headers and trust that clang 15 will not be used to build the runtime, please go ahead and slap up that pull and I'll be happy to test that out instead. 😄 |
|
@bnbarham, why don't we get this fix in for now, then you can figure out how to get your fix in later. |
@swift-ci please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that I would prefer that we handle this similarly to the other libraries (e.g. CoreFoundation). Can we introduce a SWIFT_BEGIN_DECLS
and SWIFT_END_DECLS
instead?
Didn't your previous attempt to do that, #61476, fail? I'm not interested in debugging that: I'd rather get this simple temporary fix in again, that we know gets the Swift compiler and runtime building again with clang 15, and then you guys can work on one of those more permanent fixes if you like and revert this once you get one of those working. @compnerd, what do you think? |
I think that the issue with my patch was that I was trying to do it pervasively. If we are doing it for just these in particular, it should be pretty easy to do. If you want, I can try to put that up tomorrow. |
Sounds good, I will test out your patch tomorrow with clang 15, since the CI doesn't test that. |
@compnerd, if you don't have time for that |
Sorry, been trying to repair the Windows builds (there have been a number of regressions). I'm still not particularly happy with this approach, but I agree that we should get this in before the branch for 5.8. I'm leaving it unmerged for now as @bnbarham was going to check if these headers are fully internal, but we need to get this in tomorrow morning at the latest. |
|
@bnbarham, feel free to close this and put up your own version instead, as I'm unfamiliar with those new issues you both are raising. |
@buttaface I've merged that one |
This is required because of a change in clang 15, where attributes need to come after
extern "C"
:https://reviews.llvm.org/D126061
llvm/llvm-project#56077
Fixes #61468
This is still needed because @compnerd's fix only allows changing attribute ordering but doesn't affect this recent @martinboehme change (plus Saleem's change didn't make it into the final clang 15 patch release).
Pinging @eeckstein, as this is your original commit with one whitespace removed, and @bnbarham, since you reverted this last week.