Skip to content

[Explicit Module Builds] Prevent SerializedModuleLoader from running in Explicit Module Build mode. #32903

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
Jul 22, 2020

Conversation

artemcm
Copy link
Contributor

@artemcm artemcm commented Jul 15, 2020

In order to avoid accidentally implicitly loading modules that are expected but were not provided as explicit inputs.

  • Rename SerializedModuleLoader into ImplicitSerializedModuleLoader
  • Use either ImplicitSerializedModuleLoader or ExplicitSwiftModuleLoader for loading of partial modules, depending on whether we are in Explicit Module Build or Implicit Module Build mode.

@artemcm artemcm requested a review from nkcsgexi July 15, 2020 17:20
@artemcm artemcm force-pushed the NoImplicitWhenExplicit branch from a9910d6 to 7cefbf9 Compare July 15, 2020 17:21
@artemcm
Copy link
Contributor Author

artemcm commented Jul 15, 2020

@swift-ci test

Copy link
Contributor

@nkcsgexi nkcsgexi left a comment

Choose a reason for hiding this comment

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

Makes sense to me! Only a minor suggestion.

@artemcm artemcm force-pushed the NoImplicitWhenExplicit branch from 7cefbf9 to 62286f4 Compare July 15, 2020 18:01
@artemcm
Copy link
Contributor Author

artemcm commented Jul 15, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 7cefbf9419b65142c85cb9055dfd1265ed14ef3d

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 7cefbf9419b65142c85cb9055dfd1265ed14ef3d

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 7cefbf9419b65142c85cb9055dfd1265ed14ef3d

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 7cefbf9419b65142c85cb9055dfd1265ed14ef3d

@artemcm
Copy link
Contributor Author

artemcm commented Jul 15, 2020

Please test with following PR:
swiftlang/llvm-project#1465

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 62286f40b22c3f25e9d7f4d714ed78831db36493

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 62286f40b22c3f25e9d7f4d714ed78831db36493

@artemcm artemcm force-pushed the NoImplicitWhenExplicit branch from 62286f4 to d54460d Compare July 16, 2020 16:54
@artemcm
Copy link
Contributor Author

artemcm commented Jul 16, 2020

Please test with following PR:
swiftlang/llvm-project#1465

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 62286f40b22c3f25e9d7f4d714ed78831db36493

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 62286f40b22c3f25e9d7f4d714ed78831db36493

@artemcm
Copy link
Contributor Author

artemcm commented Jul 16, 2020

Please test with following PR:
swiftlang/llvm-project#1465

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - d54460d87055ec33bfb594590ec3a1e34b5c1a50

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - d54460d87055ec33bfb594590ec3a1e34b5c1a50

@artemcm artemcm force-pushed the NoImplicitWhenExplicit branch 2 times, most recently from 3877689 to 5836a7a Compare July 17, 2020 03:04
@artemcm
Copy link
Contributor Author

artemcm commented Jul 17, 2020

Please test with following PR:
swiftlang/llvm-project#1465

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - d54460d87055ec33bfb594590ec3a1e34b5c1a50

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - d54460d87055ec33bfb594590ec3a1e34b5c1a50

@artemcm artemcm force-pushed the NoImplicitWhenExplicit branch from 5836a7a to 8cbba04 Compare July 17, 2020 19:57
@artemcm
Copy link
Contributor Author

artemcm commented Jul 17, 2020

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 5836a7a69e4cf3fbb2c4e2966f2b8456a8d4992c

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 5836a7a69e4cf3fbb2c4e2966f2b8456a8d4992c

@artemcm
Copy link
Contributor Author

artemcm commented Jul 21, 2020

Please test with following PR:
swiftlang/llvm-project#1465

@swift-ci Please smoke test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 4f8cd28e266e4900da687790328017a95838c858

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 4f8cd28e266e4900da687790328017a95838c858

@artemcm artemcm force-pushed the NoImplicitWhenExplicit branch from 9c294fd to 7fe842e Compare July 21, 2020 15:57
@artemcm
Copy link
Contributor Author

artemcm commented Jul 21, 2020

Please test with following PR:
swiftlang/llvm-project#1465

@swift-ci Please smoke test

@artemcm artemcm force-pushed the NoImplicitWhenExplicit branch from 7fe842e to c1da91d Compare July 21, 2020 19:46
@artemcm
Copy link
Contributor Author

artemcm commented Jul 21, 2020

Please test with following PR:
swiftlang/llvm-project#1465

@swift-ci Please smoke test

…in Explicit Module Build mode.

In order to avoid accidentally implicitly loading modules that are expected but were not provided as explicit inputs.

- Use either SerializedModuleLoader or ExplicitSwiftModuleLoader for loading of partial modules, depending on whether we are in Explicit Module Build or Implicit Module Build mode.
@artemcm artemcm force-pushed the NoImplicitWhenExplicit branch from c1da91d to 140fd73 Compare July 22, 2020 16:01
@artemcm
Copy link
Contributor Author

artemcm commented Jul 22, 2020

Please test with following PR:
swiftlang/llvm-project#1465

@swift-ci Please smoke test

@artemcm artemcm merged commit 95c8391 into swiftlang:master Jul 22, 2020
@compnerd
Copy link
Member

This seems to have caused a regression on the Windows builder:
https://ci-external.swift.org/job/oss-swift-windows-x86_64-vs2019/2188/

Could you please take a look @artemcm?

CC: @shahmishal - seems that the CI didn't trigger?

@artemcm
Copy link
Contributor Author

artemcm commented Jul 22, 2020

This seems to have caused a regression on the Windows builder:
https://ci-external.swift.org/job/oss-swift-windows-x86_64-vs2019/2188/

Could you please take a look @artemcm?

CC: @shahmishal - seems that the CI didn't trigger?

@compnerd , what is the path structure for Swift.swiftmodule on Windows? I think the change to lit.cfg here just needs to be updated with a windows special case.

@compnerd
Copy link
Member

It is identical to Linux, just with windows instead of linux in the resource directory

@artemcm
Copy link
Contributor Author

artemcm commented Jul 22, 2020

It is identical to Linux, just with windows instead of linux in the resource directory

Got it, I'll prepare a patch right away.

compnerd added a commit to compnerd/apple-swift that referenced this pull request Jul 23, 2020
Repair the Windows build after swiftlang#32903
3405691582 added a commit to 3405691582/swift that referenced this pull request Jul 23, 2020
In swiftlang#32903, a substitution pseudovariable was introduced to refer to the
path containing Swift.swiftmodule. Since builds on Linux put this file
in a path with a subdirectory named after the architecture name -- and
presumably builds on mac OS do not, a conditional was required to
distinguish these two cases.

However, builds on OpenBSD for example also put Swift.swiftmodule in a
similar subdirectory like Linux. Thus, invert the sense of this
conditional to encompass both Linux, OpenBSD, and make mac OS the
specific case, which it is here.
Comment on lines -429 to +433
// [Note: ModuleInterfaceLoader-defer-to-SerializedModuleLoader]), which
// [Note: ModuleInterfaceLoader-defer-to-ImplicitSerializedModuleLoader]), which
// is interpreted by the overarching loader as a command to use the
// SerializedModuleLoader. If we failed to find a .swiftmodule, this falls
// ImplicitSerializedModuleLoader. If we failed to find a .swiftmodule, this falls
Copy link
Contributor

Choose a reason for hiding this comment

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

Please be careful about updating comments. There are a couple of issues here:

  1. This is exceeding the line length limit now.
  2. This broke a reference to the note, since the reference now has a new name but the note's name in ModuleInterfaceLoader.cpp is not changed in this PR.

I'm going to fix these two issues in an upcoming PR, it's not a big deal; just something to be careful about.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR with fix: #33124

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