From fc94ec4332f669b23ddbfc4a654b5ed7bcdebc9b Mon Sep 17 00:00:00 2001 From: Andy Gocke Date: Thu, 13 Jan 2022 02:53:37 -0800 Subject: [PATCH] Revert "Fix native frame unwind in syscall on arm64 for VS4Mac crash report. (#63598)" This reverts commit dc67c7022efae05891dfc1eb7b21afa001a1b99c. --- src/coreclr/debug/createdump/crashinfomac.cpp | 6 +- src/coreclr/debug/createdump/stackframe.h | 9 +- src/coreclr/debug/createdump/threadinfo.cpp | 10 -- .../pal/src/exception/remote-unwind.cpp | 129 +++++------------- 4 files changed, 38 insertions(+), 116 deletions(-) diff --git a/src/coreclr/debug/createdump/crashinfomac.cpp b/src/coreclr/debug/createdump/crashinfomac.cpp index 3ada3bd767c96..ad9c247e37dfd 100644 --- a/src/coreclr/debug/createdump/crashinfomac.cpp +++ b/src/coreclr/debug/createdump/crashinfomac.cpp @@ -277,9 +277,6 @@ void CrashInfo::VisitSegment(MachOModule& module, const segment_command_64& segm uint64_t start = segment.vmaddr + module.LoadBias(); uint64_t end = start + segment.vmsize; - // Add this module segment to the set used by the thread unwinding to lookup the module base address for an ip. - AddModuleAddressRange(start, end, module.BaseAddress()); - // Round to page boundary start = start & PAGE_MASK; _ASSERTE(start > 0); @@ -300,6 +297,9 @@ void CrashInfo::VisitSegment(MachOModule& module, const segment_command_64& segm } // Add this module segment to the module mappings list m_moduleMappings.insert(moduleRegion); + + // Add this module segment to the set used by the thread unwinding to lookup the module base address for an ip. + AddModuleAddressRange(start, end, module.BaseAddress()); } else { diff --git a/src/coreclr/debug/createdump/stackframe.h b/src/coreclr/debug/createdump/stackframe.h index 00c3a1cfb7fd8..75e20d93120c0 100644 --- a/src/coreclr/debug/createdump/stackframe.h +++ b/src/coreclr/debug/createdump/stackframe.h @@ -66,16 +66,9 @@ struct StackFrame } } -// See comment in threadinfo.cpp UnwindNativeFrames function -#if defined(__aarch64__) - #define STACK_POINTER_MASK ~0x7 -#else - #define STACK_POINTER_MASK ~0x0 -#endif - inline uint64_t ModuleAddress() const { return m_moduleAddress; } inline uint64_t InstructionPointer() const { return m_instructionPointer; } - inline uint64_t StackPointer() const { return m_stackPointer & STACK_POINTER_MASK; } + inline uint64_t StackPointer() const { return m_stackPointer; } inline uint32_t NativeOffset() const { return m_nativeOffset; } inline uint32_t Token() const { return m_token; } inline uint32_t ILOffset() const { return m_ilOffset; } diff --git a/src/coreclr/debug/createdump/threadinfo.cpp b/src/coreclr/debug/createdump/threadinfo.cpp index e64eafc8d29a9..82509f5275065 100644 --- a/src/coreclr/debug/createdump/threadinfo.cpp +++ b/src/coreclr/debug/createdump/threadinfo.cpp @@ -53,16 +53,6 @@ ThreadInfo::UnwindNativeFrames(CONTEXT* pContext) uint64_t ip = 0, sp = 0; GetFrameLocation(pContext, &ip, &sp); -#if defined(__aarch64__) - // ARM64 can have frames with the same SP but different IPs. Increment sp so it gets added to the stack - // frames in the correct order and to prevent the below loop termination on non-increasing sp. Since stack - // pointers are always 8 byte align, this increase is masked off in StackFrame::StackPointer() to get the - // original stack pointer. - if (sp == previousSp && ip != previousIp) - { - sp++; - } -#endif if (ip == 0 || sp <= previousSp) { TRACE_VERBOSE("Unwind: sp not increasing or ip == 0 sp %p ip %p\n", (void*)sp, (void*)ip); break; diff --git a/src/coreclr/pal/src/exception/remote-unwind.cpp b/src/coreclr/pal/src/exception/remote-unwind.cpp index b27fc9680bbed..af0293ba5bc60 100644 --- a/src/coreclr/pal/src/exception/remote-unwind.cpp +++ b/src/coreclr/pal/src/exception/remote-unwind.cpp @@ -57,7 +57,6 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. #include #include #include "compact_unwind_encoding.h" -#define MACOS_ARM64_POINTER_AUTH_MASK 0x7fffffffffffull #endif // Sub-headers included from the libunwind.h contain an empty struct @@ -1423,56 +1422,25 @@ StepWithCompactNoEncoding(const libunwindInfo* info) #if defined(TARGET_ARM64) -#define ARM64_SYSCALL_OPCODE 0xD4001001 -#define ARM64_BL_OPCODE_MASK 0xFC000000 -#define ARM64_BL_OPCODE 0x94000000 -#define ARM64_BLR_OPCODE_MASK 0xFFFFFC00 -#define ARM64_BLR_OPCODE 0xD63F0000 -#define ARM64_BLRA_OPCODE_MASK 0xFEFFF800 -#define ARM64_BLRA_OPCODE 0xD63F0800 - -static bool -StepWithCompactNoEncoding(const libunwindInfo* info) -{ - // Check that the function is a syscall "wrapper" and assume there is no frame and pop the return address. - uint32_t opcode; - unw_word_t addr = info->Context->Pc - sizeof(opcode); - if (!ReadValue32(info, &addr, &opcode)) { - ERROR("StepWithCompactNoEncoding: can read opcode %p\n", (void*)addr); - return false; - } - // Is the IP pointing just after a "syscall" opcode? - if (opcode != ARM64_SYSCALL_OPCODE) { - ERROR("StepWithCompactNoEncoding: not in syscall wrapper function\n"); - return false; - } - // Pop the return address from the stack - info->Context->Pc = info->Context->Lr; - TRACE("StepWithCompactNoEncoding: SUCCESS new pc %p sp %p\n", (void*)info->Context->Pc, (void*)info->Context->Sp); - return true; -} - -static bool +inline static bool ReadCompactEncodingRegister(const libunwindInfo* info, unw_word_t* addr, DWORD64* reg) { - uint64_t value; - if (!info->ReadMemory((PVOID)*addr, &value, sizeof(value))) { + *addr -= sizeof(uint64_t); + if (!ReadValue64(info, addr, (uint64_t*)reg)) { return false; } - *reg = VAL64(value); - *addr -= sizeof(uint64_t); return true; } -static bool -ReadCompactEncodingRegisterPair(const libunwindInfo* info, unw_word_t* addr, DWORD64* first, DWORD64* second) +inline static bool +ReadCompactEncodingRegisterPair(const libunwindInfo* info, unw_word_t* addr, DWORD64*second, DWORD64* first) { // Registers are effectively pushed in pairs // - // *first = **addr // *addr -= 8 - // *second= **addr + // **addr = *first // *addr -= 8 + // **addr = *second if (!ReadCompactEncodingRegister(info, addr, first)) { return false; } @@ -1482,8 +1450,8 @@ ReadCompactEncodingRegisterPair(const libunwindInfo* info, unw_word_t* addr, DWO return true; } -static bool -ReadCompactEncodingRegisterPair(const libunwindInfo* info, unw_word_t* addr, NEON128* first, NEON128* second) +inline static bool +ReadCompactEncodingRegisterPair(const libunwindInfo* info, unw_word_t* addr, NEON128*second, NEON128* first) { if (!ReadCompactEncodingRegisterPair(info, addr, &first->Low, &second->Low)) { return false; @@ -1516,28 +1484,30 @@ static bool StepWithCompactEncodingArm64(const libunwindInfo* info, compact_unwind_encoding_t compactEncoding, bool hasFrame) { CONTEXT* context = info->Context; - unw_word_t addr; - if (hasFrame) - { - context->Sp = context->Fp + 16; - addr = context->Fp + 8; - if (!ReadCompactEncodingRegisterPair(info, &addr, &context->Lr, &context->Fp)) { - return false; - } - // Strip pointer authentication bits - context->Lr &= MACOS_ARM64_POINTER_AUTH_MASK; - } - else - { + unw_word_t callerSp; + + if (hasFrame) { + // caller Sp is callee Fp plus saved FP and LR + callerSp = context->Fp + 2 * sizeof(uint64_t); + } else { // Get the leat significant bit in UNWIND_ARM64_FRAMELESS_STACK_SIZE_MASK uint64_t stackSizeScale = UNWIND_ARM64_FRAMELESS_STACK_SIZE_MASK & ~(UNWIND_ARM64_FRAMELESS_STACK_SIZE_MASK - 1); - uint64_t stackSize = ((compactEncoding & UNWIND_ARM64_FRAMELESS_STACK_SIZE_MASK) / stackSizeScale) * 16; + uint64_t stackSize = (compactEncoding & UNWIND_ARM64_FRAMELESS_STACK_SIZE_MASK) / stackSizeScale * 16; - addr = context->Sp + stackSize; + callerSp = context->Sp + stackSize; } - // Unwound return address is stored in Lr + context->Sp = callerSp; + + unw_word_t addr = callerSp; + + if (hasFrame && + !ReadCompactEncodingRegisterPair(info, &addr, &context->Lr, &context->Fp)) { + return false; + } + + // unwound return address is stored in Lr context->Pc = context->Lr; if (compactEncoding & UNWIND_ARM64_FRAME_X19_X20_PAIR && @@ -1576,10 +1546,7 @@ StepWithCompactEncodingArm64(const libunwindInfo* info, compact_unwind_encoding_ !ReadCompactEncodingRegisterPair(info, &addr, &context->V[14], &context->V[15])) { return false; } - if (!hasFrame) - { - context->Sp = addr; - } + TRACE("SUCCESS: compact step encoding %08x pc %p sp %p fp %p lr %p\n", compactEncoding, (void*)context->Pc, (void*)context->Sp, (void*)context->Fp, (void*)context->Lr); return true; @@ -1590,11 +1557,11 @@ StepWithCompactEncodingArm64(const libunwindInfo* info, compact_unwind_encoding_ static bool StepWithCompactEncoding(const libunwindInfo* info, compact_unwind_encoding_t compactEncoding, unw_word_t functionStart) { +#if defined(TARGET_AMD64) if (compactEncoding == 0) { return StepWithCompactNoEncoding(info); } -#if defined(TARGET_AMD64) switch (compactEncoding & UNWIND_X86_64_MODE_MASK) { case UNWIND_X86_64_MODE_RBP_FRAME: @@ -1608,6 +1575,11 @@ StepWithCompactEncoding(const libunwindInfo* info, compact_unwind_encoding_t com return false; } #elif defined(TARGET_ARM64) + if (compactEncoding == 0) + { + TRACE("Compact unwind missing for %p\n", (void*)info->Context->Pc); + return false; + } switch (compactEncoding & UNWIND_ARM64_MODE_MASK) { case UNWIND_ARM64_MODE_FRAME: @@ -1745,12 +1717,6 @@ static void UnwindContextToContext(unw_cursor_t *cursor, CONTEXT *winContext) unw_get_reg(cursor, UNW_AARCH64_X28, (unw_word_t *) &winContext->X28); unw_get_reg(cursor, UNW_AARCH64_X29, (unw_word_t *) &winContext->Fp); unw_get_reg(cursor, UNW_AARCH64_X30, (unw_word_t *) &winContext->Lr); -#ifdef __APPLE__ - // Strip pointer authentication bits which seem to be leaking out of libunwind - // Seems like ptrauth_strip() / __builtin_ptrauth_strip() should work, but currently - // errors with "this target does not support pointer authentication" - winContext->Pc = winContext->Pc & MACOS_ARM64_POINTER_AUTH_MASK; -#endif // __APPLE__ TRACE("sp %p pc %p lr %p fp %p\n", winContext->Sp, winContext->Pc, winContext->Lr, winContext->Fp); #elif defined(TARGET_S390X) unw_get_reg(cursor, UNW_REG_IP, (unw_word_t *) &winContext->PSWAddr); @@ -2160,33 +2126,6 @@ PAL_VirtualUnwindOutOfProc(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *cont #elif defined(TARGET_ARM64) TRACE("Unwind: pc %p sp %p fp %p\n", (void*)context->Pc, (void*)context->Sp, (void*)context->Fp); result = GetProcInfo(context->Pc, &procInfo, &info, &step, false); - if (result && step) - { - // If the PC is at the start of the function, the previous instruction is BL and the unwind encoding is frameless - // with nothing on stack (0x02000000), back up PC by 1 to the previous function and get the unwind info for that - // function. - if ((context->Pc == procInfo.start_ip) && - (procInfo.format & (UNWIND_ARM64_MODE_MASK | UNWIND_ARM64_FRAMELESS_STACK_SIZE_MASK)) == UNWIND_ARM64_MODE_FRAMELESS) - { - uint32_t opcode; - unw_word_t addr = context->Pc - sizeof(opcode); - if (ReadValue32(&info, &addr, &opcode)) - { - // Is the previous instruction a BL opcode? - if ((opcode & ARM64_BL_OPCODE_MASK) == ARM64_BL_OPCODE || - (opcode & ARM64_BLR_OPCODE_MASK) == ARM64_BLR_OPCODE || - (opcode & ARM64_BLRA_OPCODE_MASK) == ARM64_BLRA_OPCODE) - { - TRACE("Unwind: getting unwind info for PC - 1 opcode %08x\n", opcode); - result = GetProcInfo(context->Pc - 1, &procInfo, &info, &step, false); - } - else - { - TRACE("Unwind: not BL* opcode %08x\n", opcode); - } - } - } - } #else #error Unexpected architecture #endif