Skip to content

[windows] add a check for a matching python architecture #1942

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Sources/SwiftDriver/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ add_library(SwiftDriver
Utilities/FileMetadata.swift
Utilities/FileType.swift
Utilities/PredictableRandomNumberGenerator.swift
Utilities/PythonArchitecture.swift
Utilities/RelativePathAdditions.swift
Utilities/Sanitizer.swift
Utilities/StringAdditions.swift
Expand Down
10 changes: 9 additions & 1 deletion Sources/SwiftDriver/Driver/Driver.swift
Original file line number Diff line number Diff line change
Expand Up @@ -864,6 +864,14 @@ public struct Driver {
if case .subcommand = try Self.invocationRunMode(forArgs: args).mode {
throw Error.subcommandPassedToDriver
}

#if os(Windows)
if args[1] == "-repl" { // a `-` gets automatically added to `swift repl`.
try checkIfMatchingPythonArch(
cwd: ProcessEnv.cwd, env: env, diagnosticsEngine: diagnosticsEngine)
}
#endif

var args = args
if let additional = env["ADDITIONAL_SWIFT_DRIVER_FLAGS"] {
args.append(contentsOf: additional.components(separatedBy: " "))
Expand Down Expand Up @@ -955,7 +963,7 @@ public struct Driver {
negative: .disableIncrementalFileHashing,
default: false)
self.recordedInputMetadata = .init(uniqueKeysWithValues:
Set(inputFiles).compactMap { inputFile -> (TypedVirtualPath, FileMetadata)? in
Set(inputFiles).compactMap { inputFile -> (TypedVirtualPath, FileMetadata)? in
guard let modTime = try? fileSystem.lastModificationTime(for: inputFile.file) else { return nil }
if incrementalFileHashes {
guard let data = try? fileSystem.readFileContents(inputFile.file) else { return nil }
Expand Down
122 changes: 122 additions & 0 deletions Sources/SwiftDriver/Utilities/PythonArchitecture.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import Foundation

import struct TSCBasic.AbsolutePath
import struct TSCBasic.ByteString
import struct TSCBasic.Diagnostic
import protocol TSCBasic.DiagnosticData
import class TSCBasic.DiagnosticsEngine
import protocol TSCBasic.FileSystem
import protocol TSCBasic.OutputByteStream
import func TSCBasic.getEnvSearchPaths
import func TSCBasic.lookupExecutablePath

/// Check that the architecture of the toolchain matches the architecture
/// of the Python installation.
///
/// When installing the x86 toolchain on ARM64 Windows, if the user does not
/// install an x86 version of Python, they will get acryptic error message
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "acryptic" => "a cryptic"

/// when running lldb (`0xC000007B`). Calling this function before invoking
/// lldb gives them a warning to help troublshoot the issue.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "troublshoot" mis-spelled

///
/// - Parameters:
/// - cwd: The current working directory.
/// - env: A dictionary of the environment variables and their values. Usually of the parent shell.
/// - diagnosticsEngine: DiagnosticsEngine instance to use for printing the warning.
public func checkIfMatchingPythonArch(
cwd: AbsolutePath?, env: [String: String], diagnosticsEngine: DiagnosticsEngine
) throws {
#if arch(arm64)
let toolchainArchitecture = COFFBinaryExecutableArchitecture.arm64
#elseif arch(x86_64)
let toolchainArchitecture = COFFBinaryExecutableArchitecture.x64
#elseif arch(x86)
let toolchainArchitecture = COFFBinaryExecutableArchitecture.x86
#else
return
#endif
let pythonArchitecture = COFFBinaryExecutableArchitecture.readWindowsExecutableArchitecture(
cwd: cwd, env: env, filename: "python.exe")

if toolchainArchitecture != pythonArchitecture {
diagnosticsEngine.emit(
.warning(
"""
There is an architecture mismatch between the installed toolchain and the resolved Python's architecture:
Toolchain: \(toolchainArchitecture)
Python: \(pythonArchitecture)
"""))
}
}

/// Some of the architectures that can be stored in a COFF header.
enum COFFBinaryExecutableArchitecture: String {
case x86 = "X86"
case x64 = "X64"
case arm64 = "ARM64"
case unknown = "Unknown"

static func fromPEMachineByte(machine: UInt16) -> Self {
// https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#machine-types
switch machine {
case 0x014c: return .x86
case 0x8664: return .x64
case 0xAA64: return .arm64
default: return .unknown
}
}

/// Resolves the filename from the `Path` environment variable and read its COFF header to determine the architecture
/// of the binary.
///
/// - Parameters:
/// - cwd: The current working directory.
/// - env: A dictionary of the environment variables and their values. Usually of the parent shell.
/// - filename: The name of the file we are resolving the architecture of.
/// - Returns: The architecture of the file which was found in the `Path`.
static func readWindowsExecutableArchitecture(
cwd: AbsolutePath?, env: [String: String], filename: String
) -> Self {
let searchPaths = getEnvSearchPaths(pathString: env["Path"], currentWorkingDirectory: cwd)
Copy link
Contributor

Choose a reason for hiding this comment

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

Environment on Windows is case-insensitive and this WILL lead to very subtle bugs in cases where Path happens to be cased as PATH.

Do we have an Environment abstraction available to SwiftDriver like we have in SwiftPM that we can use instead of a raw [String: String] dictionary?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have TSCBasic.ProcessEnvironmentBlock which I just transitioned most uses in the driver to: fe4a66c.

Though this is missing a bunch of good stuff that SPM's Environment has, it does use ProcessEnvironmentKey which appears to differentiate case sensitivity for Windows in particular. @charles-zablit we should switch to that here.

guard let filePath = lookupExecutablePath(
filename: filename, currentWorkingDirectory: cwd, searchPaths: searchPaths)
else {
return .unknown
}
guard let fileHandle = FileHandle(forReadingAtPath: filePath.pathString) else {
return .unknown
}

defer { fileHandle.closeFile() }

// Infering the architecture of a Windows executable from its COFF header involves the following:
// 1. Get the COFF header offset from the pointer located at the 0x3C offset (4 bytes long).
// 2. Jump to that offset and read the next 6 bytes.
// 3. The first 4 are the signature which should be equal to 0x50450000.
// 4. The last 2 are the machine architecture which can be infered from the value we get.
//
// The link below provides a visualization of the COFF header and the process to get to it.
// https://upload.wikimedia.org/wikipedia/commons/1/1b/Portable_Executable_32_bit_Structure_in_SVG_fixed.svg
fileHandle.seek(toFileOffset: 0x3C)
guard let offsetPointer = try? fileHandle.read(upToCount: 4),
offsetPointer.count == 4
else {
return .unknown
}

let peHeaderOffset = offsetPointer.withUnsafeBytes { $0.load(as: UInt32.self) }

fileHandle.seek(toFileOffset: UInt64(peHeaderOffset))
guard let coffHeader = try? fileHandle.read(upToCount: 6), coffHeader.count == 6 else {
return .unknown
}

let signature = coffHeader.prefix(4)
let machineBytes = coffHeader.suffix(2)

guard signature == Data([0x50, 0x45, 0x00, 0x00]) else {
return .unknown
}

return .fromPEMachineByte(machine: machineBytes.withUnsafeBytes { $0.load(as: UInt16.self) })
}
}