Skip to content

[Serialization] Use full target architectures for swiftmodule files #21053

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

Merged
merged 1 commit into from
Dec 6, 2018

Conversation

jrose-apple
Copy link
Contributor

Previously these used the same "major architecture" names that the #if os(...) query accepts, but that can be a problem when building for, say, both armv7 and armv7s, even if the content is "the same".

For a transition period where external build tools are involved, the compiler will look for "arm.swiftmodule" if it fails to find "armv7.swiftmodule" or any other 32-bit ARM architecture. No other Apple platform architectures are affected, and AFAIK no one's using the architecture-based layout on Linux or any other platforms.

rdar://problem/45174692

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@jrose-apple jrose-apple requested a review from compnerd December 5, 2018 20:01
@swift-ci
Copy link
Contributor

swift-ci commented Dec 5, 2018

Build failed
Swift Test Linux Platform
Git Sha - 3fea4f4a346a4593c6c2a27ca30fb02702e08280

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

@jrose-apple
Copy link
Contributor Author

@swift-ci Please clean test Linux

@@ -142,6 +142,23 @@ bool SerializedModuleLoader::maybeDiagnoseArchitectureMismatch(
return true;
}

static std::pair<llvm::SmallString<16>, llvm::SmallString<16>>
getArchSpecificModuleFileNames(StringRef archName) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not pass along the Triple rather than StringRef? It feels like it would be nicer to derive that from the triple rather than the architecture name specifically. We could have a boolean flag for whether it should do the fallback or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that actually makes the call sites nicer, and as a helper function that's the important thing.

archDocFile += file_types::getExtension(file_types::TY_SwiftModuleDocFile);
}
// FIXME: We used to use "major architecture" names for these files---the
// names checked in "#if os(...)". Fall back to that name in the one case
Copy link
Member

Choose a reason for hiding this comment

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

Did you really mean #if os(...) or do you mean #if arch(...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks!

Previously these used the same "major architecture" names that the
`#if os(...)` query accepts, but that can be a problem when building
for, say, both armv7 and armv7s, even if the content is "the same".

For a transition period where external build tools are involved, the
compiler will look for "arm.swiftmodule" if it fails to find
"armv7.swiftmodule" or any other 32-bit ARM architecture. No other
Apple platform architectures are affected, and AFAIK no one's using
the architecture-based layout on Linux or any other platforms.

rdar://problem/45174692
@jrose-apple
Copy link
Contributor Author

@swift-ci Please smoke test

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test source compatibility

@jrose-apple jrose-apple merged commit 06f3c11 into swiftlang:master Dec 6, 2018
@jrose-apple jrose-apple deleted the right-on-target branch December 6, 2018 21:31
jrose-apple added a commit to jrose-apple/swift that referenced this pull request Dec 6, 2018
…wiftlang#21053)

Previously these used the same "major architecture" names that the
`#if os(...)` query accepts, but that can be a problem when building
for, say, both armv7 and armv7s, even if the content is "the same".

For a transition period where external build tools are involved, the
compiler will look for "arm.swiftmodule" if it fails to find
"armv7.swiftmodule" or any other 32-bit ARM architecture. No other
Apple platform architectures are affected, and AFAIK no one's using
the architecture-based layout on Linux or any other platforms.

rdar://problem/45174692
(cherry picked from commit 06f3c11)
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.

5 participants