[Explicit Module Builds] Add support for versioned PCMs #126

artemcm · 2020-06-13T02:07:47Z

There exists a strict limitation that a compatible PCM must have the exact same target version with the loading module (either a Swift module or another PCM). This means that each PCM may have to be built multiple times, once for each unique (SDK, architecture, toolchain) tuple of some loading module.

This PR introduces support for generating versioned PCMs for each loading module.
Now there is more state that we needed to keep track of when generating jobs for module dependencies: a cache of which modules have already been built). This state, along with the Inter-Module Dependency Graph, now live in the ExplicitModuleBuildHandler, which is constructed on-demand by the driver in the explicit module build mode. The handler is responsible for generating dependency build jobs and for vending out dependency modules as inputs/command-line options whenever the driver creates a compile job in Explicit Module Build Mode.

This PR depends on Xi's work in: swiftlang/swift#32304
Resolves rdar://problem/63944640

artemcm · 2020-06-13T02:08:58Z

Until we merge Xi's PR in apple/swift, the tests here will not pass. We can orchestrate the tandem submission once this review is complete.

nkcsgexi

Cool! I left only some minor comments.

Sources/SwiftDriver/Explicit Module Builds/ExplicitModuleBuildHandler.swift

DougGregor · 2020-06-15T20:12:15Z

Sources/SwiftDriver/Explicit Module Builds/ClangModuleBuildJobCache.swift

+struct ClangModuleBuildJobCache {
+
+  /// As tuples cannot be Hashable, wrap (ModuleDependencyId, [String]) into a struct pair
+  private struct ModuleArgumentPair : Hashable {


This makes me wonder whether there is a more fundamental problem here with the ModuleDependencyId abstraction. Right now, it assumes that the Clang modules are uniquely identified by their name within a Swift compiler job, but that's not true. Rather, they are uniquely identified by their name and by the extra build args needed to produce that variant of the model. How about pulling that into ModuleDependencyId directly:

enum ModuleDependencyId: Hashable { case swift(String) case clang(moduleName: String, buildArguments: [String]) }

then ClangModuleBuildJobCache would not have to exist and, more importantly, it wouldn't be possible to make the mistake of using the "wrong" Clang module or getting them confused, because ModuleDependencyId provides the right notion of identity. (This probably should be reflected in the JSON output from the dependency scanner, too).

Then the logic of "discovering" which clang module versions must be built would need to be moved to the dependency scanner. The dependency scanner would then need to either mutate the existing graph to have versioned PCMs or generate a versioned-PCM graph from the start, in a way that is similar to how "discovery" is done here.

It seems to me like the dependency scanner should be generating the versioned-PCM graph, because that's an accurate representation of the dependencies. Right now, the graph that comes out of the dependency scanner does not accurately represent the modules that need to be built.

I have experimented with constructing a graph from the compiler to include all versioned PCMs. The reason for me to not choose that alternative is that all transitive PCM dependencies have to be pulled into directive ones so that they can have unique command line arguments, which is not an accurate representation either.

The uniqued command line arguments are what make them unique; that's why I'm advocating for it to be part of module identity. Each module node in the dependency graph should correspond to exactly one module file that needs to be built. Anything else leaves too much interpretation to the client, and we'd rather keep the clients simple.

Okay, I understand the "transitive dependency" issue now. This is fine.

DougGregor · 2020-06-15T20:31:49Z

Tests/SwiftDriverTests/Inputs/ExplicitModuleDependencyBuildInputs.swift

@@ -39,6 +39,12 @@ enum ModuleDependenciesInputs {
          ],
          "details": {
            "swift": {
+              "extraPcmArgs": [
+                "-Xcc",
+                "-target",


I think we should use -target x86_64-apple-macosx10.15 here, as a Swift command-line argument, rather than avoiding Swift's option handling with -Xcc. Swift will translate that down to the appropriate -target for Clang automatically.

To explain why we need those -Xcc, the way Swift compiler emits those PCM build arguments is to solely reply on extra Clang arguments to instantiate a Clang instance rather than inferring any of them from Swift arguments. We add a new Swift front-end flag -only-use-extra-clang-opts to ensure no Swift arguments are translated. This is why we have to use -Xcc here.

This makes me a little nervous, because a bunch of the setup logic for the Clang importer looks at the Swift options. Is all of that meant to be disabled? Should we change the way we emit PCM's from Swift entirely?

Is all of that meant to be disabled?

No, they will be preserved as long as these flags are specified in the scanning action. When we are collecting these Clang arguments, they are collecting the post-configuration arguments in the scanning action.

Should we change the way we emit PCM's from Swift entirely?

hmm, It seems to me the existing mechanism makes more sense because generating Swift arguments always translate to the same set of Clang arguments is another layer of complexity.

Another reason for using Clang arguments here is that we are collecting them already in ClangImporter.cpp. Re-using these arguments directly seems to me to be less error-prone.

artemcm · 2020-06-17T17:16:40Z

@swift-ci please test

artemcm · 2020-06-18T16:20:38Z

@swift-ci please test

There exists a strict limitation that a compatible PCM must have the exact same target version with the loading module (either a Swift module or another PCM). This means that each PCM may have to be built multiple times, once for each unique (SDK, architecture, toolchain) tuple of some loading module. This PR introduces support for generating versioned PCMs for each loading module. Because now there is more state that we needed to keep track of when generating jobs for module dependencies, this state, along with the Inter-Module Dependency Graph, now live in the `ExplicitModuleBuildHandler`, which is constructed on-demand by the driver in the explicit module build mode. The handler is responsible for generating dependency build jobs and for vending out dependency modules as inputs/command-line options whenever the driver creates a compile job in Explicit Module Build Mode. Because this PR rewrites/refactores the majority of existing Explicit Module Build code, I tooke the opportunity to re-organize some of the files as well. Resolves rdar://problem/63944640

artemcm · 2020-06-18T17:27:53Z

@swift-ci please test

artemcm · 2020-06-18T17:28:15Z

@swift-ci please test

artemcm · 2020-06-18T17:35:28Z

@swift-ci please test

DougGregor · 2020-06-18T18:16:38Z

🚀

artemcm requested review from DougGregor and nkcsgexi June 13, 2020 02:07

nkcsgexi approved these changes Jun 15, 2020

View reviewed changes

artemcm force-pushed the EMB-VersionedPCM-Refactor branch from f535d94 to 9f199b1 Compare June 15, 2020 20:10

DougGregor reviewed Jun 15, 2020

View reviewed changes

artemcm force-pushed the EMB-VersionedPCM-Refactor branch 2 times, most recently from d0dd768 to 89a2c61 Compare June 17, 2020 17:16

artemcm force-pushed the EMB-VersionedPCM-Refactor branch from 89a2c61 to 1629738 Compare June 18, 2020 17:27

artemcm merged commit b1d4dff into swiftlang:master Jun 18, 2020

artemcm deleted the EMB-VersionedPCM-Refactor branch June 18, 2020 17:52

[Explicit Module Builds] Add support for versioned PCMs #126

[Explicit Module Builds] Add support for versioned PCMs #126

Uh oh!

Conversation

artemcm commented Jun 13, 2020

Uh oh!

artemcm commented Jun 13, 2020

Uh oh!

nkcsgexi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemcm commented Jun 17, 2020

Uh oh!

artemcm commented Jun 18, 2020

Uh oh!

artemcm commented Jun 18, 2020

Uh oh!

artemcm commented Jun 18, 2020

Uh oh!

artemcm commented Jun 18, 2020

Uh oh!

DougGregor commented Jun 18, 2020

Uh oh!

Uh oh!