Record the last fatal error message for crash reporting on Windows #72718

hjyamauchi · 2024-03-29T23:47:10Z

AFAICT, crashreporter is specific to Darwin and currently it's not possible to record the fatal error message in a crash dump on Windows. Use a similar technique to make it available for Windows crash dump tools.

hyp · 2024-03-30T02:34:38Z

nice! is there a good way to test this change as well?

tristanlabelle

Thanks, that'll be very helpful!

tristanlabelle · 2024-04-01T13:47:19Z

stdlib/public/runtime/Errors.cpp

+#if defined(_WIN32)
+  // The last fatal error message is accessible for crash reporting on
+  // Windows.
+  char *oldMessage = nullptr;


We could reuse the same code as above by defining char** lastFatalErrorMessage = &gCRAnnotations or &gLastFatalErrorMessage based on preprocessor values.

tristanlabelle · 2024-04-01T13:48:36Z

stdlib/public/runtime/CrashReporter.cpp

@@ -33,3 +33,10 @@ struct crashreporter_annotations_t gCRAnnotations __attribute__((
 }

 #endif
+
+#if defined(_WIN32)


Is the redeclaration necessary given #include "swift/Runtime/Debug.h"?

Yes, note that the one in Debug.h is extern and this here is the real definition / storage location.

hjyamauchi · 2024-04-01T23:36:59Z

nice! is there a good way to test this change as well?

Added a test

hjyamauchi · 2024-04-01T23:37:30Z

PTAL

hyp · 2024-04-02T17:18:58Z

test/Runtime/fatal-message-crash-dump-win.swift

+        }
+        let ppLastFatalErrorMessage = unsafeBitCast(
+            GetProcAddress(hSwiftCore, "gLastFatalErrorMessage"),
+            to: UnsafeMutablePointer<UnsafeMutablePointer<UInt8>>.self)


Is it possible to cast it to Optional<UnsafeMutablePointer<Optional<UnsafeMutablePointer<UInt8>>>> instead? That way you can compare it for nil using optional instead of the bit pattern.

hyp

Thanks!

DougGregor · 2024-04-02T20:07:19Z

@swift-ci please smoke test

hyp · 2024-04-02T20:52:12Z

@hjyamauchi Also, could you add a test case for try! failure too? I think that error description should now be captured from the same output

compnerd · 2024-04-02T21:56:24Z

Also consider: #72785 - this is currently Windows specific which seems like a shame. Additionally, I am concerned about the uniqueness of the address of the storage when used across modules on PE/COFF where the address of a global imported would be the IAT slot IIRC.

hyp · 2024-04-02T22:03:26Z

@hjyamauchi here's an example of a try failure:

enum Errs: Error {
 case getMyDescription(String)
}

func throwings(_ i: Int) throws {
  if i == 0 {
    throw Errs.getMyDescription("hello")
  }
}

func testMe() {
  try!  throwings(0)
}

testMe()

hjyamauchi · 2024-04-02T23:53:53Z

@hyp Thanks. I added a try! test.

@compnerd Would it make sense (and are you willing) to adapt the tests in this PR into #72785 ? (or I could also work on that if preferred)

al45tair · 2024-04-03T08:51:24Z

I'm a little conflicted about this. I think @compnerd is right that there's an issue with the global storage on Windows, but on a more general note I'm not sure how users are expected to make use of this (on non-Darwin platforms).

What's really needed (IMO) is a Windows version of the crash handling code that I added for Darwin and Linux, and then a little bit of extra work to better integrate fatalError() with that on all platforms. The Darwin crash reporter code will stay, because that's still useful there, but on Linux and Windows getting the extra data into a crash log like this isn't trivial.

From a platform standpoint, on Windows it might actually make sense for fatalError() to report things to the Event Log using ReportEvent(). They'd end up as separate events in the application event log that you'd find before the actual crash, but you wouldn't need to have a crash handler installed or enabled to get the data.

compnerd · 2024-04-03T14:35:15Z

Having a custom stream in the mini dump is another option on windows that we shouldn't rule out imo. That does require that we install a custom dump handler though. I'm hesitant to expose a stream writer as SPI as that would cause additional code execution in the address space after a fatalError.

hjyamauchi · 2024-04-03T16:54:56Z

I'm a little conflicted about this. I think @compnerd is right that there's an issue with the global storage on Windows, but on a more general note I'm not sure how users are expected to make use of this (on non-Darwin platforms).

A way that I'm thinking of using this is through sentry/crashpad as in thebrowsercompany/swift-sentry#35 This works and puts the fatal error messages into sentry crash reports through a minidump.

What's really needed (IMO) is a Windows version of the crash handling code that I added for Darwin and Linux, and then a little bit of extra work to better integrate fatalError() with that on all platforms. The Darwin crash reporter code will stay, because that's still useful there, but on Linux and Windows getting the extra data into a crash log like this isn't trivial.

Can you point to "the crash handling code that I added for Darwin and Linux", if available?

From a platform standpoint, on Windows it might actually make sense for fatalError() to report things to the Event Log using ReportEvent(). They'd end up as separate events in the application event log that you'd find before the actual crash, but you wouldn't need to have a crash handler installed or enabled to get the data.

Do you mind clarifying "report things to the Event Log using ReportEvent()"? I'm not sure what ReportEvent refers to.

compnerd · 2024-04-03T17:14:26Z

A way that I'm thinking of using this is through sentry/crashpad as in thebrowsercompany/swift-sentry#35 This works and puts the fatal error messages into sentry crash reports through a minidump.

That is a product level decision, not a language level. The only two "blessed" error reporting mechanisms for Windows would be the custom minidump writer and ReportEvent. The latter being preferable as the user may wish to have control over the minidump emission and us squatting that space would be a problem. Sentry and crashpad are both outside the scope of supportable tools for the language runtime.

Do you mind clarifying "report things to the Event Log using ReportEvent()"? I'm not sure what ReportEvent refers to.

https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-reporteventw

al45tair · 2024-04-03T17:27:56Z

Can you point to "the crash handling code that I added for Darwin and Linux", if available?

The Linux crash handler is here: CrashHandlerLinux.cpp. There's a similar one for macOS, and I've been meaning to get around to writing something for Windows too. It's on by default for Linux, and I was intending to have it on by default for Windows also, at least for console applications (subject to discussing it with @compnerd, since it does mean running code in a crashed process, which is all kinds of fun).

al45tair · 2024-04-03T17:30:29Z

The latter being preferable as the user may wish to have control over the minidump emission and us squatting that space would be a problem.

Agreed. The existing Linux and macOS crash handlers both make sure to crash the process the same way it would have crashed without them.

hjyamauchi · 2024-04-09T19:21:30Z

Closing this in favor of #72785

hjyamauchi requested review from mikeash and al45tair as code owners March 29, 2024 23:47

hjyamauchi requested review from compnerd and hyp March 29, 2024 23:47

tristanlabelle approved these changes Apr 1, 2024

View reviewed changes

hjyamauchi force-pushed the fatalerror branch from c7fa387 to a6b2669 Compare April 1, 2024 23:33

hyp reviewed Apr 2, 2024

View reviewed changes

hjyamauchi force-pushed the fatalerror branch from a6b2669 to e49142e Compare April 2, 2024 18:16

hyp approved these changes Apr 2, 2024

View reviewed changes

Record the last fatal error message for crash reporting on Windows

23ec6c9

hjyamauchi force-pushed the fatalerror branch from e49142e to 23ec6c9 Compare April 2, 2024 23:45

hjyamauchi closed this Apr 9, 2024

Record the last fatal error message for crash reporting on Windows #72718

Record the last fatal error message for crash reporting on Windows #72718

Uh oh!

Conversation

hjyamauchi commented Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hyp commented Mar 30, 2024

Uh oh!

tristanlabelle left a comment

Choose a reason for hiding this comment

Uh oh!

tristanlabelle Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

hjyamauchi Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

tristanlabelle Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

hjyamauchi Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

hjyamauchi commented Apr 1, 2024

Uh oh!

hjyamauchi commented Apr 1, 2024

Uh oh!

hyp Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

hjyamauchi Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

hyp left a comment

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Apr 2, 2024

Uh oh!

hyp commented Apr 2, 2024

Uh oh!

compnerd commented Apr 2, 2024

Uh oh!

hyp commented Apr 2, 2024

Uh oh!

hjyamauchi commented Apr 2, 2024

Uh oh!

al45tair commented Apr 3, 2024

Uh oh!

compnerd commented Apr 3, 2024

Uh oh!

hjyamauchi commented Apr 3, 2024

Uh oh!

compnerd commented Apr 3, 2024

Uh oh!

al45tair commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

al45tair commented Apr 3, 2024

Uh oh!

hjyamauchi commented Apr 9, 2024

Uh oh!

Uh oh!

hjyamauchi commented Mar 29, 2024 •

edited

Loading

al45tair commented Apr 3, 2024 •

edited

Loading