Skip to content

Handle signals on Windows in exit tests. #766

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 11 commits into from
Oct 21, 2024

Conversation

grynspan
Copy link
Contributor

@grynspan grynspan commented Oct 15, 2024

This PR adds support for handling signals as distinct exit conditions on Windows. Previously, we didn't support them because Windows itself has minimal support. However, through the judicious use of "stuffing a bunch of bits into a 32-bit field", we are able to propagate raised signals out of the child process and detect them in the parent process.

We do this by installing our own signal handlers for all signals supported on Windows, then masking the caught signal against an NTSTATUS severity and facility that are unlikely to be reported by the system in practice. In the parent process, we look for exit codes that match these values and extract the signal from them when found. Because the namespace for exit codes on Windows is shared with uncaught VEH/SEH exceptions (which are propagated to the parent process as NTSTATUS codes), there is the potential for conflict with some hypothetical real exception code, but I'm taking a gamble here that my choice of NTSTATUS facility is unique.

(Bonus points for there not being convenient macros to construct an NTSTATUS code anywhere in the Windows SDK I can find.)

Checklist:

  • Code and documentation should follow the style of the Style Guide.
  • If public symbols are renamed or modified, DocC references should be updated.

@grynspan grynspan added enhancement New feature or request windows 🪟 Windows support workaround Workaround for an issue in another component (may need to revert later) exit-tests ☠️ Work related to exit tests labels Oct 15, 2024
@grynspan grynspan added this to the Swift 6.1 milestone Oct 15, 2024
@grynspan grynspan self-assigned this Oct 15, 2024
@grynspan grynspan requested a review from compnerd October 15, 2024 22:58
@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan
Copy link
Contributor Author

@swift-ci test

2 similar comments
@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan grynspan force-pushed the jgrynspan/signal-handling-on-Windows branch from e30e32a to 65f4091 Compare October 17, 2024 18:47
@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan
Copy link
Contributor Author

@swift-ci test

1 similar comment
@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan
Copy link
Contributor Author

Some additional documentation blobs I'll put here for posterity in case we decide we want to add them:

For .exitCode(_:):

  ///
  /// On Windows, the system uses [Structured Exception Handling](https://learn.microsoft.com/en-us/windows/win32/debug/structured-exception-handling)
  /// to handle some error conditions. If an exception is raised and is not
  /// handled, it will terminate the current process with an exit code equal to
  /// that exception's [exception code](https://learn.microsoft.com/en-us/windows/win32/debug/getexceptioncode).

For .signal(_:):

  ///
  /// ## Signal handling on Windows
  ///
  /// On Windows, by default, the C runtime will terminate a process with exit
  /// code `3` if a raised signal is not handled, exactly as if `exit(3)` were
  /// called.
  ///
  /// In order to accurately report the exit condition of an exit test,
  /// the testing library installs signal handlers for all signals supported by
  /// Windows before an exit test runs. If your code calls `signal(n, SIG_DFL)`
  /// to reset the signal handler for some signal _n_, the system will remove
  /// the signal handler installed by the testing library.
  ///
  /// If you cannot avoid resetting the signal handler in an exit test to
  /// `SIG_DFL`, pass ``failure`` instead of ``signal(_:)`` when you call
  /// ``expect(exitsWith:_:sourceLocation:performing:)`` or
  /// ``require(exitsWith:_:sourceLocation:performing:)``.

This PR adds support for handling signals as distinct exit conditions on Windows. Previously, we didn't support them because Windows itself has minimal support. However, through the judicious use of "stuffing a bunch of bits into a 32-bit field", we are able to propagate raised signals out of the child process and detect them in the parent process.

We do this by installing our own signal handlers for all signals supported on Windows, then masking the caught signal against an `NTSTATUS` severity and facility that are unlikely to be reported by the system in practice. In the parent process, we look for exit codes that match these values and extract the signal from them when found. Because the namespace for exit codes on Windows is shared with uncaught VEH/SEH exceptions (which are propagated to the parent process as `NTSTATUS` codes), there is the potential for conflict with some hypothetical _real_ exception code, but I'm taking a gamble here that my choice of `NTSTATUS` facility is unique.

(Bonus points for there not being convenient macros to construct an `NTSTATUS` code anywhere in the Windows SDK I can find.)
@grynspan grynspan force-pushed the jgrynspan/signal-handling-on-Windows branch from fb9da8b to a94eea2 Compare October 21, 2024 12:36
@grynspan
Copy link
Contributor Author

@swift-ci test

1 similar comment
@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan grynspan merged commit d7d2427 into main Oct 21, 2024
3 checks passed
@grynspan grynspan deleted the jgrynspan/signal-handling-on-Windows branch October 21, 2024 16:01
grynspan added a commit that referenced this pull request Oct 21, 2024
On Windows, `NTSTATUS` is a typedef of `long`. On 32-bit Windows, `long` is
imported into Swift as `Int` but on 64-bit Windows,it's imported as `Int32`.
These types are layout-equivalent but not implicitly convertible, so we get
errors on 32-bit Windows when using an `NTSTATUS` (i.e. `Int`) where a `CInt`
(i.e. `Int32`) is expected.

This PR changes the types of the constants introduced in #766 to be explicitly
`CInt` so that they are usable on both flavours of Windows.
@grynspan grynspan mentioned this pull request Oct 21, 2024
2 tasks
grynspan added a commit that referenced this pull request Oct 21, 2024
On Windows, `NTSTATUS` is a typedef of `long`. On 32-bit Windows, `long`
is imported into Swift as `Int` but on 64-bit Windows,it's imported as
`Int32`. These types are layout-equivalent but not implicitly
convertible, so we get errors on 32-bit Windows when using an `NTSTATUS`
(i.e. `Int`) where a `CInt` (i.e. `Int32`) is expected.

This PR changes the types of the constants introduced in #766 to be
explicitly `CInt` so that they are usable on both flavours of Windows.

### Checklist:

- [x] Code and documentation should follow the style of the [Style
Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md).
- [x] If public symbols are renamed or modified, DocC references should
be updated.
@hjyamauchi
Copy link
Contributor

@grynspan I'm not sure but does it looks like the Windows CI broke: https://ci-external.swift.org/job/swift-main-windows-toolchain/734/consoleText?

C:\Users\swift-ci\jenkins\workspace\swift-main-windows-toolchain\swift-testing\Sources\Testing\Support\Additions\WinSDKAdditions.swift:27:13: error: cannot convert value of type 'Int32' to expected argument type 'Int'
25 | 
26 |   // Set the severity and status bits.
27 |   result |= STATUS_SEVERITY_ERROR << 30
   |             `- error: cannot convert value of type 'Int32' to expected argument type 'Int'
28 |   result |= 1 << 29 // "Customer" bit
29 | 

C:\Users\swift-ci\jenkins\workspace\swift-main-windows-toolchain\swift-testing\Sources\Testing\Support\Additions\WinSDKAdditions.swift:27:35: error: cannot convert value of type 'Int32' to expected argument type 'Int'
25 | 
26 |   // Set the severity and status bits.
27 |   result |= STATUS_SEVERITY_ERROR << 30
   |                                   `- error: cannot convert value of type 'Int32' to expected argument type 'Int'
28 |   result |= 1 << 29 // "Customer" bit
29 | 

C:\Users\swift-ci\jenkins\workspace\swift-main-windows-toolchain\swift-testing\Sources\Testing\ExitTests\ExitTest.swift:224:74: error: cannot convert value of type 'NTSTATUS' (aka 'Int') to expected argument type 'Int32'
222 |     // For an explanation of this magic, see the corresponding logic in
223 |     // ExitTest.callAsFunction().
224 |     if case let .exitCode(exitCode) = result.exitCondition, (exitCode & ~STATUS_CODE_MASK) == STATUS_SIGNAL_CAUGHT_BITS {
    |                                                                          `- error: cannot convert value of type 'NTSTATUS' (aka 'Int') to expected argument type 'Int32'
225 |       result.exitCondition = .signal(exitCode & STATUS_CODE_MASK)
226 |     }

C:\Users\swift-ci\jenkins\workspace\swift-main-windows-toolchain\swift-testing\Sources\Testing\ExitTests\ExitTest.swift:224:71: error: binary operator '&' cannot be applied to two 'CInt' (aka 'Int32') operands
222 |     // For an explanation of this magic, see the corresponding logic in
223 |     // ExitTest.callAsFunction().
224 |     if case let .exitCode(exitCode) = result.exitCondition, (exitCode & ~STATUS_CODE_MASK) == STATUS_SIGNAL_CAUGHT_BITS {
    |                                                                       |- error: binary operator '&' cannot be applied to two 'CInt' (aka 'Int32') operands
    |                                                                       `- note: overloads for '&' exist with these partially matching parameter lists: (Int, Int), (Int32, Int32)
225 |       result.exitCondition = .signal(exitCode & STATUS_CODE_MASK)
226 |     }

C:\Users\swift-ci\jenkins\workspace\swift-main-windows-toolchain\swift-testing\Sources\Testing\ExitTests\ExitTest.swift:225:49: error: cannot convert value of type 'NTSTATUS' (aka 'Int') to expected argument type 'Int32'
223 |     // ExitTest.callAsFunction().
224 |     if case let .exitCode(exitCode) = result.exitCondition, (exitCode & ~STATUS_CODE_MASK) == STATUS_SIGNAL_CAUGHT_BITS {

225 |       result.exitCondition = .signal(exitCode & STATUS_CODE_MASK)

    |                                                 `- error: cannot convert value of type 'NTSTATUS' (aka 'Int') to expected argument type 'Int32'

226 |     }

227 | #endif

@grynspan
Copy link
Contributor Author

That has already been resolved. See #776.

@hjyamauchi
Copy link
Contributor

Thanks @grynspan !!

@grynspan grynspan mentioned this pull request Oct 22, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request exit-tests ☠️ Work related to exit tests windows 🪟 Windows support workaround Workaround for an issue in another component (may need to revert later)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants