From 691f82fca3bec112e72340544beefda359b53901 Mon Sep 17 00:00:00 2001 From: Doug Schaefer Date: Tue, 12 Nov 2024 22:31:56 -0500 Subject: [PATCH 1/5] Mark targets on Windows as static except those in DLLs. Reverts the prevous attempt at fixing symbol export errors in EXEs. Instead, for Windows triples, we mark all targets as needing to be static. Then, after the products are planned, walks throught the staticTargets of DLL products and marks as not static. This allows executables in packages to import modules in local DLL products as exporting. This is a temporary fix until we add the ability for targets to explicity depend on local products. --- .../SwiftModuleBuildDescription.swift | 67 +++---------------- .../LLBuildManifestBuilder+Swift.swift | 19 +----- .../Build/BuildPlan/BuildPlan+Product.swift | 13 +--- Sources/Build/BuildPlan/BuildPlan.swift | 13 +++- Tests/BuildTests/WindowsBuildPlanTests.swift | 53 +++------------ 5 files changed, 31 insertions(+), 134 deletions(-) diff --git a/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift b/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift index 3d7c6f831dc..cfe3adf0fdf 100644 --- a/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift +++ b/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift @@ -137,11 +137,7 @@ public final class SwiftModuleBuildDescription { var modulesPath: AbsolutePath { let suffix = self.buildParameters.suffix - var path = self.buildParameters.buildPath.appending(component: "Modules\(suffix)") - if self.windowsTargetType == .dynamic { - path = path.appending("dynamic") - } - return path + return self.buildParameters.buildPath.appending(component: "Modules\(suffix)") } /// The path to the swiftmodule file after compilation. @@ -278,18 +274,8 @@ public final class SwiftModuleBuildDescription { /// Whether to disable sandboxing (e.g. for macros). private let shouldDisableSandbox: Bool - /// For Windows, we default to static objects and but also create objects - /// that export symbols for DLLs. This allows library targets to be used - /// in both contexts - public enum WindowsTargetType { - case `static` - case dynamic - } - /// The target type. Leave nil for non-Windows behavior. - public let windowsTargetType: WindowsTargetType? - - /// The corresponding target for dynamic library export (i.e., not -static) - public private(set) var windowsDynamicTarget: SwiftModuleBuildDescription? = nil + /// Whether to add -static on Windows to reduce symbol exports + public var isWindowsStatic: Bool /// Create a new target description with target and build parameters. init( @@ -346,13 +332,8 @@ public final class SwiftModuleBuildDescription { observabilityScope: observabilityScope ) - if buildParameters.triple.isWindows() { - // Default to static and add another target for DLLs - self.windowsTargetType = .static - self.windowsDynamicTarget = .init(windowsExportFor: self) - } else { - self.windowsTargetType = nil - } + // default to -static on Windows + self.isWindowsStatic = buildParameters.triple.isWindows() if self.shouldEmitObjCCompatibilityHeader { self.moduleMap = try self.generateModuleMap() @@ -375,31 +356,6 @@ public final class SwiftModuleBuildDescription { try self.generateTestObservation() } - /// Private init to set up exporting version of this module - private init(windowsExportFor parent: SwiftModuleBuildDescription) { - self.windowsTargetType = .dynamic - self.windowsDynamicTarget = nil - self.tempsPath = parent.tempsPath.appending("dynamic") - - // The rest of these are just copied from the parent - self.package = parent.package - self.target = parent.target - self.swiftTarget = parent.swiftTarget - self.toolsVersion = parent.toolsVersion - self.buildParameters = parent.buildParameters - self.macroBuildParameters = parent.macroBuildParameters - self.derivedSources = parent.derivedSources - self.pluginDerivedSources = parent.pluginDerivedSources - self.pluginDerivedResources = parent.pluginDerivedResources - self.testTargetRole = parent.testTargetRole - self.fileSystem = parent.fileSystem - self.buildToolPluginInvocationResults = parent.buildToolPluginInvocationResults - self.prebuildCommandResults = parent.prebuildCommandResults - self.observabilityScope = parent.observabilityScope - self.shouldGenerateTestObservation = parent.shouldGenerateTestObservation - self.shouldDisableSandbox = parent.shouldDisableSandbox - } - private func generateTestObservation() throws { guard target.type == .test else { return @@ -579,16 +535,9 @@ public final class SwiftModuleBuildDescription { args += ["-parse-as-library"] } - switch self.windowsTargetType { - case .static: - // Static on Windows - args += ["-static"] - case .dynamic: - // Add the static versions to the include path - // FIXME: need to be much more deliberate about what we're including - args += ["-I", self.modulesPath.parentDirectory.pathString] - case .none: - break + // Add -static to reduce symbol export count + if self.isWindowsStatic { + args += ["-static"] } // Only add the build path to the framework search path if there are binary frameworks to link against. diff --git a/Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift b/Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift index 73a96bb4140..11668bbe1b3 100644 --- a/Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift +++ b/Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift @@ -53,16 +53,6 @@ extension LLBuildManifestBuilder { ) } else { try self.addCmdWithBuiltinSwiftTool(target, inputs: inputs, cmdOutputs: cmdOutputs) - if let dynamicTarget = target.windowsDynamicTarget { - // Generate dynamic module for Windows - let inputs = try self.computeSwiftCompileCmdInputs(dynamicTarget) - let objectNodes = dynamicTarget.buildParameters.prepareForIndexing == .off ? try dynamicTarget.objects.map(Node.file) : [] - let moduleNode = Node.file(dynamicTarget.moduleOutputPath) - let cmdOutputs = objectNodes + [moduleNode] - try self.addCmdWithBuiltinSwiftTool(dynamicTarget, inputs: inputs, cmdOutputs: cmdOutputs) - self.addTargetCmd(dynamicTarget, cmdOutputs: cmdOutputs) - try self.addModuleWrapCmd(dynamicTarget) - } } self.addTargetCmd(target, cmdOutputs: cmdOutputs) @@ -542,7 +532,7 @@ extension LLBuildManifestBuilder { inputs: cmdOutputs, outputs: [targetOutput] ) - if self.plan.graph.isInRootPackages(target.target, satisfying: target.buildParameters.buildEnvironment), target.windowsTargetType != .dynamic { + if self.plan.graph.isInRootPackages(target.target, satisfying: target.buildParameters.buildEnvironment) { if !target.isTestTarget { self.addNode(targetOutput, toTarget: .main) } @@ -646,11 +636,6 @@ extension SwiftModuleBuildDescription { } public func getLLBuildTargetName() -> String { - let name = self.target.getLLBuildTargetName(buildParameters: self.buildParameters) - if self.windowsTargetType == .dynamic { - return "dynamic." + name - } else { - return name - } + self.target.getLLBuildTargetName(buildParameters: self.buildParameters) } } diff --git a/Sources/Build/BuildPlan/BuildPlan+Product.swift b/Sources/Build/BuildPlan/BuildPlan+Product.swift index c78da860236..10fa8c6bc3b 100644 --- a/Sources/Build/BuildPlan/BuildPlan+Product.swift +++ b/Sources/Build/BuildPlan/BuildPlan+Product.swift @@ -101,18 +101,7 @@ extension BuildPlan { buildProduct.staticTargets = dependencies.staticTargets.map(\.module) buildProduct.dylibs = dependencies.dylibs - buildProduct.objects += try dependencies.staticTargets.flatMap { - if buildProduct.product.type == .library(.dynamic), - case let .swift(swiftModule) = $0, - let dynamic = swiftModule.windowsDynamicTarget, - buildProduct.product.modules.contains(id: swiftModule.target.id) - { - // On Windows, export symbols from the direct swift targets of the DLL product - return try dynamic.objects - } else { - return try $0.objects - } - } + buildProduct.objects += try dependencies.staticTargets.flatMap { try $0.objects } buildProduct.libraryBinaryPaths = dependencies.libraryBinaryPaths buildProduct.availableTools = dependencies.availableTools } diff --git a/Sources/Build/BuildPlan/BuildPlan.swift b/Sources/Build/BuildPlan/BuildPlan.swift index 17f382ec366..2e83aae6d8c 100644 --- a/Sources/Build/BuildPlan/BuildPlan.swift +++ b/Sources/Build/BuildPlan/BuildPlan.swift @@ -537,9 +537,6 @@ public class BuildPlan: SPMBuildCore.BuildPlan { switch buildTarget { case .swift(let target): try self.plan(swiftTarget: target) - if let dynamicTarget = target.windowsDynamicTarget { - try self.plan(swiftTarget: dynamicTarget) - } case .clang(let target): try self.plan(clangTarget: target) } @@ -552,6 +549,16 @@ public class BuildPlan: SPMBuildCore.BuildPlan { // FIXME: We need to find out if any product has a target on which it depends // both static and dynamically and then issue a suitable diagnostic or auto // handle that situation. + + // Ensure modules in Windows DLLs export their symbols + for product in productMap.values where product.product.type == .library(.dynamic) && product.buildParameters.triple.isWindows() { + for target in product.staticTargets { + let targetId: ModuleBuildDescription.ID = .init(moduleID: target.id, destination: product.buildParameters.destination) + if case let .swift(buildDescription) = targetMap[targetId] { + buildDescription.isWindowsStatic = false + } + } + } } public func createAPIToolCommonArgs(includeLibrarySearchPaths: Bool) throws -> [String] { diff --git a/Tests/BuildTests/WindowsBuildPlanTests.swift b/Tests/BuildTests/WindowsBuildPlanTests.swift index 4672091d051..f68cec065f1 100644 --- a/Tests/BuildTests/WindowsBuildPlanTests.swift +++ b/Tests/BuildTests/WindowsBuildPlanTests.swift @@ -71,57 +71,17 @@ final class WindowsBuildPlanTests: XCTestCase { ) let label: String - let dylibPrefix: String - let dylibExtension: String - let dynamic: String switch triple { case Triple.x86_64Windows: label = "x86_64-unknown-windows-msvc" - dylibPrefix = "" - dylibExtension = "dll" - dynamic = "/dynamic" case Triple.x86_64MacOS: label = "x86_64-apple-macosx" - dylibPrefix = "lib" - dylibExtension = "dylib" - dynamic = "" case Triple.x86_64Linux: label = "x86_64-unknown-linux-gnu" - dylibPrefix = "lib" - dylibExtension = "so" - dynamic = "" default: label = "fixme" - dylibPrefix = "" - dylibExtension = "" - dynamic = "" } - let tools: [String: [String]] = [ - "C.exe-\(label)-debug.exe": [ - "/path/to/build/\(label)/debug/coreLib.build/coreLib.swift.o", - "/path/to/build/\(label)/debug/exe.build/main.swift.o", - "/path/to/build/\(label)/debug/objectLib.build/objectLib.swift.o", - "/path/to/build/\(label)/debug/staticLib.build/staticLib.swift.o", - "/path/to/build/\(label)/debug/\(dylibPrefix)DLLProduct.\(dylibExtension)", - "/path/to/build/\(label)/debug/exe.product/Objects.LinkFileList", - ] + (triple.isMacOSX ? [] : [ - // modulewrap - "/path/to/build/\(label)/debug/coreLib.build/coreLib.swiftmodule.o", - "/path/to/build/\(label)/debug/exe.build/exe.swiftmodule.o", - "/path/to/build/\(label)/debug/objectLib.build/objectLib.swiftmodule.o", - "/path/to/build/\(label)/debug/staticLib.build/staticLib.swiftmodule.o", - ]), - "C.DLLProduct-\(label)-debug.dylib": [ - "/path/to/build/\(label)/debug/coreLib.build/coreLib.swift.o", - "/path/to/build/\(label)/debug/dllLib.build\(dynamic)/dllLib.swift.o", - "/path/to/build/\(label)/debug/DLLProduct.product/Objects.LinkFileList", - ] + (triple.isMacOSX ? [] : [ - "/path/to/build/\(label)/debug/coreLib.build/coreLib.swiftmodule.o", - "/path/to/build/\(label)/debug/dllLib.build/dllLib.swiftmodule.o", - ]) - ] - let plan = try await BuildPlan( destinationBuildParameters: mockBuildParameters( destination: .target, @@ -142,11 +102,18 @@ final class WindowsBuildPlanTests: XCTestCase { observabilityScope: observability.topScope ) try llbuild.generateManifest(at: "/manifest") + let commands = llbuild.manifest.commands - for (name, inputNames) in tools { - let command = try XCTUnwrap(llbuild.manifest.commands[name]) - XCTAssertEqual(Set(command.tool.inputs), Set(inputNames.map({ Node.file(.init($0)) }))) + func hasStatic(_ name: String) throws -> Bool { + let tool = try XCTUnwrap(commands[name]?.tool as? SwiftCompilerTool) + return tool.otherArguments.contains("-static") } + + XCTAssertEqual(try hasStatic("C.coreLib-\(label)-debug.module"), false, label) + XCTAssertEqual(try hasStatic("C.dllLib-\(label)-debug.module"), false, label) + XCTAssertEqual(try hasStatic("C.staticLib-\(label)-debug.module"), triple.isWindows(), label) + XCTAssertEqual(try hasStatic("C.objectLib-\(label)-debug.module"), triple.isWindows(), label) + XCTAssertEqual(try hasStatic("C.exe-\(label)-debug.module"), triple.isWindows(), label) } func testWindows() async throws { From 593bd5592e4e12842fbc7559d4771183ad1fde7d Mon Sep 17 00:00:00 2001 From: Doug Schaefer Date: Wed, 13 Nov 2024 20:21:03 -0500 Subject: [PATCH 2/5] Just do the direct targets of the DLL. Doing recursive deps brought back the exported a symbol you imported warnings. --- Sources/Build/BuildPlan/BuildPlan.swift | 2 +- Tests/BuildTests/WindowsBuildPlanTests.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/Build/BuildPlan/BuildPlan.swift b/Sources/Build/BuildPlan/BuildPlan.swift index 2e83aae6d8c..d2b73cdaa85 100644 --- a/Sources/Build/BuildPlan/BuildPlan.swift +++ b/Sources/Build/BuildPlan/BuildPlan.swift @@ -552,7 +552,7 @@ public class BuildPlan: SPMBuildCore.BuildPlan { // Ensure modules in Windows DLLs export their symbols for product in productMap.values where product.product.type == .library(.dynamic) && product.buildParameters.triple.isWindows() { - for target in product.staticTargets { + for target in product.product.modules { let targetId: ModuleBuildDescription.ID = .init(moduleID: target.id, destination: product.buildParameters.destination) if case let .swift(buildDescription) = targetMap[targetId] { buildDescription.isWindowsStatic = false diff --git a/Tests/BuildTests/WindowsBuildPlanTests.swift b/Tests/BuildTests/WindowsBuildPlanTests.swift index f68cec065f1..d62fa83d442 100644 --- a/Tests/BuildTests/WindowsBuildPlanTests.swift +++ b/Tests/BuildTests/WindowsBuildPlanTests.swift @@ -109,7 +109,7 @@ final class WindowsBuildPlanTests: XCTestCase { return tool.otherArguments.contains("-static") } - XCTAssertEqual(try hasStatic("C.coreLib-\(label)-debug.module"), false, label) + XCTAssertEqual(try hasStatic("C.coreLib-\(label)-debug.module"), triple.isWindows(), label) XCTAssertEqual(try hasStatic("C.dllLib-\(label)-debug.module"), false, label) XCTAssertEqual(try hasStatic("C.staticLib-\(label)-debug.module"), triple.isWindows(), label) XCTAssertEqual(try hasStatic("C.objectLib-\(label)-debug.module"), triple.isWindows(), label) From 806b724df40bcba16748596155ecb16270651c43 Mon Sep 17 00:00:00 2001 From: Doug Schaefer Date: Wed, 13 Nov 2024 20:52:20 -0500 Subject: [PATCH 3/5] Ignore warning about exporting an imported symbol. This will happen when a module that exports a symbol for a DLL is also linked into an EXE with modules that import it. --- .../Build/BuildDescription/ProductBuildDescription.swift | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Sources/Build/BuildDescription/ProductBuildDescription.swift b/Sources/Build/BuildDescription/ProductBuildDescription.swift index bb92d95bc16..1b34cbbc177 100644 --- a/Sources/Build/BuildDescription/ProductBuildDescription.swift +++ b/Sources/Build/BuildDescription/ProductBuildDescription.swift @@ -193,6 +193,12 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription // radar://112671586 supress unnecessary warnings if triple.isMacOSX { args += ["-Xlinker", "-no_warn_duplicate_libraries"] + } else if triple.isWindows() { + // locally defined symbol imported + // Occurs when an object file exports a symbol is imported + // into another module in the same executable. + // FIXME: Need to support targets as DLLs + args += ["-Xlinker", "/ignore:4217"] } switch derivedProductType { From b88e7e4594278b8e892d691f077fdb7e2924ec16 Mon Sep 17 00:00:00 2001 From: Doug Schaefer Date: Thu, 14 Nov 2024 12:19:10 -0500 Subject: [PATCH 4/5] Fix broken test with the new ignore link arg on Windows. --- Tests/BuildTests/BuildPlanTests.swift | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Tests/BuildTests/BuildPlanTests.swift b/Tests/BuildTests/BuildPlanTests.swift index 50c19a9d32a..5c93c8f0f3d 100644 --- a/Tests/BuildTests/BuildPlanTests.swift +++ b/Tests/BuildTests/BuildPlanTests.swift @@ -3768,7 +3768,9 @@ final class BuildPlanTests: XCTestCase { result.plan.destinationBuildParameters.toolchain.swiftCompilerPath.pathString, "-L", buildPath.pathString, "-o", buildPath.appending(components: "exe.exe").pathString, - "-module-name", "exe", "-emit-executable", + "-module-name", "exe", + "-Xlinker", "/ignore:4217", + "-emit-executable", "@\(buildPath.appending(components: "exe.product", "Objects.LinkFileList"))", "-target", "x86_64-unknown-windows-msvc", "-g", From 9eedec4fd090c40c78f4438bf23b59a8e7b42600 Mon Sep 17 00:00:00 2001 From: Doug Schaefer Date: Thu, 14 Nov 2024 14:27:33 -0500 Subject: [PATCH 5/5] Renable the exporting imported warning This can hide real user problems. We'll check back later if these warnings are being too much noise. --- .../Build/BuildDescription/ProductBuildDescription.swift | 8 ++------ Tests/BuildTests/BuildPlanTests.swift | 4 +--- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/Sources/Build/BuildDescription/ProductBuildDescription.swift b/Sources/Build/BuildDescription/ProductBuildDescription.swift index 1b34cbbc177..fb78c14baa3 100644 --- a/Sources/Build/BuildDescription/ProductBuildDescription.swift +++ b/Sources/Build/BuildDescription/ProductBuildDescription.swift @@ -193,13 +193,9 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription // radar://112671586 supress unnecessary warnings if triple.isMacOSX { args += ["-Xlinker", "-no_warn_duplicate_libraries"] - } else if triple.isWindows() { - // locally defined symbol imported - // Occurs when an object file exports a symbol is imported - // into another module in the same executable. - // FIXME: Need to support targets as DLLs - args += ["-Xlinker", "/ignore:4217"] } + // We may also need to turn off locally defined symbol imported on Windows + // args += ["-Xlinker", "/ignore:4217"] switch derivedProductType { case .macro: diff --git a/Tests/BuildTests/BuildPlanTests.swift b/Tests/BuildTests/BuildPlanTests.swift index 5c93c8f0f3d..50c19a9d32a 100644 --- a/Tests/BuildTests/BuildPlanTests.swift +++ b/Tests/BuildTests/BuildPlanTests.swift @@ -3768,9 +3768,7 @@ final class BuildPlanTests: XCTestCase { result.plan.destinationBuildParameters.toolchain.swiftCompilerPath.pathString, "-L", buildPath.pathString, "-o", buildPath.appending(components: "exe.exe").pathString, - "-module-name", "exe", - "-Xlinker", "/ignore:4217", - "-emit-executable", + "-module-name", "exe", "-emit-executable", "@\(buildPath.appending(components: "exe.product", "Objects.LinkFileList"))", "-target", "x86_64-unknown-windows-msvc", "-g",