From e55979b1e2af94b9894327f66b36cacf8ebf4017 Mon Sep 17 00:00:00 2001 From: Greg Clayton Date: Thu, 13 Dec 2018 17:24:30 +0000 Subject: [PATCH] Fix MinidumpParser::GetFilteredModuleList() and test it The MinidumpParser::GetFilteredModuleList() code was attempting to iterate through the entire module list and if it found more than one entry for a given module name, it wanted to pick the MinidumpModule with the lowest address. A bug existed where it wasn't doing that due to "exists" variable being inverted. "exists" was set to true if it was inserted, not if it existed. Furthermore, the order of the modules would be modified by sorting all modules from low address to high address (using MinidumpModule::base_of_image). This fix also maintains the original order which means your executable is at index 0 as intended instead of some random shared library. Tests were added to ensure this functionality doesn't regress. Differential Revision: https://reviews.llvm.org/D55614 llvm-svn: 349062 --- .../Process/minidump/MinidumpParser.cpp | 45 +++++++++++------- .../minidump/Inputs/modules-dup-min-addr.dmp | Bin 0 -> 420 bytes .../Process/minidump/Inputs/modules-order.dmp | Bin 0 -> 420 bytes .../Process/minidump/MinidumpParserTest.cpp | 38 +++++++++++++++ 4 files changed, 65 insertions(+), 18 deletions(-) create mode 100644 lldb/unittests/Process/minidump/Inputs/modules-dup-min-addr.dmp create mode 100644 lldb/unittests/Process/minidump/Inputs/modules-order.dmp diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp index 6b94d4fede32..adaa01c76d36 100644 --- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp +++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp @@ -274,36 +274,45 @@ llvm::ArrayRef MinidumpParser::GetModuleList() { std::vector MinidumpParser::GetFilteredModuleList() { llvm::ArrayRef modules = GetModuleList(); - // map module_name -> pair(load_address, pointer to module struct in memory) - llvm::StringMap> lowest_addr; + // map module_name -> filtered_modules index + typedef llvm::StringMap MapType; + MapType module_name_to_filtered_index; std::vector filtered_modules; - + llvm::Optional name; std::string module_name; for (const auto &module : modules) { name = GetMinidumpString(module.module_name_rva); - + if (!name) continue; - + module_name = name.getValue(); + + MapType::iterator iter; + bool inserted; + // See if we have inserted this module aready into filtered_modules. If we + // haven't insert an entry into module_name_to_filtered_index with the + // index where we will insert it if it isn't in the vector already. + std::tie(iter, inserted) = module_name_to_filtered_index.try_emplace( + module_name, filtered_modules.size()); - auto iter = lowest_addr.end(); - bool exists; - std::tie(iter, exists) = lowest_addr.try_emplace( - module_name, std::make_pair(module.base_of_image, &module)); - - if (exists && module.base_of_image < iter->second.first) - iter->second = std::make_pair(module.base_of_image, &module); + if (inserted) { + // This module has not been seen yet, insert it into filtered_modules at + // the index that was inserted into module_name_to_filtered_index using + // "filtered_modules.size()" above. + filtered_modules.push_back(&module); + } else { + // This module has been seen. Modules are sometimes mentioned multiple + // times when they are mapped discontiguously, so find the module with + // the lowest "base_of_image" and use that as the filtered module. + auto dup_module = filtered_modules[iter->second]; + if (module.base_of_image < dup_module->base_of_image) + filtered_modules[iter->second] = &module; + } } - - filtered_modules.reserve(lowest_addr.size()); - for (const auto &module : lowest_addr) { - filtered_modules.push_back(module.second.second); - } - return filtered_modules; } diff --git a/lldb/unittests/Process/minidump/Inputs/modules-dup-min-addr.dmp b/lldb/unittests/Process/minidump/Inputs/modules-dup-min-addr.dmp new file mode 100644 index 0000000000000000000000000000000000000000..a7a1c4afe85a8878ee68e16c37482e33e19402e4 GIT binary patch literal 420 zcmeZu@eP=~oPmLvfq_8*h|vK%P{0C+U4WP$h$Voy28daJ_yG`40b=F`Ab|!L8yOf% z7|{f=a6w9{(Zrcxd<7^&04Bh|&_aZn2-PI8V0OT0pw=lgvWo|3wjqNlgDZm(kTzog R@%0%>7;+g3fOH}T9{}do4io?Y literal 0 HcmV?d00001 diff --git a/lldb/unittests/Process/minidump/Inputs/modules-order.dmp b/lldb/unittests/Process/minidump/Inputs/modules-order.dmp new file mode 100644 index 0000000000000000000000000000000000000000..cab926b212972b5d2093f8a6a440d398cdd1aadc GIT binary patch literal 420 zcmeZu@eP=~oPmLvfq_8*h|vK%P{0C+U4WP$h$Voy28daJ_yG`40b=F`Ab|!L8yOf% z7|{f=a6w9{(Zrcxd<7^&04Bh|&_aZn2-PI8V0OT0pw=lgvWo|3wjqNlgDZm(kTzog T@%0%>7;+g3fOH}nKZyYV=GYDu literal 0 HcmV?d00001 diff --git a/lldb/unittests/Process/minidump/MinidumpParserTest.cpp b/lldb/unittests/Process/minidump/MinidumpParserTest.cpp index 57f27d4f6ba3..7508cf236546 100644 --- a/lldb/unittests/Process/minidump/MinidumpParserTest.cpp +++ b/lldb/unittests/Process/minidump/MinidumpParserTest.cpp @@ -533,3 +533,41 @@ TEST_F(MinidumpParserTest, ConvertMinidumpContext_x86_32_wow64) { } } } + +TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMinAddress) { + SetUpData("modules-dup-min-addr.dmp"); + // Test that if we have two modules in the module list: + // /tmp/a with range [0x2000-0x3000) + // /tmp/a with range [0x1000-0x2000) + // That we end up with one module in the filtered list with the + // range [0x1000-0x2000). MinidumpParser::GetFilteredModuleList() is + // trying to ensure that if we have the same module mentioned more than + // one time, we pick the one with the lowest base_of_image. + std::vector filtered_modules = + parser->GetFilteredModuleList(); + EXPECT_EQ(1, filtered_modules.size()); + EXPECT_EQ(0x0000000000001000, filtered_modules[0]->base_of_image); +} + +TEST_F(MinidumpParserTest, MinidumpModuleOrder) { + SetUpData("modules-order.dmp"); + // Test that if we have two modules in the module list: + // /tmp/a with range [0x2000-0x3000) + // /tmp/b with range [0x1000-0x2000) + // That we end up with two modules in the filtered list with the same ranges + // and in the same order. Previous versions of the + // MinidumpParser::GetFilteredModuleList() function would sort all images + // by address and modify the order of the modules. + std::vector filtered_modules = + parser->GetFilteredModuleList(); + llvm::Optional name; + EXPECT_EQ(2, filtered_modules.size()); + EXPECT_EQ(0x0000000000002000, filtered_modules[0]->base_of_image); + name = parser->GetMinidumpString(filtered_modules[0]->module_name_rva); + ASSERT_TRUE((bool)name); + EXPECT_EQ(std::string("/tmp/a"), *name); + EXPECT_EQ(0x0000000000001000, filtered_modules[1]->base_of_image); + name = parser->GetMinidumpString(filtered_modules[1]->module_name_rva); + ASSERT_TRUE((bool)name); + EXPECT_EQ(std::string("/tmp/b"), *name); +}