From 0b740c453c0a21209c88b71bf50a7ab0b00be64c Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Mon, 25 Jul 2022 19:18:33 -0700 Subject: [PATCH 1/5] Separate memory and module regions. Move std::string m_fileName out of MemoryRegion into ModuleRegion that is derived from MemoryRegion. Reduces createdump memory usage. --- src/coreclr/debug/createdump/crashinfo.cpp | 25 ++++++- src/coreclr/debug/createdump/crashinfo.h | 5 +- src/coreclr/debug/createdump/crashinfomac.cpp | 8 +- .../debug/createdump/crashinfounix.cpp | 26 +++---- .../debug/createdump/dumpwriterelf.cpp | 6 +- src/coreclr/debug/createdump/memoryregion.h | 74 +++++++++++-------- 6 files changed, 89 insertions(+), 55 deletions(-) diff --git a/src/coreclr/debug/createdump/crashinfo.cpp b/src/coreclr/debug/createdump/crashinfo.cpp index 044a731eb86846..2ca52d3d72f553 100644 --- a/src/coreclr/debug/createdump/crashinfo.cpp +++ b/src/coreclr/debug/createdump/crashinfo.cpp @@ -529,9 +529,9 @@ CrashInfo::AddOrReplaceModuleMapping(CLRDATA_ADDRESS baseAddress, ULONG64 size, uint32_t flags = GetMemoryRegionFlags((ULONG_PTR)baseAddress); - // Make sure that the page containing the PE header for the managed asseblies is in the dump + // Make sure that the page containing the PE header for the managed assemblies is in the dump // especially on MacOS where they are added artificially. - MemoryRegion header(flags, start, start + PAGE_SIZE); + ModuleRegion header(flags, start, start + PAGE_SIZE); InsertMemoryRegion(header); // Add or change the module mapping for this PE image. The managed assembly images may already @@ -541,7 +541,7 @@ CrashInfo::AddOrReplaceModuleMapping(CLRDATA_ADDRESS baseAddress, ULONG64 size, if (found == m_moduleMappings.end()) { // On MacOS the assemblies are always added. - MemoryRegion newRegion(flags, start, end, 0, name); + ModuleRegion newRegion(flags, start, end, 0, name); m_moduleMappings.insert(newRegion); m_cbModuleMappings += newRegion.Size(); @@ -552,7 +552,7 @@ CrashInfo::AddOrReplaceModuleMapping(CLRDATA_ADDRESS baseAddress, ULONG64 size, else if (found->FileName().compare(name) != 0) { // Create the new memory region with the managed assembly name. - MemoryRegion newRegion(*found, name); + ModuleRegion newRegion(*found, name); // Remove and cleanup the old one m_moduleMappings.erase(found); @@ -889,6 +889,23 @@ CrashInfo::CombineMemoryRegions() } } +// +// Searches for a module region for a given address. +// +const ModuleRegion* +CrashInfo::SearchModuleRegions(const ModuleRegion& search) +{ + std::set::iterator found = m_moduleMappings.find(search); + for (; found != m_moduleMappings.end(); found++) + { + if (search.StartAddress() >= found->StartAddress() && search.StartAddress() < found->EndAddress()) + { + return &*found; + } + } + return nullptr; +} + // // Searches for a memory region given an address. // diff --git a/src/coreclr/debug/createdump/crashinfo.h b/src/coreclr/debug/createdump/crashinfo.h index 6673e2dec7d3e5..6d46c802044307 100644 --- a/src/coreclr/debug/createdump/crashinfo.h +++ b/src/coreclr/debug/createdump/crashinfo.h @@ -71,7 +71,7 @@ class CrashInfo : public ICLRDataEnumMemoryRegionsCallback, public ICLRDataLoggi std::vector m_auxvEntries; // full auxv entries #endif std::vector m_threads; // threads found and suspended - std::set m_moduleMappings; // module memory mappings + std::set m_moduleMappings; // module memory mappings std::set m_otherMappings; // other memory mappings std::set m_memoryRegions; // memory regions from DAC, etc. std::set m_moduleAddresses; // memory region to module base address @@ -105,6 +105,7 @@ class CrashInfo : public ICLRDataEnumMemoryRegionsCallback, public ICLRDataLoggi void AddModuleAddressRange(uint64_t startAddress, uint64_t endAddress, uint64_t baseAddress); void AddModuleInfo(bool isManaged, uint64_t baseAddress, IXCLRDataModule* pClrDataModule, const std::string& moduleName); int InsertMemoryRegion(uint64_t address, size_t size); + const ModuleRegion* SearchModuleRegions(const ModuleRegion& search); static const MemoryRegion* SearchMemoryRegions(const std::set& regions, const MemoryRegion& search); inline pid_t Pid() const { return m_pid; } @@ -122,7 +123,7 @@ class CrashInfo : public ICLRDataEnumMemoryRegionsCallback, public ICLRDataLoggi inline const uint64_t RuntimeBaseAddress() const { return m_runtimeBaseAddress; } inline const std::vector& Threads() const { return m_threads; } - inline const std::set& ModuleMappings() const { return m_moduleMappings; } + inline const std::set& ModuleMappings() const { return m_moduleMappings; } inline const std::set& OtherMappings() const { return m_otherMappings; } inline const std::set& MemoryRegions() const { return m_memoryRegions; } inline const siginfo_t* SigInfo() const { return &m_siginfo; } diff --git a/src/coreclr/debug/createdump/crashinfomac.cpp b/src/coreclr/debug/createdump/crashinfomac.cpp index 89c71c9e586c85..a44139a4a34521 100644 --- a/src/coreclr/debug/createdump/crashinfomac.cpp +++ b/src/coreclr/debug/createdump/crashinfomac.cpp @@ -179,7 +179,7 @@ CrashInfo::InitializeOtherMappings() // to include all the native and managed module regions. for (const MemoryRegion& region : m_allMemoryRegions) { - std::set::iterator found = m_moduleMappings.find(region); + std::set::iterator found = m_moduleMappings.find(ModuleRegion(region)); if (found == m_moduleMappings.end()) { m_otherMappings.insert(region); @@ -315,8 +315,8 @@ void CrashInfo::VisitSegment(MachOModule& module, const segment_command_64& segm assert(end > 0); // Add module memory region if not already on the list - MemoryRegion newModule(regionFlags, start, end, offset, module.Name()); - std::set::iterator existingModule = m_moduleMappings.find(newModule); + ModuleRegion newModule(regionFlags, start, end, offset, module.Name()); + std::set::iterator existingModule = m_moduleMappings.find(newModule); if (existingModule == m_moduleMappings.end()) { if (g_diagnosticsVerbose) @@ -340,7 +340,7 @@ void CrashInfo::VisitSegment(MachOModule& module, const segment_command_64& segm uint64_t numberPages = newModule.SizeInPages(); for (size_t p = 0; p < numberPages; p++, start += PAGE_SIZE, offset += PAGE_SIZE) { - MemoryRegion gap(newModule.Flags(), start, start + PAGE_SIZE, offset, newModule.FileName()); + ModuleRegion gap(newModule.Flags(), start, start + PAGE_SIZE, offset, newModule.FileName()); const auto& found = m_moduleMappings.find(gap); if (found != m_moduleMappings.end()) diff --git a/src/coreclr/debug/createdump/crashinfounix.cpp b/src/coreclr/debug/createdump/crashinfounix.cpp index 4d39ade7fda907..a6f2bb3c246b6b 100644 --- a/src/coreclr/debug/createdump/crashinfounix.cpp +++ b/src/coreclr/debug/createdump/crashinfounix.cpp @@ -267,20 +267,20 @@ CrashInfo::EnumerateMemoryRegions() if (strchr(permissions, 'p')) { regionFlags |= MEMORY_REGION_FLAG_PRIVATE; } - MemoryRegion memoryRegion(regionFlags, start, end, offset, std::string(moduleName != nullptr ? moduleName : "")); + ModuleRegion moduleRegion(regionFlags, start, end, offset, std::string(moduleName != nullptr ? moduleName : "")); if (moduleName != nullptr && *moduleName == '/') { - m_moduleMappings.insert(memoryRegion); - m_cbModuleMappings += memoryRegion.Size(); + m_moduleMappings.insert(moduleRegion); + m_cbModuleMappings += moduleRegion.Size(); } else { - m_otherMappings.insert(memoryRegion); + m_otherMappings.insert(moduleRegion); } if (linuxGateAddress != nullptr && reinterpret_cast(start) == linuxGateAddress) { - InsertMemoryRegion(memoryRegion); + InsertMemoryRegion(moduleRegion); } free(moduleName); free(permissions); @@ -290,7 +290,7 @@ CrashInfo::EnumerateMemoryRegions() if (g_diagnostics) { TRACE("Module mappings (%06llx):\n", m_cbModuleMappings / PAGE_SIZE); - for (const MemoryRegion& region : m_moduleMappings) + for (const ModuleRegion& region : m_moduleMappings) { region.Trace(); } @@ -333,8 +333,8 @@ CrashInfo::VisitModule(uint64_t baseAddress, std::string& moduleName) // it with the one found in the /proc//maps. if (moduleName.empty()) { - MemoryRegion search(0, baseAddress, baseAddress + PAGE_SIZE); - const MemoryRegion* region = SearchMemoryRegions(m_moduleMappings, search); + ModuleRegion search(0, baseAddress, baseAddress + PAGE_SIZE); + const ModuleRegion* region = SearchModuleRegions(search); if (region != nullptr) { moduleName = region->FileName(); @@ -477,12 +477,12 @@ CrashInfo::VisitProgramHeader(uint64_t loadbias, uint64_t baseAddress, Phdr* phd uint32_t CrashInfo::GetMemoryRegionFlags(uint64_t start) { - MemoryRegion search(0, start, start + PAGE_SIZE, 0); - const MemoryRegion* region = SearchMemoryRegions(m_moduleMappings, search); - if (region != nullptr) { - return region->Flags(); + ModuleRegion search(0, start, start + PAGE_SIZE, 0); + const ModuleRegion* moduleRegion = SearchModuleRegions(search); + if (moduleRegion != nullptr) { + return moduleRegion->Flags(); } - region = SearchMemoryRegions(m_otherMappings, search); + const MemoryRegion* region = SearchMemoryRegions(m_otherMappings, search); if (region != nullptr) { return region->Flags(); } diff --git a/src/coreclr/debug/createdump/dumpwriterelf.cpp b/src/coreclr/debug/createdump/dumpwriterelf.cpp index 675113b59bd998..1b4247061c1ea2 100644 --- a/src/coreclr/debug/createdump/dumpwriterelf.cpp +++ b/src/coreclr/debug/createdump/dumpwriterelf.cpp @@ -290,7 +290,7 @@ DumpWriter::GetNTFileInfoSize(size_t* alignmentBytes) size += count; // File name storage needed - for (const MemoryRegion& image : m_crashInfo.ModuleMappings()) { + for (const ModuleRegion& image : m_crashInfo.ModuleMappings()) { size += image.FileName().length(); } // Notes must end on 4 byte alignment @@ -338,7 +338,7 @@ DumpWriter::WriteNTFileInfo() return false; } - for (const MemoryRegion& image : m_crashInfo.ModuleMappings()) + for (const ModuleRegion& image : m_crashInfo.ModuleMappings()) { struct NTFileEntry entry { (unsigned long)image.StartAddress(), (unsigned long)image.EndAddress(), (unsigned long)(image.Offset() / pageSize) }; if (!WriteData(&entry, sizeof(entry))) { @@ -346,7 +346,7 @@ DumpWriter::WriteNTFileInfo() } } - for (const MemoryRegion& image : m_crashInfo.ModuleMappings()) + for (const ModuleRegion& image : m_crashInfo.ModuleMappings()) { if (!WriteData(image.FileName().c_str(), image.FileName().length()) || !WriteData("\0", 1)) { diff --git a/src/coreclr/debug/createdump/memoryregion.h b/src/coreclr/debug/createdump/memoryregion.h index e7b8f31a0b2692..568abd5537df88 100644 --- a/src/coreclr/debug/createdump/memoryregion.h +++ b/src/coreclr/debug/createdump/memoryregion.h @@ -27,21 +27,7 @@ struct MemoryRegion uint64_t m_endAddress; uint64_t m_offset; - // The name used for NT_FILE output - std::string m_fileName; - public: - MemoryRegion(uint32_t flags, uint64_t start, uint64_t end, uint64_t offset, const std::string& filename) : - m_flags(flags), - m_startAddress(start), - m_endAddress(end), - m_offset(offset), - m_fileName(filename) - { - assert((start & ~PAGE_MASK) == 0); - assert((end & ~PAGE_MASK) == 0); - } - MemoryRegion(uint32_t flags, uint64_t start, uint64_t end, uint64_t offset) : m_flags(flags), m_startAddress(start), @@ -60,16 +46,6 @@ struct MemoryRegion assert((end & ~PAGE_MASK) == 0); } - // copy with new file name constructor - MemoryRegion(const MemoryRegion& region, const std::string& fileName) : - m_flags(region.m_flags), - m_startAddress(region.m_startAddress), - m_endAddress(region.m_endAddress), - m_offset(region.m_offset), - m_fileName(fileName) - { - } - // copy with new flags constructor. The file name is not copied. MemoryRegion(const MemoryRegion& region, uint32_t flags) : m_flags(flags), @@ -84,8 +60,7 @@ struct MemoryRegion m_flags(region.m_flags), m_startAddress(region.m_startAddress), m_endAddress(region.m_endAddress), - m_offset(region.m_offset), - m_fileName(region.m_fileName) + m_offset(region.m_offset) { } @@ -100,7 +75,6 @@ struct MemoryRegion inline uint64_t Size() const { return m_endAddress - m_startAddress; } inline uint64_t SizeInPages() const { return Size() / PAGE_SIZE; } inline uint64_t Offset() const { return m_offset; } - inline const std::string& FileName() const { return m_fileName; } bool operator<(const MemoryRegion& rhs) const { @@ -113,7 +87,7 @@ struct MemoryRegion return (m_startAddress <= rhs.m_startAddress) && (m_endAddress >= rhs.m_endAddress); } - void Trace(const char* prefix = "") const + void Trace(const char* prefix = "", const char* suffix = "") const { TRACE("%s%" PRIA PRIx64 " - %" PRIA PRIx64 " (%06" PRIx64 ") %" PRIA PRIx64 " %c%c%c%c%c %02x %s\n", prefix, @@ -127,6 +101,48 @@ struct MemoryRegion (m_flags & MEMORY_REGION_FLAG_SHARED) ? 's' : '-', (m_flags & MEMORY_REGION_FLAG_PRIVATE) ? 'p' : '-', m_flags, - m_fileName.c_str()); + suffix); + } +}; + +struct ModuleRegion : MemoryRegion +{ +private: + std::string m_fileName; + +public: + ModuleRegion(uint32_t flags, uint64_t start, uint64_t end, uint64_t offset, const std::string& filename) : MemoryRegion(flags, start, end, offset), + m_fileName(filename) + { + } + + ModuleRegion(uint32_t flags, uint64_t start, uint64_t end, uint64_t offset) : MemoryRegion(flags, start, end, offset) + { + } + + ModuleRegion(uint32_t flags, uint64_t start, uint64_t end) : MemoryRegion(flags, start, end) + { + } + + // copy with new file name constructor + ModuleRegion(const ModuleRegion& region, const std::string& fileName) : MemoryRegion(region), + m_fileName(fileName) + { + } + + // copy constructor from MemoryRegion + ModuleRegion(const MemoryRegion& region) : MemoryRegion(region) + { + } + + ~ModuleRegion() + { + } + + inline const std::string& FileName() const { return m_fileName; } + + void Trace(const char* prefix = "") const + { + MemoryRegion::Trace(prefix, m_fileName.c_str()); } }; From 56e42c2cfc8a75ddc51a8bb0bf30ac60eeba285d Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Thu, 18 Jan 2024 14:51:47 -0800 Subject: [PATCH 2/5] Fix arm32 sign extension issues. Cleanup: make void* address parameters uint64_t. --- src/coreclr/debug/createdump/crashinfo.cpp | 36 ++++++++++--------- src/coreclr/debug/createdump/crashinfo.h | 7 ++-- src/coreclr/debug/createdump/crashinfomac.cpp | 12 ++++--- .../debug/createdump/crashinfounix.cpp | 14 ++++---- src/coreclr/debug/createdump/datatarget.cpp | 3 +- .../debug/createdump/dumpwriterelf.cpp | 2 +- .../debug/createdump/dumpwritermacho.cpp | 2 +- src/coreclr/debug/createdump/memoryregion.h | 2 ++ 8 files changed, 45 insertions(+), 33 deletions(-) diff --git a/src/coreclr/debug/createdump/crashinfo.cpp b/src/coreclr/debug/createdump/crashinfo.cpp index 2ca52d3d72f553..93e0111376f10e 100644 --- a/src/coreclr/debug/createdump/crashinfo.cpp +++ b/src/coreclr/debug/createdump/crashinfo.cpp @@ -138,7 +138,8 @@ CrashInfo::EnumMemoryRegion( /* [in] */ CLRDATA_ADDRESS address, /* [in] */ ULONG32 size) { - m_enumMemoryPagesAdded += InsertMemoryRegion((ULONG_PTR)address, size); + address = CONVERT_FROM_SIGN_EXTENDED(address); + m_enumMemoryPagesAdded += InsertMemoryRegion(address, size); return S_OK; } @@ -449,10 +450,12 @@ CrashInfo::EnumerateManagedModules() DacpGetModuleData moduleData; if (SUCCEEDED(hr = moduleData.Request(pClrDataModule.GetPtr()))) { - TRACE("MODULE: %" PRIA PRIx64 " dyn %d inmem %d file %d pe %" PRIA PRIx64 " pdb %" PRIA PRIx64, (uint64_t)moduleData.LoadedPEAddress, moduleData.IsDynamic, + uint64_t loadedPEAddress = CONVERT_FROM_SIGN_EXTENDED(moduleData.LoadedPEAddress); + + TRACE("MODULE: %" PRIA PRIx64 " dyn %d inmem %d file %d pe %" PRIA PRIx64 " pdb %" PRIA PRIx64, loadedPEAddress, moduleData.IsDynamic, moduleData.IsInMemory, moduleData.IsFileLayout, (uint64_t)moduleData.PEAssembly, (uint64_t)moduleData.InMemoryPdbAddress); - if (!moduleData.IsDynamic && moduleData.LoadedPEAddress != 0) + if (!moduleData.IsDynamic && loadedPEAddress != 0) { ArrayHolder wszUnicodeName = new WCHAR[MAX_LONGPATH + 1]; if (SUCCEEDED(hr = pClrDataModule->GetFileName(MAX_LONGPATH, nullptr, wszUnicodeName))) @@ -460,10 +463,10 @@ CrashInfo::EnumerateManagedModules() std::string moduleName = ConvertString(wszUnicodeName.GetPtr()); // Change the module mapping name - AddOrReplaceModuleMapping(moduleData.LoadedPEAddress, moduleData.LoadedPESize, moduleName); + AddOrReplaceModuleMapping(loadedPEAddress, moduleData.LoadedPESize, moduleName); // Add managed module info - AddModuleInfo(true, moduleData.LoadedPEAddress, pClrDataModule, moduleName); + AddModuleInfo(true, loadedPEAddress, pClrDataModule, moduleName); } else { TRACE("\nModule.GetFileName FAILED %08x\n", hr); @@ -517,17 +520,17 @@ CrashInfo::UnwindAllThreads() // Replace an existing module mapping with one with a different name. // void -CrashInfo::AddOrReplaceModuleMapping(CLRDATA_ADDRESS baseAddress, ULONG64 size, const std::string& name) +CrashInfo::AddOrReplaceModuleMapping(uint64_t baseAddress, uint64_t size, const std::string& name) { // Round to page boundary (single-file managed assemblies are not page aligned) - ULONG_PTR start = ((ULONG_PTR)baseAddress) & PAGE_MASK; + uint64_t start = baseAddress & PAGE_MASK; assert(start > 0); // Round up to page boundary - ULONG_PTR end = ((baseAddress + size) + (PAGE_SIZE - 1)) & PAGE_MASK; + uint64_t end = ((baseAddress + size) + (PAGE_SIZE - 1)) & PAGE_MASK; assert(end > 0); - uint32_t flags = GetMemoryRegionFlags((ULONG_PTR)baseAddress); + uint32_t flags = GetMemoryRegionFlags(baseAddress); // Make sure that the page containing the PE header for the managed assemblies is in the dump // especially on MacOS where they are added artificially. @@ -648,15 +651,15 @@ CrashInfo::AddModuleInfo(bool isManaged, uint64_t baseAddress, IXCLRDataModule* if (isManaged) { IMAGE_DOS_HEADER dosHeader; - if (ReadMemory((void*)baseAddress, &dosHeader, sizeof(dosHeader))) + if (ReadMemory(baseAddress, &dosHeader, sizeof(dosHeader))) { WORD magic; - if (ReadMemory((void*)(baseAddress + dosHeader.e_lfanew + offsetof(IMAGE_NT_HEADERS, OptionalHeader.Magic)), &magic, sizeof(magic))) + if (ReadMemory(baseAddress + dosHeader.e_lfanew + offsetof(IMAGE_NT_HEADERS, OptionalHeader.Magic), &magic, sizeof(magic))) { if (magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { IMAGE_NT_HEADERS32 header; - if (ReadMemory((void*)(baseAddress + dosHeader.e_lfanew), &header, sizeof(header))) + if (ReadMemory(baseAddress + dosHeader.e_lfanew, &header, sizeof(header))) { imageSize = header.OptionalHeader.SizeOfImage; timeStamp = header.FileHeader.TimeDateStamp; @@ -665,7 +668,7 @@ CrashInfo::AddModuleInfo(bool isManaged, uint64_t baseAddress, IXCLRDataModule* else if (magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) { IMAGE_NT_HEADERS64 header; - if (ReadMemory((void*)(baseAddress + dosHeader.e_lfanew), &header, sizeof(header))) + if (ReadMemory(baseAddress + dosHeader.e_lfanew, &header, sizeof(header))) { imageSize = header.OptionalHeader.SizeOfImage; timeStamp = header.FileHeader.TimeDateStamp; @@ -694,7 +697,7 @@ CrashInfo::AddModuleInfo(bool isManaged, uint64_t baseAddress, IXCLRDataModule* // ReadMemory from target and add to memory regions list // bool -CrashInfo::ReadMemory(void* address, void* buffer, size_t size) +CrashInfo::ReadMemory(uint64_t address, void* buffer, size_t size) { size_t read = 0; if (!ReadProcessMemory(address, buffer, size, &read)) @@ -702,7 +705,7 @@ CrashInfo::ReadMemory(void* address, void* buffer, size_t size) return false; } assert(read == size); - InsertMemoryRegion(reinterpret_cast(address), read); + InsertMemoryRegion(address, read); return true; } @@ -712,6 +715,7 @@ CrashInfo::ReadMemory(void* address, void* buffer, size_t size) int CrashInfo::InsertMemoryRegion(uint64_t address, size_t size) { + assert(address == CONVERT_FROM_SIGN_EXTENDED(address)); assert(size < UINT_MAX); // Round to page boundary @@ -831,7 +835,7 @@ CrashInfo::PageCanBeRead(uint64_t start) { BYTE buffer[1]; size_t read; - return ReadProcessMemory((void*)start, buffer, 1, &read); + return ReadProcessMemory(start, buffer, 1, &read); } // diff --git a/src/coreclr/debug/createdump/crashinfo.h b/src/coreclr/debug/createdump/crashinfo.h index 6d46c802044307..d324c144457076 100644 --- a/src/coreclr/debug/createdump/crashinfo.h +++ b/src/coreclr/debug/createdump/crashinfo.h @@ -97,8 +97,8 @@ class CrashInfo : public ICLRDataEnumMemoryRegionsCallback, public ICLRDataLoggi bool GatherCrashInfo(DumpType dumpType); void CombineMemoryRegions(); bool EnumerateMemoryRegionsWithDAC(DumpType dumpType); - bool ReadMemory(void* address, void* buffer, size_t size); // read memory and add to dump - bool ReadProcessMemory(void* address, void* buffer, size_t size, size_t* read); // read raw memory + bool ReadMemory(uint64_t address, void* buffer, size_t size); // read memory and add to dump + bool ReadProcessMemory(uint64_t address, void* buffer, size_t size, size_t* read); // read raw memory uint64_t GetBaseAddressFromAddress(uint64_t address); uint64_t GetBaseAddressFromName(const char* moduleName); ModuleInfo* GetModuleInfoFromBaseAddress(uint64_t baseAddress); @@ -131,6 +131,7 @@ class CrashInfo : public ICLRDataEnumMemoryRegionsCallback, public ICLRDataLoggi inline const std::vector& AuxvEntries() const { return m_auxvEntries; } inline size_t GetAuxvSize() const { return m_auxvEntries.size() * sizeof(elf_aux_entry); } #endif + bool ReadMemory(void* address, void* buffer, size_t size) { return ReadMemory((uint64_t)address, buffer, size); } // IUnknown STDMETHOD(QueryInterface)(___in REFIID InterfaceId, ___out PVOID* Interface); @@ -160,7 +161,7 @@ class CrashInfo : public ICLRDataEnumMemoryRegionsCallback, public ICLRDataLoggi bool InitializeDAC(DumpType dumpType); bool EnumerateManagedModules(); bool UnwindAllThreads(); - void AddOrReplaceModuleMapping(CLRDATA_ADDRESS baseAddress, ULONG64 size, const std::string& pszName); + void AddOrReplaceModuleMapping(uint64_t baseAddress, uint64_t size, const std::string& pszName); int InsertMemoryRegion(const MemoryRegion& region); uint32_t GetMemoryRegionFlags(uint64_t start); bool PageCanBeRead(uint64_t start); diff --git a/src/coreclr/debug/createdump/crashinfomac.cpp b/src/coreclr/debug/createdump/crashinfomac.cpp index a44139a4a34521..59a1436215207f 100644 --- a/src/coreclr/debug/createdump/crashinfomac.cpp +++ b/src/coreclr/debug/createdump/crashinfomac.cpp @@ -256,7 +256,7 @@ void CrashInfo::VisitModule(MachOModule& module) m_runtimeBaseAddress = module.BaseAddress(); RuntimeInfo runtimeInfo { }; - if (ReadMemory((void*)(module.BaseAddress() + symbolOffset), &runtimeInfo, sizeof(RuntimeInfo))) + if (ReadMemory(module.BaseAddress() + symbolOffset, &runtimeInfo, sizeof(RuntimeInfo))) { if (strcmp(runtimeInfo.Signature, RUNTIME_INFO_SIGNATURE) == 0) { @@ -274,7 +274,7 @@ void CrashInfo::VisitModule(MachOModule& module) m_runtimeBaseAddress = module.BaseAddress(); uint8_t cookie[sizeof(g_debugHeaderCookie)]; - if (ReadMemory((void*)(module.BaseAddress() + symbolOffset), cookie, sizeof(cookie))) + if (ReadMemory(module.BaseAddress() + symbolOffset, cookie, sizeof(cookie))) { if (memcmp(cookie, g_debugHeaderCookie, sizeof(g_debugHeaderCookie)) == 0) { @@ -375,12 +375,14 @@ CrashInfo::VisitSection(MachOModule& module, const section_64& section) uint32_t CrashInfo::GetMemoryRegionFlags(uint64_t start) { + assert(start == CONVERT_FROM_SIGN_EXTENDED(start)); + MemoryRegion search(0, start, start + PAGE_SIZE, 0); const MemoryRegion* region = SearchMemoryRegions(m_allMemoryRegions, search); if (region != nullptr) { return region->Flags(); } - TRACE("GetMemoryRegionFlags: %016llx FAILED\n", start); + TRACE_VERBOSE("GetMemoryRegionFlags: %016llx FAILED\n", start); return PF_R | PF_W | PF_X; } @@ -388,7 +390,7 @@ CrashInfo::GetMemoryRegionFlags(uint64_t start) // Read raw memory // bool -CrashInfo::ReadProcessMemory(void* address, void* buffer, size_t size, size_t* read) +CrashInfo::ReadProcessMemory(uint64_t address, void* buffer, size_t size, size_t* read) { assert(buffer != nullptr); assert(read != nullptr); @@ -411,7 +413,7 @@ CrashInfo::ReadProcessMemory(void* address, void* buffer, size_t size, size_t* r { g_readProcessMemoryResult = result; TRACE_VERBOSE("ReadProcessMemory(%p %d): vm_read_overwrite failed bytesLeft %d bytesRead %d from %p: %s (%x)\n", - address, size, bytesLeft, bytesRead, (void*)addressAligned, mach_error_string(result), result); + (void*)address, size, bytesLeft, bytesRead, (void*)addressAligned, mach_error_string(result), result); break; } ssize_t bytesToCopy = PAGE_SIZE - offset; diff --git a/src/coreclr/debug/createdump/crashinfounix.cpp b/src/coreclr/debug/createdump/crashinfounix.cpp index a6f2bb3c246b6b..da967d7ef89131 100644 --- a/src/coreclr/debug/createdump/crashinfounix.cpp +++ b/src/coreclr/debug/createdump/crashinfounix.cpp @@ -374,7 +374,7 @@ CrashInfo::VisitModule(uint64_t baseAddress, std::string& moduleName) // explicit initialization for old gcc support; instead of just runtimeInfo { } RuntimeInfo runtimeInfo { .Signature = { }, .Version = 0, .RuntimeModuleIndex = { }, .DacModuleIndex = { }, .DbiModuleIndex = { }, .RuntimeVersion = { } }; - if (ReadMemory((void*)(baseAddress + symbolOffset), &runtimeInfo, sizeof(RuntimeInfo))) + if (ReadMemory(baseAddress + symbolOffset, &runtimeInfo, sizeof(RuntimeInfo))) { if (strcmp(runtimeInfo.Signature, RUNTIME_INFO_SIGNATURE) == 0) { @@ -395,7 +395,7 @@ CrashInfo::VisitModule(uint64_t baseAddress, std::string& moduleName) m_runtimeBaseAddress = baseAddress; uint8_t cookie[sizeof(g_debugHeaderCookie)]; - if (ReadMemory((void*)(baseAddress + symbolOffset), cookie, sizeof(cookie))) + if (ReadMemory(baseAddress + symbolOffset, cookie, sizeof(cookie))) { if (memcmp(cookie, g_debugHeaderCookie, sizeof(g_debugHeaderCookie)) == 0) { @@ -414,7 +414,7 @@ BOOL ReadMemoryAdapter(PVOID address, PVOID buffer, SIZE_T size) { size_t read = 0; - return g_crashInfo->ReadProcessMemory(address, buffer, size, &read); + return g_crashInfo->ReadProcessMemory(CONVERT_FROM_SIGN_EXTENDED(address), buffer, size, &read); } // @@ -477,6 +477,8 @@ CrashInfo::VisitProgramHeader(uint64_t loadbias, uint64_t baseAddress, Phdr* phd uint32_t CrashInfo::GetMemoryRegionFlags(uint64_t start) { + assert(start == CONVERT_FROM_SIGN_EXTENDED(start)); + ModuleRegion search(0, start, start + PAGE_SIZE, 0); const ModuleRegion* moduleRegion = SearchModuleRegions(search); if (moduleRegion != nullptr) { @@ -486,7 +488,7 @@ CrashInfo::GetMemoryRegionFlags(uint64_t start) if (region != nullptr) { return region->Flags(); } - TRACE("GetMemoryRegionFlags: FAILED\n"); + TRACE_VERBOSE("GetMemoryRegionFlags: %016llx FAILED\n", start); return PF_R | PF_W | PF_X; } @@ -494,7 +496,7 @@ CrashInfo::GetMemoryRegionFlags(uint64_t start) // Read raw memory // bool -CrashInfo::ReadProcessMemory(void* address, void* buffer, size_t size, size_t* read) +CrashInfo::ReadProcessMemory(uint64_t address, void* buffer, size_t size, size_t* read) { assert(buffer != nullptr); assert(read != nullptr); @@ -504,7 +506,7 @@ CrashInfo::ReadProcessMemory(void* address, void* buffer, size_t size, size_t* r if (m_canUseProcVmReadSyscall) { iovec local{ buffer, size }; - iovec remote{ address, size }; + iovec remote{ (void*)address, size }; *read = process_vm_readv(m_pid, &local, 1, &remote, 1, 0); } diff --git a/src/coreclr/debug/createdump/datatarget.cpp b/src/coreclr/debug/createdump/datatarget.cpp index 420300e37ab746..243d8458bda538 100644 --- a/src/coreclr/debug/createdump/datatarget.cpp +++ b/src/coreclr/debug/createdump/datatarget.cpp @@ -118,8 +118,9 @@ DumpDataTarget::ReadVirtual( /* [in] */ ULONG32 size, /* [optional][out] */ ULONG32 *done) { + address = CONVERT_FROM_SIGN_EXTENDED(address); size_t read = 0; - if (!m_crashInfo.ReadProcessMemory((void*)(ULONG_PTR)address, buffer, size, &read)) + if (!m_crashInfo.ReadProcessMemory(address, buffer, size, &read)) { TRACE("DumpDataTarget::ReadVirtual %p %d FAILED\n", (void*)address, size); *done = 0; diff --git a/src/coreclr/debug/createdump/dumpwriterelf.cpp b/src/coreclr/debug/createdump/dumpwriterelf.cpp index 1b4247061c1ea2..48d4e609876e4a 100644 --- a/src/coreclr/debug/createdump/dumpwriterelf.cpp +++ b/src/coreclr/debug/createdump/dumpwriterelf.cpp @@ -189,7 +189,7 @@ DumpWriter::WriteDump() size_t bytesToRead = std::min(size, sizeof(m_tempBuffer)); size_t read = 0; - if (!m_crashInfo.ReadProcessMemory((void*)address, m_tempBuffer, bytesToRead, &read)) { + if (!m_crashInfo.ReadProcessMemory(address, m_tempBuffer, bytesToRead, &read)) { printf_error("Error reading memory at %" PRIA PRIx64 " size %08zx FAILED %s (%d)\n", address, bytesToRead, strerror(g_readProcessMemoryErrno), g_readProcessMemoryErrno); return false; } diff --git a/src/coreclr/debug/createdump/dumpwritermacho.cpp b/src/coreclr/debug/createdump/dumpwritermacho.cpp index 79b71132f3a3a8..277adadf7a0cc4 100644 --- a/src/coreclr/debug/createdump/dumpwritermacho.cpp +++ b/src/coreclr/debug/createdump/dumpwritermacho.cpp @@ -268,7 +268,7 @@ DumpWriter::WriteSegments() size_t bytesToRead = std::min(size, sizeof(m_tempBuffer)); size_t read = 0; - if (!m_crashInfo.ReadProcessMemory((void*)address, m_tempBuffer, bytesToRead, &read)) { + if (!m_crashInfo.ReadProcessMemory(address, m_tempBuffer, bytesToRead, &read)) { printf_error("Error reading memory at %" PRIA PRIx64 " size %08zx FAILED %s (%x)\n", address, bytesToRead, mach_error_string(g_readProcessMemoryResult), g_readProcessMemoryResult); return false; } diff --git a/src/coreclr/debug/createdump/memoryregion.h b/src/coreclr/debug/createdump/memoryregion.h index 568abd5537df88..3b4a96c0608b85 100644 --- a/src/coreclr/debug/createdump/memoryregion.h +++ b/src/coreclr/debug/createdump/memoryregion.h @@ -9,6 +9,8 @@ extern long g_pageSize; #undef PAGE_MASK #define PAGE_MASK (~(PAGE_SIZE-1)) +#define CONVERT_FROM_SIGN_EXTENDED(offset) ((ULONG_PTR)(offset)) + enum MEMORY_REGION_FLAGS : uint32_t { // PF_X = 0x01, // Execute From e3608feb3eba6dd0a5c8ed6536ddf2682a9a7664 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Fri, 19 Jan 2024 12:27:53 -0800 Subject: [PATCH 3/5] Don't add "(deleted)" files to the module list on Linux. Issue: https://github.com/dotnet/runtime/issues/90547 --- src/coreclr/debug/createdump/crashinfounix.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/debug/createdump/crashinfounix.cpp b/src/coreclr/debug/createdump/crashinfounix.cpp index da967d7ef89131..6873cb4b3262b6 100644 --- a/src/coreclr/debug/createdump/crashinfounix.cpp +++ b/src/coreclr/debug/createdump/crashinfounix.cpp @@ -271,8 +271,14 @@ CrashInfo::EnumerateMemoryRegions() if (moduleName != nullptr && *moduleName == '/') { - m_moduleMappings.insert(moduleRegion); - m_cbModuleMappings += moduleRegion.Size(); + // Don't add files that don't exists anymore especially /memfd:doublemapper. + std::string name(moduleName); + size_t last = name.rfind(" (deleted)"); + if (last == std::string::npos) + { + m_moduleMappings.insert(moduleRegion); + m_cbModuleMappings += moduleRegion.Size(); + } } else { From f9617c54680b38f9b30b3c71581920c5e8bc29ba Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Fri, 26 Jan 2024 05:03:08 -0800 Subject: [PATCH 4/5] Code review feedback --- src/coreclr/debug/createdump/crashinfounix.cpp | 5 ++--- src/coreclr/debug/createdump/memoryregion.h | 5 +++++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/coreclr/debug/createdump/crashinfounix.cpp b/src/coreclr/debug/createdump/crashinfounix.cpp index 6873cb4b3262b6..61911b701619e3 100644 --- a/src/coreclr/debug/createdump/crashinfounix.cpp +++ b/src/coreclr/debug/createdump/crashinfounix.cpp @@ -267,13 +267,12 @@ CrashInfo::EnumerateMemoryRegions() if (strchr(permissions, 'p')) { regionFlags |= MEMORY_REGION_FLAG_PRIVATE; } - ModuleRegion moduleRegion(regionFlags, start, end, offset, std::string(moduleName != nullptr ? moduleName : "")); + ModuleRegion moduleRegion(regionFlags, start, end, offset, moduleName); if (moduleName != nullptr && *moduleName == '/') { // Don't add files that don't exists anymore especially /memfd:doublemapper. - std::string name(moduleName); - size_t last = name.rfind(" (deleted)"); + size_t last = moduleRegion.FileName().rfind(" (deleted)"); if (last == std::string::npos) { m_moduleMappings.insert(moduleRegion); diff --git a/src/coreclr/debug/createdump/memoryregion.h b/src/coreclr/debug/createdump/memoryregion.h index 3b4a96c0608b85..124c9fbcb1423e 100644 --- a/src/coreclr/debug/createdump/memoryregion.h +++ b/src/coreclr/debug/createdump/memoryregion.h @@ -113,6 +113,11 @@ struct ModuleRegion : MemoryRegion std::string m_fileName; public: + ModuleRegion(uint32_t flags, uint64_t start, uint64_t end, uint64_t offset, char* filename) : MemoryRegion(flags, start, end, offset), + m_fileName(filename != nullptr ? filename : "") + { + } + ModuleRegion(uint32_t flags, uint64_t start, uint64_t end, uint64_t offset, const std::string& filename) : MemoryRegion(flags, start, end, offset), m_fileName(filename) { From 1979581911dbb5bb3b2fe9d9e5de08853fee59b8 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Fri, 26 Jan 2024 10:06:15 -0800 Subject: [PATCH 5/5] Add deleted modules to other mappings --- src/coreclr/debug/createdump/crashinfounix.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/coreclr/debug/createdump/crashinfounix.cpp b/src/coreclr/debug/createdump/crashinfounix.cpp index 61911b701619e3..9f72707263b07a 100644 --- a/src/coreclr/debug/createdump/crashinfounix.cpp +++ b/src/coreclr/debug/createdump/crashinfounix.cpp @@ -278,6 +278,10 @@ CrashInfo::EnumerateMemoryRegions() m_moduleMappings.insert(moduleRegion); m_cbModuleMappings += moduleRegion.Size(); } + else + { + m_otherMappings.insert(moduleRegion); + } } else {