Skip to content

[ModuleInterfaces] Combine the normalized target triple into the cache hash #27469

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
Oct 3, 2019

Conversation

harlanhaskins
Copy link
Contributor

Previously, we'd combine just the target architecture, and rely on the
fact that the .swiftinterface is in a reasonably-target-specific
subdirectory to include enough entropy to avoid hash collisions. But in
the presence of a VFS or if two targets are sharing the same
.swiftinterface file (which can sometimes happen in tests), they will
collide since the hash only includes architecture.

Instead, use the same normalization that the serialized module loader
uses, and serialize the normalized target triple instead.

Fixes rdar://55881335

@harlanhaskins harlanhaskins force-pushed the target-acquired branch 2 times, most recently from a2b3bbf to dea6d75 Compare October 1, 2019 21:12
@harlanhaskins
Copy link
Contributor Author

@swift-ci please test

@edymtt edymtt self-requested a review October 1, 2019 21:55
Copy link
Contributor

@beccadax beccadax left a comment

Choose a reason for hiding this comment

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

Found a typo in a comment, and I'm not sure the behavior will be correct for Android targets, but otherwise LGTM.

// across targets. Note that we this normalization explicitly doesn't
// include the minimum deployment target (e.g. the '12.0' in 'ios12.0').
auto normalizedTargetTriple =
getTargetSpecificModuleTriple(SubInvocation.getLangOptions().Target);
Copy link
Contributor

Choose a reason for hiding this comment

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

@compnerd I notice that Android drops the API version when it normalizes the target triple. Will that behavior be appropriate here?

@swift-ci
Copy link
Contributor

swift-ci commented Oct 1, 2019

Build failed
Swift Test OS X Platform
Git Sha - 73294815746a581d70218caeb2f08727274e6a35

@swift-ci
Copy link
Contributor

swift-ci commented Oct 1, 2019

Build failed
Swift Test Linux Platform
Git Sha - 73294815746a581d70218caeb2f08727274e6a35

…e hash

Previously, we'd combine just the target architecture, and rely on the
fact that the .swiftinterface is in a reasonably-target-specific
subdirectory to include enough entropy to avoid hash collisions. But in
the presence of a VFS or if two targets are sharing the same
.swiftinterface file (which can sometimes happen in tests), they will
collide since the hash only includes architecture.

Instead, use the same normalization that the serialized module loader
uses, and serialize the normalized target triple instead.

Fixes rdar://55881335
@harlanhaskins
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2019

Build failed
Swift Test Linux Platform
Git Sha - 73294815746a581d70218caeb2f08727274e6a35

@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2019

Build failed
Swift Test OS X Platform
Git Sha - 73294815746a581d70218caeb2f08727274e6a35

@harlanhaskins
Copy link
Contributor Author

LLDB failure is unrelated

@swift-ci please test Linux

@harlanhaskins harlanhaskins merged commit ce531be into swiftlang:master Oct 3, 2019
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