Use a MinidumpCallback to force include memory around instruction pointer

We noticed that some of our Windows minidumps were missing the memory around the
faulting instruction pointer. We finally realized that it was only happening on
dumps from Windows XP. Some testing revealed that the older version of
DbgHelp.dll on WinXP doesn't appear to implement this correctly. (It includes a
memory window around the instruction pointer on the crashing thread, but it uses
the actual IP at the top of the stack, which is inside KiFastSystemCallRet,
instead of using the IP from the EXCEPTION_POINTERS context.) Using a
MinidumpCallback, this patch forces the correct window of memory to be included.
I tested that these tests pass both on Windows 7 (where they passed without my
changes anyway, but still pass with my changes) as well as on Windows XP.

[Mark Mentovai]

I didn’t really read the test, but I’m glad that you included it.

http://breakpad.appspot.com/259001/diff/1/2
File src/client/windows/handler/exception_handler.cc (right):

http://breakpad.appspot.com/259001/diff/1/2#newcode774
Line 774: // callback will ensure that it gets included.
What happens if a newer dbghelp is in use?

http://breakpad.appspot.com/259001/diff/1/2#newcode803
Line 803: printf("start: 0x%llx, end: 0x%lx\n", context.memory_base,
Needed?

http://breakpad.appspot.com/259001/diff/1/2#newcode807
Line 807: printf("memory_base: 0x%llx, memory_size: 0x%lx\n",
Needed?

http://breakpad.appspot.com/259001/diff/1/4
File src/client/windows/unittests/exception_handler_death_test.cc (right):

http://breakpad.appspot.com/259001/diff/1/4#newcode30
Line 30: #include <string>
C++ system headers follow C system headers.

http://breakpad.appspot.com/259001/diff/1/4#newcode529
Line 529: }
}  // namespace

[Ted Mielczarek]

Was this LGO?

http://breakpad.appspot.com/259001/diff/1/2
File src/client/windows/handler/exception_handler.cc (right):

http://breakpad.appspot.com/259001/diff/1/2#newcode774
Line 774: // callback will ensure that it gets included.
On 2011/01/24 19:06:24, Mark Mentovai wrote:
> What happens if a newer dbghelp is in use?

I tested this both with DbgHelp from Windows Server 2003 (which doesn't
correctly put the memory in the minidump) and DbgHelp from Windows 7 (which
does), and this patch doesn't seem to have any negative effects that I can tell.
The tests pass with both, which means that we correctly write a minidump and we
can process it with the processor libraries. (The processor library would choke
if we wound up with two memory regions covering the same range:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/mi...
)

http://breakpad.appspot.com/259001/diff/1/2#newcode803
Line 803: printf("start: 0x%llx, end: 0x%lx\n", context.memory_base,
On 2011/01/24 19:06:24, Mark Mentovai wrote:
> Needed?

Oops, I thought I killed all my debug printfs. I'll get rid of those.

[Mark Mentovai]

Yes, this was LGO (although in the absence of a response from me, the prudent
thing would have been to upload a fresh snapshot and let me know it was ready
for another look.)

[Ted Mielczarek]

On 2011/01/25 17:30:20, Mark Mentovai wrote:
> Yes, this was LGO (although in the absence of a response from me, the prudent
> thing would have been to upload a fresh snapshot and let me know it was ready
> for another look.)

Sorry, that would have been the sensible thing to do.