Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

finagolfin
Copy link
Member

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.

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
@finagolfin
Copy link
Member Author

I just noticed that this was reverted again in #62337 last week, but only on the rebranch, not on main.

@bnbarham
Copy link
Contributor

bnbarham commented Dec 8, 2022

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 SWIFT_RUNTIME_EXPORT to the beginning of the decl for both main and rebranch (which is only possible with comp nerd's changes).

@finagolfin
Copy link
Member Author

Is building the runtime with a release clang something that we support?

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 SWIFT_RUNTIME_EXPORT to the beginning may be the fix someday, but since Saleem's change didn't make it into the last clang 15 patch release, we're probably stuck with this workaround until we're sure clang 15 is no longer used, ie years down the line.

@bnbarham
Copy link
Contributor

bnbarham commented Dec 8, 2022

Is building the runtime with a release clang something that we support?

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.

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 swift/Runtime and I get:

SwiftDemangle/MangleHack.cpp
21:#include "swift/Runtime/Portability.h"

Demangling/Errors.cpp
40:#include "swift/Runtime/Atomic.h"

LLVMPasses/LLVMARCOpts.h
16:#include "swift/Runtime/Config.h"

LLVMPasses/ARCEntryPointBuilder.h
18:#include "swift/Runtime/Config.h"
19:#include "swift/Runtime/RuntimeFnWrappersGen.h"

IRGen/GenCall.cpp
23:#include "swift/Runtime/Config.h"

IRGen/IRGenModule.cpp
29:#include "swift/Runtime/RuntimeFnWrappersGen.h"
30:#include "swift/Runtime/Config.h"
1100:#include "swift/Runtime/RuntimeFunctions.def"

IRGen/IRGenModule.h
1412:#include "swift/Runtime/RuntimeFunctions.def"

IRGen/GenDecl.cpp
33:#include "swift/Runtime/HeapObject.h"

So the only problematic one is IRGen/GenDecl.cpp and the others seem like whatever is being used should probably be pulled out of Runtime. In any case, as far as I can tell IRGen/GenDecl.cpp doesn't actually use HeapObject.h anywhere so we should just be able to remove it. Are there any other uses you're concerned about?

@finagolfin
Copy link
Member Author

When you say "I don't think it's the default", what do you mean there?

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.

IRGen/GenDecl.cpp doesn't actually use HeapObject.h anywhere so we should just be able to remove it. Are there any other uses you're concerned about?

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
Copy link
Contributor

bnbarham commented Dec 9, 2022

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.

#62497

@finagolfin
Copy link
Member Author

@bnbarham, why don't we get this fix in for now, then you can figure out how to get your fix in later.

@bnbarham
Copy link
Contributor

I'm okay with that as long as @compnerd and @mikeash have no objections. This is what was there already, I had just reverted it so...

@bnbarham
Copy link
Contributor

@swift-ci please test

Copy link
Member

@compnerd compnerd left a 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?

@finagolfin
Copy link
Member Author

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?

@compnerd
Copy link
Member

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.

@finagolfin
Copy link
Member Author

Sounds good, I will test out your patch tomorrow with clang 15, since the CI doesn't test that.

@finagolfin
Copy link
Member Author

@compnerd, if you don't have time for that SWIFT_BEGIN_DECLS patch now, why don't we get this in before the 5.8 branch and you can get that alternate patch in later?

@finagolfin
Copy link
Member Author

@bnbarham, no response from @compnerd lately- he's probably busy with other issues- why don't you merge this before the 5.8 branch tomorrow and get it building with clang 15 again, then we can always replace this pull later if wanted?

@compnerd
Copy link
Member

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
Copy link
Contributor

I'm leaving it unmerged for now as @bnbarham was going to check if these headers are fully internal

stdlib/public/SwiftShims/swift/shims/Visibility.h is definitely part of the release, but the other headers are internal. If we want to avoid that, we could define the new macros in include/swift/Runtime/Config.h just under the #include "swift/shims/Visibility.h" include instead? Then after fixing the rebranch conflicts caused by this, I'll put up a PR to just re-order them on rebranch instead (which will then merge back into main when rebranch is merged).

@finagolfin
Copy link
Member Author

@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.

@bnbarham
Copy link
Contributor

#62667 and I'll change #62497 to be on rebranch after fixing the conflicts.

@bnbarham
Copy link
Contributor

@buttaface I've merged that one

@bnbarham bnbarham closed this Dec 19, 2022
@finagolfin finagolfin deleted the clang-15 branch December 19, 2022 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Swift compiler cannot be built with clang 15 on linux/Android
4 participants