From 3279794487885ea6c5ac30111a35264a51d83e4d Mon Sep 17 00:00:00 2001 From: "mark@chromium.org" Date: Tue, 24 Jul 2012 22:45:30 +0000 Subject: Speculatively back out r989 per http://codereview.chromium.org/10805065/ : Ted Mielczarek: > You could try backing out r989, although Mozilla has been running with that > patch for months without issue. Me: > src/client/windows/handler/exception_handler.cc in r989 appears to have > formatting problems, an unwanted property change, and no real Breakpad review > history, so maybe we should back it out anyway until the proper process is > followed. NACL Tests nacl_integration failures: http://build.chromium.org/p/chromium/builders/NACL%20Tests/builds/30138 chrome src/native_client/tests/inbrowser_crash_test/crash_dump_tester.py says that the observed failures are a symptom of crash_service.exe itself crashing. git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@998 4c0a9323-5329-0410-9bdc-e9ce6186880e --- .../linux/minidump_writer/minidump_writer.cc | 45 ++----------- src/client/linux/minidump_writer/minidump_writer.h | 17 +---- .../minidump_writer/minidump_writer_unittest.cc | 73 +--------------------- 3 files changed, 9 insertions(+), 126 deletions(-) (limited to 'src/client/linux/minidump_writer') diff --git a/src/client/linux/minidump_writer/minidump_writer.cc b/src/client/linux/minidump_writer/minidump_writer.cc index 9c834791..e0aeb3bd 100644 --- a/src/client/linux/minidump_writer/minidump_writer.cc +++ b/src/client/linux/minidump_writer/minidump_writer.cc @@ -373,7 +373,6 @@ class MinidumpWriter { MinidumpWriter(const char* filename, const ExceptionHandler::CrashContext* context, const MappingList& mappings, - const AppMemoryList& appmem, LinuxDumper* dumper) : filename_(filename), ucontext_(context ? &context->context : NULL), @@ -385,8 +384,7 @@ class MinidumpWriter { #endif dumper_(dumper), memory_blocks_(dumper_->allocator()), - mapping_list_(mappings), - app_memory_list_(appmem) { + mapping_list_(mappings) { } bool Init() { @@ -460,9 +458,6 @@ class MinidumpWriter { return false; dir.CopyIndex(dir_index++, &dirent); - if (!WriteAppMemory()) - return false; - if (!WriteMemoryListStream(&dirent)) return false; dir.CopyIndex(dir_index++, &dirent); @@ -764,30 +759,6 @@ class MinidumpWriter { return true; } - // Write application-provided memory regions. - bool WriteAppMemory() { - for (AppMemoryList::const_iterator iter = app_memory_list_.begin(); - iter != app_memory_list_.end(); - ++iter) { - uint8_t* data_copy = - (uint8_t*) dumper_->allocator()->Alloc(iter->length); - dumper_->CopyFromProcess(data_copy, GetCrashThread(), iter->ptr, - iter->length); - - UntypedMDRVA memory(&minidump_writer_); - if (!memory.Allocate(iter->length)) { - return false; - } - memory.Copy(data_copy, iter->length); - MDMemoryDescriptor desc; - desc.start_of_memory_range = (uintptr_t)iter->ptr; - desc.memory = memory.location(); - memory_blocks_.push_back(desc); - } - - return true; - } - static bool ShouldIncludeMapping(const MappingInfo& mapping) { if (mapping.name[0] == 0 || // only want modules with filenames. mapping.offset || // only want to include one mapping per shared lib. @@ -1363,22 +1334,17 @@ class MinidumpWriter { wasteful_vector memory_blocks_; // Additional information about some mappings provided by the caller. const MappingList& mapping_list_; - // Additional memory regions to be included in the dump, - // provided by the caller. - const AppMemoryList& app_memory_list_; }; bool WriteMinidump(const char* filename, pid_t crashing_process, const void* blob, size_t blob_size) { MappingList m; - AppMemoryList a; - return WriteMinidump(filename, crashing_process, blob, blob_size, m, a); + return WriteMinidump(filename, crashing_process, blob, blob_size, m); } bool WriteMinidump(const char* filename, pid_t crashing_process, const void* blob, size_t blob_size, - const MappingList& mappings, - const AppMemoryList& appmem) { + const MappingList& mappings) { if (blob_size != sizeof(ExceptionHandler::CrashContext)) return false; const ExceptionHandler::CrashContext* context = @@ -1388,7 +1354,7 @@ bool WriteMinidump(const char* filename, pid_t crashing_process, reinterpret_cast(context->siginfo.si_addr)); dumper.set_crash_signal(context->siginfo.si_signo); dumper.set_crash_thread(context->tid); - MinidumpWriter writer(filename, context, mappings, appmem, &dumper); + MinidumpWriter writer(filename, context, mappings, &dumper); if (!writer.Init()) return false; return writer.Dump(); @@ -1396,9 +1362,8 @@ bool WriteMinidump(const char* filename, pid_t crashing_process, bool WriteMinidump(const char* filename, const MappingList& mappings, - const AppMemoryList& appmem, LinuxDumper* dumper) { - MinidumpWriter writer(filename, NULL, mappings, appmem, dumper); + MinidumpWriter writer(filename, NULL, mappings, dumper); if (!writer.Init()) return false; return writer.Dump(); diff --git a/src/client/linux/minidump_writer/minidump_writer.h b/src/client/linux/minidump_writer/minidump_writer.h index 4ff3b469..e79eb79b 100644 --- a/src/client/linux/minidump_writer/minidump_writer.h +++ b/src/client/linux/minidump_writer/minidump_writer.h @@ -51,16 +51,6 @@ struct MappingEntry { // A list of typedef std::list MappingList; -// These entries store a list of memory regions that the client wants included -// in the minidump. -struct AppMemory { - AppMemory(void *ptr, size_t length) : ptr(ptr), length(length) {} - - void *ptr; - size_t length; -}; -typedef std::list AppMemoryList; - // Write a minidump to the filesystem. This function does not malloc nor use // libc functions which may. Thus, it can be used in contexts where the state // of the heap may be corrupt. @@ -74,16 +64,13 @@ typedef std::list AppMemoryList; bool WriteMinidump(const char* filename, pid_t crashing_process, const void* blob, size_t blob_size); -// This overload also allows passing a list of known mappings and -// a list of additional memory regions to be included in the minidump. +// This overload also allows passing a list of known mappings. bool WriteMinidump(const char* filename, pid_t crashing_process, const void* blob, size_t blob_size, - const MappingList& mappings, - const AppMemoryList& appdata); + const MappingList& mappings); bool WriteMinidump(const char* filename, const MappingList& mappings, - const AppMemoryList& appdata, LinuxDumper* dumper); } // namespace google_breakpad diff --git a/src/client/linux/minidump_writer/minidump_writer_unittest.cc b/src/client/linux/minidump_writer/minidump_writer_unittest.cc index 4f69013a..31e1440d 100644 --- a/src/client/linux/minidump_writer/minidump_writer_unittest.cc +++ b/src/client/linux/minidump_writer/minidump_writer_unittest.cc @@ -32,7 +32,6 @@ #include #include #include -#include #include #include @@ -154,13 +153,12 @@ TEST(MinidumpWriterTest, MappingInfo) { strcpy(info.name, kMemoryName); MappingList mappings; - AppMemoryList memory_list; MappingEntry mapping; mapping.first = info; memcpy(mapping.second, kModuleGUID, sizeof(MDGUID)); mappings.push_back(mapping); ASSERT_TRUE(WriteMinidump(templ.c_str(), child, &context, sizeof(context), - mappings, memory_list)); + mappings)); // Read the minidump. Load the module list, and ensure that // the mmap'ed |memory| is listed with the given module name @@ -259,14 +257,13 @@ TEST(MinidumpWriterTest, MappingInfoContained) { strcpy(info.name, kMemoryName); MappingList mappings; - AppMemoryList memory_list; MappingEntry mapping; mapping.first = info; memcpy(mapping.second, kModuleGUID, sizeof(MDGUID)); mappings.push_back(mapping); ASSERT_TRUE( WriteMinidump(dumpfile.c_str(), child, &context, sizeof(context), - mappings, memory_list)); + mappings)); // Read the minidump. Load the module list, and ensure that // the mmap'ed |memory| is listed with the given module name @@ -387,69 +384,3 @@ TEST(MinidumpWriterTest, DeletedBinary) { module_identifier += "0"; EXPECT_EQ(module_identifier, module->debug_identifier()); } - -// Test that an additional memory region can be added to the minidump. -TEST(MinidumpWriterTest, AdditionalMemory) { - int fds[2]; - ASSERT_NE(-1, pipe(fds)); - - // These are defined here so the parent can use them to check the - // data from the minidump afterwards. - const u_int32_t kMemorySize = sysconf(_SC_PAGESIZE); - // Get some heap memory. - u_int8_t* memory = new u_int8_t[kMemorySize]; - const uintptr_t kMemoryAddress = reinterpret_cast(memory); - ASSERT_TRUE(memory); - // Stick some data into the memory so the contents can be verified. - for (int i = 0; i < kMemorySize; ++i) { - memory[i] = i % 255; - } - - const pid_t child = fork(); - if (child == 0) { - close(fds[1]); - char b; - HANDLE_EINTR(read(fds[0], &b, sizeof(b))); - close(fds[0]); - syscall(__NR_exit); - } - close(fds[0]); - - ExceptionHandler::CrashContext context; - // This needs a valid context for minidump writing to work, but getting - // a useful one from the child is too much work, so just use one from - // the parent since the child is just a forked copy anyway. - //TODO(ted): this won't work for Android if unit tests ever get run there. - ASSERT_EQ(0, getcontext(&context.context)); - context.tid = child; - - AutoTempDir temp_dir; - string templ = "/tmp/minidump-memory.dmp"; //temp_dir.path() + "/minidump-writer-unittest"; - unlink(templ.c_str()); - - MappingList mappings; - AppMemoryList memory_list; - // Add the memory region to the list of memory to be included. - memory_list.push_back(AppMemory(memory, kMemorySize)); - ASSERT_TRUE(WriteMinidump(templ.c_str(), child, &context, sizeof(context), - mappings, memory_list)); - - // Read the minidump. Ensure that the memory region is present - Minidump minidump(templ.c_str()); - ASSERT_TRUE(minidump.Read()); - - MinidumpMemoryList* dump_memory_list = minidump.GetMemoryList(); - ASSERT_TRUE(dump_memory_list); - const MinidumpMemoryRegion* region = - dump_memory_list->GetMemoryRegionForAddress(kMemoryAddress); - ASSERT_TRUE(region); - - EXPECT_EQ(kMemoryAddress, region->GetBase()); - EXPECT_EQ(kMemorySize, region->GetSize()); - - // Verify memory contents. - EXPECT_EQ(0, memcmp(region->GetMemory(), memory, kMemorySize)); - - delete[] memory; - close(fds[1]); -} -- cgit v1.2.1