Add address and reason for IN_PAGE_ERROR.

Add address and reason for IN_PAGE_ERROR.

ACCESS_VIOLATION and IN_PAGE_ERROR both specify
read/write/dep flags and address. ACCESS_VIOLATION currently
reports these, but IN_PAGE_ERROR does not. This change makes
IN_PAGE_ERROR report this information as well, and also the
additional NTSTATUS value for the underlying cause.

[bungeman]

https://breakpad.appspot.com/1794002/diff/1/src/google_breakpad/common/minidu...
File src/google_breakpad/common/minidump_exception_win32.h (right):

https://breakpad.appspot.com/1794002/diff/1/src/google_breakpad/common/minidu...
src/google_breakpad/common/minidump_exception_win32.h:1898:
MD_NTSTATUS_WIN_STATUS_VHD_DIFFERENCING_CHAIN_ERROR_IN_PARENT = 0xC03A0019,
Need to remove trailing comma for strict conformance.

https://breakpad.appspot.com/1794002/diff/1/src/processor/minidump_processor.cc
File src/processor/minidump_processor.cc (right):

https://breakpad.appspot.com/1794002/diff/1/src/processor/minidump_processor....
src/processor/minidump_processor.cc:6012: } break;
Hmmm... seems this break should go inside the block on its own line.

[bungeman]

There is a certain class of crashes Windows I'm currently investigating but a
great many of them are just listed as IN_PAGE_ERROR. The additional information
given by ACCESS_VIOLATION has always been helpful, and it is becoming a bit of a
bother downloading minidumps just to run '!analyze -v' in order to see why and
where the IN_PAGE_ERROR occurred.

In order to make things easier on future users, I propose that IN_PAGE_ERROR act
more like ACCESS_VIOLATION, with the addition of reporting the associated
NTSTATUS code. The size of the list of strings and codes for the NTSTATUS is
somewhat large, let me know if there is a better means of handling that.

I'm also not entirely sure how to test this. I have a large pile of minidumps at
the moment which would test this, but I can't check those in. Currently the
tests pass with this change, but obviously there should be an additional one.
I'm not entirely sure at the moment how to write a short example program to
reliably produce an IN_PAGE_ERROR, but I'm sure there's a way somehow.

[Mark Mentovai]

https://breakpad.appspot.com/1794002/diff/40001/src/google_breakpad/common/mi...
File src/google_breakpad/common/minidump_exception_win32.h (right):

https://breakpad.appspot.com/1794002/diff/40001/src/google_breakpad/common/mi...
src/google_breakpad/common/minidump_exception_win32.h:115:
MD_NTSTATUS_WIN_STATUS_USER_APC = 0x000000C0,
I hope you did this whole thing with some kind of programmatic transformation
and didn’t type it all in manually!

If you did and it’s some simple-ish sed, could you provide it in a comment so
that it can easily be regenerated in the future?  This listing is sparse, and
it’d be a pain to go through it in the future to find things that were added.
It’d be easier to just regenerate the whole thing.

https://breakpad.appspot.com/1794002/diff/40001/src/processor/minidump_proces...
File src/processor/minidump_processor.cc (right):

https://breakpad.appspot.com/1794002/diff/40001/src/processor/minidump_proces...
src/processor/minidump_processor.cc:653: switch (ntstatus) {
Same here, obviously.

https://breakpad.appspot.com/1794002/diff/40001/src/processor/minidump_proces...
src/processor/minidump_processor.cc:6458: }
Actually, this function belongs in its own file.

[bungeman]

Note that I'm really doing this to scratch an itch; I'm not really all that
familiar with breakpad and I'm not currently a committer. So any advice on where
things should go would be most appreciated.

https://breakpad.appspot.com/1794002/diff/40001/src/google_breakpad/common/mi...
File src/google_breakpad/common/minidump_exception_win32.h (right):

https://breakpad.appspot.com/1794002/diff/40001/src/google_breakpad/common/mi...
src/google_breakpad/common/minidump_exception_win32.h:115:
MD_NTSTATUS_WIN_STATUS_USER_APC = 0x000000C0,
On 2015/04/09 20:10:12, Mark Mentovai wrote:
> I hope you did this whole thing with some kind of programmatic transformation
> and didn’t type it all in manually!
> 
> If you did and it’s some simple-ish sed, could you provide it in a comment so
> that it can easily be regenerated in the future?  This listing is sparse, and
> it’d be a pain to go through it in the future to find things that were added.
> It’d be easier to just regenerate the whole thing.

I was up all night! No, I just pulled this in from
https://msdn.microsoft.com/en-us/library/cc704588.aspx 2.3.1 NTSTATUS values. I
didn't know how this would look when I started, so I just did something like

0x([0-9A-F]+)\n([A-Z_0-9]+)\n[^\n]+

and replaced with

  MD_NTSTATUS_WIN_\2 = 0x\1,

and cleaned up the last comma and some multi-line descriptions manually. All of
these values can be found in the Windows headers, but there seems to be more
than one place to get some of them. On the other hand, it might be better to
just pull these out of ntstatus.h, since that is probably more reliable. I
really just threw this together to get feedback on whether this magnitude of
values could be considered. If it's ok to just create a separate file for these
to split things up, I'll make sure these values make sense and can be generated
sanely (as well as the code).

Also, we only really need the ones which begin win '0xC' since those are the
error codes (I'm assuming we're only interested in error codes here, it did
cause an exception).

https://breakpad.appspot.com/1794002/diff/40001/src/processor/minidump_proces...
File src/processor/minidump_processor.cc (right):

https://breakpad.appspot.com/1794002/diff/40001/src/processor/minidump_proces...
src/processor/minidump_processor.cc:653: switch (ntstatus) {
On 2015/04/09 20:10:12, Mark Mentovai wrote:
> Same here, obviously.

Done.

https://breakpad.appspot.com/1794002/diff/40001/src/processor/minidump_proces...
src/processor/minidump_processor.cc:6458: }
On 2015/04/09 20:10:12, Mark Mentovai wrote:
> Actually, this function belongs in its own file.

Are you referring to get_ntstatus_reason, or are you talking about the
per-platform parts of this, or about the (almost, but not quite entirely) shared
code between IN_PAGE_ERROR and ACCESS_VIOLATION? I can certainly more
get_ntstatus_reason into it's own file, but I'm not sure of the most logical
place (other than next to this file). If you have a good place and name, let me
know, and I'll put it there.

There do seem to be a number of things in here which would be nice to split up
into their own independent scopes.

[Mark Mentovai]

I meant to break GetNTSTATUSReason() into its own file. Looks like I put the
comment on the wrong thing.

It’s a 6,400-line beast that’s pretty straightforward and mostly generated, and
you’re adding it to a file that’s already 1,500 lines long. Sure, other things
in this file could be broken out, but let’s just start with this one new
function.

(Ultimately, hopefully, Crashpad will obsolete all of this. In Crashpad, we’d
call something like this “symbolic_constants”—we already have things like
https://chromium.googlesource.com/crashpad/crashpad/+/48b1964d1b21/util/posix...,
for example. We’ve also been much better about avoiding 1,500-line files in
Crashpad.)

[Mark Mentovai]

Apologies for the duplicate, but breakpad.appsopt.com doesn’t show the comment
(yet?) so I’m trying one more time.

I meant to break GetNTSTATUSReason() into its own file. Looks like I put
the comment on the wrong thing.

It’s a 6,400-line beast that’s pretty straightforward and mostly
generated, and you’re adding it to a file that’s already 1,500 lines
long. Sure, other things in this file could be broken out, but let’s
just start with this one new function.

(Ultimately, hopefully, Crashpad will obsolete all of this. In Crashpad,
we’d call something like this “symbolic_constants”—we already have
things like
https://chromium.googlesource.com/crashpad/crashpad/+/48b1964d1b21/util/posix...,
for example. We’ve also been much better about avoiding 1,500-line files
in Crashpad.)

[bungeman]

symbolic_constants_win it is. I've split this out and managed to figure out how
add the new file to the build system. Looks like some stackwalker stuff was
missing from the processor gyp file, so I added it to make minidump_stackwalker
work with the gyp build. I've run this through minidump_stackwalker on my local
.dmp files and things work as expected.

[Mark Mentovai]

LGTM otherwise.

https://breakpad.appspot.com/1794002/diff/170001/Makefile.am
File Makefile.am (right):

https://breakpad.appspot.com/1794002/diff/170001/Makefile.am#newcode269
Makefile.am:269: src/processor/symbolic_constants_win.cc\
Space before \, here and on the next line.

[bungeman]

I'm not currently a committer on the project, so feel free to land this
yourself, or make me a committer and I'll land it. I'm assuming that one
normally uses 'gcl commit' to land?

https://breakpad.appspot.com/1794002/diff/170001/Makefile.am
File Makefile.am (right):

https://breakpad.appspot.com/1794002/diff/170001/Makefile.am#newcode269
Makefile.am:269: src/processor/symbolic_constants_win.cc\
On 2015/04/10 21:39:41, Mark Mentovai wrote:
> Space before \, here and on the next line.

Done.

[Mark Mentovai]

Cool, I landed this as r1441. If you have more patches, I’ll make you a
committer. Thanks!