Allow registering memory regions to be included in dumps

This patch got backed out, so re-submitting it for review. I cleaned up a few
formatting problems (apparently some tabs snuck in) as well as the accidental
file mode changes. This patch was originally reviewed at
https://bugzilla.mozilla.org/show_bug.cgi?id=662646 .

[Mark Mentovai]

http://breakpad.appspot.com/450002/diff/1/src/client/linux/handler/exception_...
File src/client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/450002/diff/1/src/client/linux/handler/exception_...
src/client/linux/handler/exception_handler.cc:567: void
ExceptionHandler::RegisterAppMemory(void *ptr, size_t length) {
Breakpad tends to put the * on the type name and not the variable name. This
file is no exception. Please adhere to the prevailing style. This applies
throughout this change.

http://breakpad.appspot.com/450002/diff/1/src/client/linux/handler/exception_...
src/client/linux/handler/exception_handler.cc:572: for (AppMemoryList::iterator
iter = app_memory_list_.begin();
std::find?

http://breakpad.appspot.com/450002/diff/1/src/client/linux/handler/exception_...
File src/client/linux/handler/exception_handler_unittest.cc (right):

http://breakpad.appspot.com/450002/diff/1/src/client/linux/handler/exception_...
src/client/linux/handler/exception_handler_unittest.cc:988: // Stick some data
into the memory so the contents can be verified.
Leave a blank line before each comment so that each section stands out as its
own thing. Otherwise, the comments get lost among the code. Throughout this
function.

http://breakpad.appspot.com/450002/diff/1/src/client/linux/handler/exception_...
src/client/linux/handler/exception_handler_unittest.cc:989: for (unsigned int i
= 0; i < kMemorySize; ++i) {
kMemorySize is a u_int32_t, so probably i should be the same.

http://breakpad.appspot.com/450002/diff/1/src/client/linux/minidump_writer/mi...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/450002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:796: (uint8_t*)
dumper_->allocator()->Alloc(iter->length);
Use cplusplus<style>(casts).

http://breakpad.appspot.com/450002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:806:
desc.start_of_memory_range = (uintptr_t)iter->ptr;
Here too.

http://breakpad.appspot.com/450002/diff/1/src/client/linux/minidump_writer/mi...
File src/client/linux/minidump_writer/minidump_writer.h (right):

http://breakpad.appspot.com/450002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.h:57: AppMemory(void *ptr,
size_t length) : ptr(ptr), length(length) {}
Is it really helpful to keep this constructor? It’s not used in too many
locations. We might be better off by getting rid of the constructor altogether,
and just making the callers set the two fields, like a real old-school struct.

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

http://breakpad.appspot.com/450002/diff/1/src/client/windows/handler/exceptio...
src/client/windows/handler/exception_handler.cc:49:
AppMemoryList::const_iterator iter, end;
One declaration at a time, please. Our style guide forbids what’s written here.

http://breakpad.appspot.com/450002/diff/1/src/client/windows/handler/exceptio...
src/client/windows/handler/exception_handler.cc:941: for
(AppMemoryList::iterator iter = app_memory_info_.begin();
std::find here too?

http://breakpad.appspot.com/450002/diff/1/src/client/windows/handler/exceptio...
File src/client/windows/handler/exception_handler.h (right):

http://breakpad.appspot.com/450002/diff/1/src/client/windows/handler/exceptio...
src/client/windows/handler/exception_handler.h:70: #include <list>
Sort.

http://breakpad.appspot.com/450002/diff/1/src/client/windows/handler/exceptio...
src/client/windows/handler/exception_handler.h:85: AppMemory(ULONG64 ptr, ULONG
length) : ptr(ptr), length(length) {}
As before.

[Mark Mentovai]

http://breakpad.appspot.com/450002/diff/6002/src/client/linux/minidump_writer...
File src/client/linux/minidump_writer/minidump_writer.h (right):

http://breakpad.appspot.com/450002/diff/6002/src/client/linux/minidump_writer...
src/client/linux/minidump_writer/minidump_writer.h:61: return ptr == other.ptr;
What’s the logic supposed to do here when there are AppMemories that have the
same ptr but different lengths?

[Ted Mielczarek]

http://breakpad.appspot.com/450002/diff/6002/src/client/linux/minidump_writer...
File src/client/linux/minidump_writer/minidump_writer.h (right):

http://breakpad.appspot.com/450002/diff/6002/src/client/linux/minidump_writer...
src/client/linux/minidump_writer/minidump_writer.h:61: return ptr == other.ptr;
On 2012/09/12 18:58:55, Mark Mentovai wrote:
> What’s the logic supposed to do here when there are AppMemories that have the
> same ptr but different lengths?

We should probably just disallow adding a memory region with the same ptr as an
existing region.

[Mark Mentovai]

Will you make that change?

[Ted Mielczarek]

On 2012/09/12 19:09:35, Mark Mentovai wrote:
> Will you make that change?

I can do that. I'm knee-deep in getting Firefox to compile with the latest SVN +
these patches, so once I'm finished with that (and the assortment of other
issues it will uncover that I'll have to patch) I'll update these patches again.

[Ted Mielczarek]

Updated to disallow registering the same pointer twice.

[Mark Mentovai]

LGTM