From d2daf0b96c29d0c3f1070ac93fc351237133bcd0 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Sat, 17 Apr 2021 14:25:10 -0700 Subject: [PATCH] MacOS single-file diagnostic support changes (#51425) * MacOS single-file support changes Use dysymtab_command to just load the extern/export symbols. Don't read the whole symbol string table; read the symbol name a char at a time. https://github.com/dotnet/runtime/issues/38901 * Don't add null to symbol name * Code review feedback * Code review feedback * Code review feedback fix --- src/coreclr/debug/createdump/crashinfomac.cpp | 10 +- .../debug/createdump/crashinfounix.cpp | 9 +- .../debug/createdump/specialthreadinfo.h | 4 +- src/coreclr/debug/daccess/CMakeLists.txt | 4 +- src/coreclr/debug/daccess/daccess.cpp | 4 +- src/coreclr/debug/dbgutil/CMakeLists.txt | 4 +- src/coreclr/debug/dbgutil/elfreader.cpp | 2 +- src/coreclr/debug/dbgutil/machoreader.cpp | 106 ++++++++++-------- src/coreclr/debug/dbgutil/machoreader.h | 6 +- 9 files changed, 87 insertions(+), 62 deletions(-) diff --git a/src/coreclr/debug/createdump/crashinfomac.cpp b/src/coreclr/debug/createdump/crashinfomac.cpp index 98de7005281133..2dc418215bdb2a 100644 --- a/src/coreclr/debug/createdump/crashinfomac.cpp +++ b/src/coreclr/debug/createdump/crashinfomac.cpp @@ -252,9 +252,15 @@ void CrashInfo::VisitModule(MachOModule& module) // Save the runtime module path if (m_coreclrPath.empty()) { - size_t last = module.Name().rfind(MAKEDLLNAME_A("coreclr")); + size_t last = module.Name().rfind(DIRECTORY_SEPARATOR_STR_A MAKEDLLNAME_A("coreclr")); if (last != std::string::npos) { - m_coreclrPath = module.Name().substr(0, last); + m_coreclrPath = module.Name().substr(0, last + 1); + + uint64_t symbolOffset; + if (!module.TryLookupSymbol("g_dacTable", &symbolOffset)) + { + TRACE("TryLookupSymbol(g_dacTable) FAILED\n"); + } } } // VisitSegment is called for each segment of the module diff --git a/src/coreclr/debug/createdump/crashinfounix.cpp b/src/coreclr/debug/createdump/crashinfounix.cpp index 93a55394a5d8a8..2ce100a61c0aba 100644 --- a/src/coreclr/debug/createdump/crashinfounix.cpp +++ b/src/coreclr/debug/createdump/crashinfounix.cpp @@ -265,16 +265,19 @@ CrashInfo::VisitModule(uint64_t baseAddress, std::string& moduleName) } if (m_coreclrPath.empty()) { - size_t last = moduleName.rfind(MAKEDLLNAME_A("coreclr")); + size_t last = moduleName.rfind(DIRECTORY_SEPARATOR_STR_A MAKEDLLNAME_A("coreclr")); if (last != std::string::npos) { - m_coreclrPath = moduleName.substr(0, last); + m_coreclrPath = moduleName.substr(0, last + 1); // Now populate the elfreader with the runtime module info and // lookup the DAC table symbol to ensure that all the memory // necessary is in the core dump. if (PopulateForSymbolLookup(baseAddress)) { uint64_t symbolOffset; - TryLookupSymbol("g_dacTable", &symbolOffset); + if (!TryLookupSymbol("g_dacTable", &symbolOffset)) + { + TRACE("TryLookupSymbol(g_dacTable) FAILED\n"); + } } } } diff --git a/src/coreclr/debug/createdump/specialthreadinfo.h b/src/coreclr/debug/createdump/specialthreadinfo.h index c7fd75ab9617e9..661473eee565e6 100644 --- a/src/coreclr/debug/createdump/specialthreadinfo.h +++ b/src/coreclr/debug/createdump/specialthreadinfo.h @@ -10,7 +10,7 @@ // This defines a workaround to the MacOS dump format not having the OS process // and thread ids that SOS needs to map thread "indexes" to thread "ids". The MacOS // createdump adds this special memory region at this specific address that is not -// in the user or kernel address spaces. lldb is find with it. +// in the user or kernel address spaces. lldb is fine with it. #define SPECIAL_THREADINFO_SIGNATURE "THREADINFO" @@ -20,7 +20,7 @@ struct SpecialThreadInfoHeader { char signature[16]; uint32_t pid; - uint32_t numThreads; + uint32_t numThreads; // The number of SpecialThreadInfoEntry's after this header }; struct SpecialThreadInfoEntry diff --git a/src/coreclr/debug/daccess/CMakeLists.txt b/src/coreclr/debug/daccess/CMakeLists.txt index d6869aeb8e9de9..414afed38d0be9 100644 --- a/src/coreclr/debug/daccess/CMakeLists.txt +++ b/src/coreclr/debug/daccess/CMakeLists.txt @@ -42,7 +42,7 @@ target_precompile_headers(daccess PRIVATE [["stdafx.h"]]) add_dependencies(daccess eventing_headers) -if(CLR_CMAKE_HOST_OSX OR CLR_CMAKE_HOST_FREEBSD OR CLR_CMAKE_HOST_NETBSD OR CLR_CMAKE_HOST_SUNOS) +if(CLR_CMAKE_HOST_FREEBSD OR CLR_CMAKE_HOST_NETBSD OR CLR_CMAKE_HOST_SUNOS) add_definitions(-DUSE_DAC_TABLE_RVA) set(args $<$>:--dynamic> $ ${GENERATED_INCLUDE_DIR}/dactablerva.h) @@ -67,4 +67,4 @@ if(CLR_CMAKE_HOST_OSX OR CLR_CMAKE_HOST_FREEBSD OR CLR_CMAKE_HOST_NETBSD OR CLR_ ) add_dependencies(daccess dactablerva_header) -endif(CLR_CMAKE_HOST_OSX OR CLR_CMAKE_HOST_FREEBSD OR CLR_CMAKE_HOST_NETBSD OR CLR_CMAKE_HOST_SUNOS) +endif(CLR_CMAKE_HOST_FREEBSD OR CLR_CMAKE_HOST_NETBSD OR CLR_CMAKE_HOST_SUNOS) diff --git a/src/coreclr/debug/daccess/daccess.cpp b/src/coreclr/debug/daccess/daccess.cpp index 71e35abec93e53..9152919b18e24d 100644 --- a/src/coreclr/debug/daccess/daccess.cpp +++ b/src/coreclr/debug/daccess/daccess.cpp @@ -28,7 +28,7 @@ #ifdef USE_DAC_TABLE_RVA #include #else -extern bool TryGetSymbol(ICorDebugDataTarget* dataTarget, uint64_t baseAddress, const char* symbolName, uint64_t* symbolAddress); +extern "C" bool TryGetSymbol(ICorDebugDataTarget* dataTarget, uint64_t baseAddress, const char* symbolName, uint64_t* symbolAddress); #endif #endif @@ -7238,7 +7238,7 @@ GetDacTableAddress(ICorDebugDataTarget* dataTarget, ULONG64 baseAddress, PULONG6 // On MacOS, FreeBSD or NetBSD use the RVA include file *dacTableAddress = baseAddress + DAC_TABLE_RVA; #else - // On Linux try to get the dac table address via the export symbol + // On Linux/MacOS try to get the dac table address via the export symbol if (!TryGetSymbol(dataTarget, baseAddress, "g_dacTable", dacTableAddress)) { return CORDBG_E_MISSING_DEBUGGER_EXPORTS; diff --git a/src/coreclr/debug/dbgutil/CMakeLists.txt b/src/coreclr/debug/dbgutil/CMakeLists.txt index 01d724d1c7d786..91f41404221df3 100644 --- a/src/coreclr/debug/dbgutil/CMakeLists.txt +++ b/src/coreclr/debug/dbgutil/CMakeLists.txt @@ -25,10 +25,10 @@ if(CLR_CMAKE_TARGET_LINUX) ) endif(CLR_CMAKE_TARGET_LINUX) -if(CLR_CMAKE_HOST_OSX) +if(CLR_CMAKE_TARGET_OSX) list(APPEND DBGUTIL_SOURCES machoreader.cpp ) -endif(CLR_CMAKE_HOST_OSX) +endif(CLR_CMAKE_TARGET_OSX) add_library_clr(dbgutil STATIC ${DBGUTIL_SOURCES}) diff --git a/src/coreclr/debug/dbgutil/elfreader.cpp b/src/coreclr/debug/dbgutil/elfreader.cpp index 5cbb4ca0efd684..bc6300a7717410 100644 --- a/src/coreclr/debug/dbgutil/elfreader.cpp +++ b/src/coreclr/debug/dbgutil/elfreader.cpp @@ -62,7 +62,7 @@ class ElfReaderExport : public ElfReader // // Main entry point to get an export symbol // -bool +extern "C" bool TryGetSymbol(ICorDebugDataTarget* dataTarget, uint64_t baseAddress, const char* symbolName, uint64_t* symbolAddress) { ElfReaderExport elfreader(dataTarget); diff --git a/src/coreclr/debug/dbgutil/machoreader.cpp b/src/coreclr/debug/dbgutil/machoreader.cpp index fea2862681a869..a19c01b94b3736 100644 --- a/src/coreclr/debug/dbgutil/machoreader.cpp +++ b/src/coreclr/debug/dbgutil/machoreader.cpp @@ -52,7 +52,7 @@ class MachOReaderExport : public MachOReader // // Main entry point to get an export symbol // -bool +extern "C" bool TryGetSymbol(ICorDebugDataTarget* dataTarget, uint64_t baseAddress, const char* symbolName, uint64_t* symbolAddress) { MachOReaderExport reader(dataTarget); @@ -64,7 +64,7 @@ TryGetSymbol(ICorDebugDataTarget* dataTarget, uint64_t baseAddress, const char* uint64_t symbolOffset; if (module.TryLookupSymbol(symbolName, &symbolOffset)) { - *symbolAddress = baseAddress + symbolOffset; + *symbolAddress = symbolOffset; return true; } *symbolAddress = 0; @@ -82,7 +82,7 @@ MachOModule::MachOModule(MachOReader& reader, mach_vm_address_t baseAddress, mac m_commands(nullptr), m_symtabCommand(nullptr), m_nlists(nullptr), - m_strtab(nullptr) + m_strtabAddress(0) { if (header != nullptr) { m_header = *header; @@ -102,10 +102,6 @@ MachOModule::~MachOModule() free(m_nlists); m_nlists = nullptr; } - if (m_strtab != nullptr) { - free(m_strtab); - m_strtab = nullptr; - } } bool @@ -128,14 +124,19 @@ MachOModule::TryLookupSymbol(const char* symbolName, uint64_t* symbolValue) if (ReadSymbolTable()) { _ASSERTE(m_nlists != nullptr); - _ASSERTE(m_strtab != nullptr); + _ASSERTE(m_strtabAddress != 0); - for (int i = 0; i < m_symtabCommand->nsyms; i++) + for (int i = 0; i < m_dysymtabCommand->nextdefsym; i++) { - char* name = m_strtab + m_nlists[i].n_un.n_strx; - if (strcmp(name, symbolName) == 0) + std::string name = GetSymbolName(i); + // Skip the leading underscores to match Linux externs + if (name[0] == '_') + { + name.erase(0, 1); + } + if (strcmp(name.c_str(), symbolName) == 0) { - *symbolValue = m_nlists[i].n_value; + *symbolValue = m_loadBias + m_nlists[i].n_value; return true; } } @@ -197,6 +198,10 @@ MachOModule::ReadLoadCommands() m_symtabCommand = (symtab_command*)command; break; + case LC_DYSYMTAB: + m_dysymtabCommand = (dysymtab_command*)command; + break; + case LC_SEGMENT_64: segment_command_64* segment = (segment_command_64*)command; m_segments.push_back(segment); @@ -255,62 +260,72 @@ MachOModule::ReadSymbolTable() return false; } _ASSERTE(m_symtabCommand != nullptr); - _ASSERTE(m_strtab == nullptr); + _ASSERTE(m_strtabAddress == 0); - m_reader.TraceVerbose("SYM: symoff %08x nsyms %d stroff %08x strsize %d\n", + m_reader.TraceVerbose("SYM: symoff %08x nsyms %d stroff %08x strsize %d iext %d next %d\n", m_symtabCommand->symoff, m_symtabCommand->nsyms, m_symtabCommand->stroff, - m_symtabCommand->strsize); - - // Read symbol table. An array of "nlist" structs. - void* symtabAddress = GetAddressFromFileOffset(m_symtabCommand->symoff); - size_t symtabSize = sizeof(nlist_64) * m_symtabCommand->nsyms; + m_symtabCommand->strsize, + m_dysymtabCommand->iextdefsym, + m_dysymtabCommand->nextdefsym); + // Read the external symbol part of symbol table. An array of "nlist" structs. + void* extSymbolTableAddress = (void*)(GetAddressFromFileOffset(m_symtabCommand->symoff) + (m_dysymtabCommand->iextdefsym * sizeof(nlist_64))); + size_t symtabSize = sizeof(nlist_64) * m_dysymtabCommand->nextdefsym; m_nlists = (nlist_64*)malloc(symtabSize); if (m_nlists == nullptr) { - m_reader.Trace("ERROR: Failed to allocate %zu byte symbol table\n", symtabSize); + m_reader.Trace("ERROR: Failed to allocate %zu byte external symbol table\n", symtabSize); return false; } - if (!m_reader.ReadMemory(symtabAddress, m_nlists, symtabSize)) + if (!m_reader.ReadMemory(extSymbolTableAddress, m_nlists, symtabSize)) { - m_reader.Trace("ERROR: Failed to read symtab at %p of %zu\n", symtabAddress, symtabSize); + m_reader.Trace("ERROR: Failed to read external symtab at %p of %zu\n", extSymbolTableAddress, symtabSize); return false; } - // Read the symbol string table. - void* strtabAddress = GetAddressFromFileOffset(m_symtabCommand->stroff); - size_t strtabSize = m_symtabCommand->strsize; - - m_strtab = (char*)malloc(strtabSize); - if (m_strtab == nullptr) - { - m_reader.Trace("ERROR: Failed to allocate %zu byte symbol string table\n", strtabSize); - return false; - } - if (!m_reader.ReadMemory(strtabAddress, m_strtab, strtabSize)) - { - m_reader.Trace("ERROR: Failed to read string table at %p of %zu\n", strtabAddress, strtabSize); - return false; - } + // Save the symbol string table address. + m_strtabAddress = GetAddressFromFileOffset(m_symtabCommand->stroff); } return true; } -void* +uint64_t MachOModule::GetAddressFromFileOffset(uint32_t offset) { _ASSERTE(!m_segments.empty()); - for (const segment_command_64* segment : m_segments) { if (offset >= segment->fileoff && offset < (segment->fileoff + segment->filesize)) { - return (void*)(m_baseAddress + offset + segment->vmaddr - segment->fileoff); + return m_loadBias + offset + segment->vmaddr - segment->fileoff; + } + } + return m_loadBias + offset; +} + +std::string +MachOModule::GetSymbolName(int index) +{ + uint64_t symbolNameAddress = m_strtabAddress + m_nlists[index].n_un.n_strx; + std::string result; + while (true) + { + char c = 0; + if (!m_reader.ReadMemory((void*)symbolNameAddress, &c, sizeof(char))) + { + m_reader.Trace("ERROR: Failed to read string table at %p\n", (void*)symbolNameAddress); + break; + } + if (c == '\0') + { + break; } + result.append(1, c); + symbolNameAddress++; } - return (void*)(m_baseAddress + offset); + return result; } //-------------------------------------------------------------------- @@ -330,20 +345,19 @@ MachOReader::EnumerateModules(mach_vm_address_t address, mach_header_64* header) MachOModule dylinker(*this, address, header); // Search for symbol for the dyld image info cache - uint64_t dyldInfoOffset = 0; - if (!dylinker.TryLookupSymbol("_dyld_all_image_infos", &dyldInfoOffset)) + uint64_t dyldInfoAddress = 0; + if (!dylinker.TryLookupSymbol("dyld_all_image_infos", &dyldInfoAddress)) { Trace("ERROR: Can not find the _dyld_all_image_infos symbol\n"); return false; } // Read the all image info from the dylinker image - void* dyldInfoAddress = (void*)(address + dyldInfoOffset); dyld_all_image_infos dyldInfo; - if (!ReadMemory(dyldInfoAddress, &dyldInfo, sizeof(dyld_all_image_infos))) + if (!ReadMemory((void*)dyldInfoAddress, &dyldInfo, sizeof(dyld_all_image_infos))) { - Trace("ERROR: Failed to read dyld_all_image_infos at %p\n", dyldInfoAddress); + Trace("ERROR: Failed to read dyld_all_image_infos at %p\n", (void*)dyldInfoAddress); return false; } std::string dylinkerPath; diff --git a/src/coreclr/debug/dbgutil/machoreader.h b/src/coreclr/debug/dbgutil/machoreader.h index 38f1bb8e6559fc..bb9ffe0fdd7ceb 100644 --- a/src/coreclr/debug/dbgutil/machoreader.h +++ b/src/coreclr/debug/dbgutil/machoreader.h @@ -22,8 +22,9 @@ class MachOModule load_command* m_commands; std::vector m_segments; symtab_command* m_symtabCommand; + dysymtab_command* m_dysymtabCommand; nlist_64* m_nlists; - char* m_strtab; + uint64_t m_strtabAddress; public: MachOModule(MachOReader& reader, mach_vm_address_t baseAddress, mach_header_64* header = nullptr, std::string* name = nullptr); @@ -43,7 +44,8 @@ class MachOModule bool ReadLoadCommands(); bool ReadSymbolTable(); - void* GetAddressFromFileOffset(uint32_t offset); + uint64_t GetAddressFromFileOffset(uint32_t offset); + std::string GetSymbolName(int index); }; class MachOReader