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

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

Merged
merged 1 commit into from Oct 3, 2019

Conversation

@harlanhaskins
Copy link
Collaborator

commented Oct 1, 2019

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 requested review from brentdax, edymtt and jrose-apple Oct 1, 2019
@harlanhaskins harlanhaskins force-pushed the harlanhaskins:target-acquired branch 2 times, most recently from a2b3bbf to dea6d75 Oct 1, 2019
@harlanhaskins harlanhaskins force-pushed the harlanhaskins:target-acquired branch from dea6d75 to 7329481 Oct 1, 2019
@harlanhaskins

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 1, 2019

@swift-ci please test

@edymtt edymtt self-requested a review Oct 1, 2019
Copy link
Contributor

left a comment

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

lib/Frontend/ModuleInterfaceLoader.cpp Outdated Show resolved Hide resolved
// 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);

This comment has been minimized.

Copy link
@brentdax

brentdax Oct 1, 2019

Contributor

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

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

Build failed
Swift Test OS X Platform
Git Sha - 7329481

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

Build failed
Swift Test Linux Platform
Git Sha - 7329481

@edymtt
edymtt approved these changes Oct 2, 2019
…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 harlanhaskins force-pushed the harlanhaskins:target-acquired branch from 7329481 to c090816 Oct 2, 2019
@harlanhaskins

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 2, 2019

@swift-ci please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

Build failed
Swift Test Linux Platform
Git Sha - 7329481

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

Build failed
Swift Test OS X Platform
Git Sha - 7329481

@harlanhaskins

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 3, 2019

LLDB failure is unrelated

@swift-ci please test Linux

@harlanhaskins harlanhaskins merged commit ce531be into apple:master Oct 3, 2019
4 checks passed
4 checks passed
Swift Test Linux Platform No test results found.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform No test results found.
Details
Swift Test OS X Platform (smoke test)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.