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.

Sign up for GitHub

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

Jump to bottom

Attempt two at solving Windows symbol linking issues #8117

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+33 −134
Open

Attempt two at solving Windows symbol linking issues #8117

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
691f82f
Mark targets on Windows as static except those in DLLs.
dschaefer2 Nov 13, 2024
593bd55
Just do the direct targets of the DLL.
dschaefer2 Nov 14, 2024
806b724
Ignore warning about exporting an imported symbol.
dschaefer2 Nov 14, 2024
b88e7e4
Fix broken test with the new ignore link arg on Windows.
dschaefer2 Nov 14, 2024
9eedec4
Renable the exporting imported warning
dschaefer2 Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Sources/Build/BuildDescription/ProductBuildDescription.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Copy link
Member

@compnerd compnerd Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please do not do this - it is hiding a real issue (one which has performance implications).

Copy link
Member Author

@dschaefer2 dschaefer2 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The ones I'm seeing aren't actionable by the user. They're caused by our lack of support for dynamic targets. There's so many, you miss any other warning that might be actionable.

If a target appears in a DLL, it doesn't get the -static arg. But then if you include it in a EXE, like tests, you get the warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

A lot of the people I talk to agree we need to allow targets as DLLs, so this is really only temporary until we can get there. We'll need to do a Pitch/Evolution proposal for that since that will need a package manifest change so it will end up being post 6.1.

Copy link
Member Author

Choose a reason for hiding this comment

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

@compnerd brings up a good point about hiding user caused warnings. I'll take this out for now and see what the fallout is. And we'll continue to work to make sure SwiftPM isn't causing these warnings.

}

switch derivedProductType {
Expand Down
67 changes: 8 additions & 59 deletions Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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()
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down
19 changes: 2 additions & 17 deletions Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
}
13 changes: 1 addition & 12 deletions Sources/Build/BuildPlan/BuildPlan+Product.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
13 changes: 10 additions & 3 deletions Sources/Build/BuildPlan/BuildPlan.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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.product.modules {
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] {
Expand Down
53 changes: 10 additions & 43 deletions Tests/BuildTests/WindowsBuildPlanTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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"), 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)
XCTAssertEqual(try hasStatic("C.exe-\(label)-debug.module"), triple.isWindows(), label)
}

func testWindows() async throws {
Expand Down