This is an archive of the discontinued LLVM Phabricator instance.

[Support] Creation of minidump after compiler crash on Windows
ClosedPublic

Authored by lenykholodov on Mar 16 2016, 11:23 AM.

Details

Summary

In the current implementation compiler only prints stack trace to console after crash. This patch adds saving of minidump files which contain a useful subset of the information for further debugging.

Diff Detail

Repository
rL LLVM

Event Timeline

lenykholodov retitled this revision from to [Support] Creation of minidump after compiler crash on Windows.
lenykholodov updated this object.
lenykholodov added reviewers: rnk, Bigcheese.
lenykholodov set the repository for this revision to rL LLVM.
lenykholodov added a subscriber: llvm-commits.
asl added a subscriber: asl.Mar 16 2016, 1:02 PM

You may also want to take a look at D14022, which was implementing this same sort of idea (though it appears to have stalled a bit).

You may also want to take a look at D14022, which was implementing this same sort of idea (though it appears to have stalled a bit).

This patch is much simpler than D14022. It was prepared as small and reliable as possible.

You may also want to take a look at D14022, which was implementing this same sort of idea (though it appears to have stalled a bit).

This patch is much simpler than D14022. It was prepared as small and reliable as possible.

The point I was getting at is that we came up with a design in D14022 that we liked and there has been a fair amount of review done on already. This patch would directly conflict with that one, and so we should determine if the original author is intending to continue their work or not, and if not, what ideas from the original patch we may want to incorporate here.

Really sorry for stalling on D14022. I've been sidetracked on some high-priority off-list work for a few months that took me off this although I've been acutely aware that having a stalled patch sitting in review for so long is bad form, so I've been intending to get back to this very very soon. Leny, I'm happy to work with you to make sure we have a solution that satisfies both our use-cases, as it's definitely an incredibly useful feature to have. I'll take a look at this patch in depth as soon as I'm back from EuroLLVM next week.

Really sorry for stalling on D14022. I've been sidetracked on some high-priority off-list work for a few months that took me off this although I've been acutely aware that having a stalled patch sitting in review for so long is bad form, so I've been intending to get back to this very very soon. Leny, I'm happy to work with you to make sure we have a solution that satisfies both our use-cases, as it's definitely an incredibly useful feature to have. I'll take a look at this patch in depth as soon as I'm back from EuroLLVM next week.

Ok. I'll prepare unit test for crash reports and will wait for update for your patch or feedback for mine.

The updated patch is based on D14022 and includes fixes for comments from reviewers.

gbedwell edited edge metadata.Apr 18 2016, 8:46 AM

Thanks for working on this!

This passes all of my local testing for our use cases for the change proposed in D14022. From that point of view, I'm very happy to resolve that review in favour of this patch. I don't feel comfortable giving LGTMs in this area of the compiler just yet, but from my point of view I'm happy.

Thanks again.

Thank you, Greg! @aaron.ballman and @rnk, could you please review updated patch?

aaron.ballman added inline comments.Apr 28 2016, 6:47 AM
include/llvm/Support/Process.h
73 ↗(On Diff #51978)

Perhaps: AreCoreFilesPrevented() to be a bit more grammatically correct?

lib/Support/Windows/Signals.inc
124

Are we still supporting 32-bit MinGW so that we need to keep doing this dance? :-(

585

No need to initialize this.

611

This doesn't qualify as a "small" vector; might as well just use std::vector.

622

Same here.

652

No need to initialize this.

696

I don't think this is reasonable to assume since the crash dump functionality could be called from any executable in the LLVM project. I would say that if we can't get the executable filename, things are in worse shape than we realize and we should just bail out.

lib/Support/Windows/WindowsSupport.h
175 ↗(On Diff #51978)

s/0/NULL

lenykholodov edited edge metadata.

Fixes according to the review. Tested llvm support building with MinGW-W64-builds-4.2.0

Hi Aaron,

Thank you for review. I have updated patch according to your comments. Could you please take a look?

include/llvm/Support/Process.h
73 ↗(On Diff #55604)

Fixed

lib/Support/Windows/Signals.inc
124

I have checked llvm support building with mingw and have not found any issues. Could you please clarify what is wrong with this code?

585

Fixed

611

This was done similar to using of SmallVector in Path.inc file. I can change type to std::vector. However, this will be inconsistent with other code in Support library.

652

Fixed

696

Fixed

lib/Support/Windows/WindowsSupport.h
175 ↗(On Diff #55604)

Fixed

aaron.ballman accepted this revision.May 2 2016, 5:58 AM
aaron.ballman edited edge metadata.

Thank you for working on this, the patch LGTM!

lib/Support/Windows/Signals.inc
124

The code is fine, I just wish we didn't have to dynamically load everything and redeclare things from public SDKs. I should have been more clear, sorry about that! There's nothing for you to change here, more of a note that we should see whether we still need to continue following this pattern for MinGW.

611

Consistency is reasonable enough, the SmallVector can handle being not small and this isn't on a hot code path, so I think it's fine as-is.

This revision is now accepted and ready to land.May 2 2016, 5:58 AM
lenykholodov added inline comments.May 4 2016, 9:59 AM
lib/Support/Windows/Signals.inc
124

Thank you for clarification

This revision was automatically updated to reflect the committed changes.