From e29d5a779f397f1cd361f9cb82f046a4aa95935e Mon Sep 17 00:00:00 2001 From: janwas Date: Wed, 4 Nov 2009 14:24:54 +0000 Subject: [PATCH] fix: wprintf: buffer mustn't be unicode (caught by self-test) test_wdbg_sym - reenable, add test for basic stack walking ia32_GetCallTarget - move frequent cases to front, add support for ILT function trampolines wdbg_sym: fix: ia32_walk_stack wasn't setting AddrStack; fix string output This was SVN commit r7163. --- source/lib/sysdep/arch/ia32/ia32.cpp | 55 ++++++++----- source/lib/sysdep/arch/ia32/ia32.h | 2 +- .../lib/sysdep/os/win/tests/test_wdbg_sym.h | 78 ++++++++++++++----- source/lib/sysdep/os/win/wdbg.cpp | 2 +- source/lib/sysdep/os/win/wdbg_heap.cpp | 2 +- source/lib/sysdep/os/win/wdbg_sym.cpp | 13 ++-- source/lib/sysdep/os/win/wprintf.cpp | 15 ++-- 7 files changed, 111 insertions(+), 56 deletions(-) diff --git a/source/lib/sysdep/arch/ia32/ia32.cpp b/source/lib/sysdep/arch/ia32/ia32.cpp index c5c9b628af..54041bce70 100644 --- a/source/lib/sysdep/arch/ia32/ia32.cpp +++ b/source/lib/sysdep/arch/ia32/ia32.cpp @@ -41,24 +41,30 @@ static bool IsCall(void* ret_addr, void*& target) // because this is really unlikely and not worth the trouble. const size_t len = maxInstructionLength; + // (most frequent cases first to reduce stack walk overhead:) + // CALL rel32 (E8 cd) if(len >= 5 && c[-5] == 0xE8) { - void* offset; - memcpy(&offset, c-4, 4); - target = (void*)(uintptr_t(offset) + uintptr_t(ret_addr)); + i32 offset; + memcpy(&offset, c-sizeof(offset), sizeof(offset)); + target = (void*)(intptr_t(ret_addr) + intptr_t(offset)); return true; } - // CALL r/m32 (FF /2) - // .. CALL [r32 + r32*s] => FF 14 SIB - if(len >= 3 && c[-3] == 0xFF && c[-2] == 0x14) + // CALL r32 => FF D0-D7 + if(len >= 2 && c[-2] == 0xFF && (c[-1] & 0xF8) == 0xD0) return true; - // .. CALL [disp32] => FF 15 disp32 + + // CALL [r32 + disp8] => FF 50-57(!54) disp8 + if(len >= 3 && c[-3] == 0xFF && (c[-2] & 0xF8) == 0x50 && c[-2] != 0x54) + return true; + + // CALL [disp32] => FF 15 disp32 if(len >= 6 && c[-6] == 0xFF && c[-5] == 0x15) { void** addr_of_target; - memcpy(&addr_of_target, c-4, 4); + memcpy(&addr_of_target, c-sizeof(addr_of_target), sizeof(addr_of_target)); target = *addr_of_target; // there are no meaningful checks we can perform: we're called from // the stack trace code, so ring0 addresses may be legit. @@ -66,32 +72,41 @@ static bool IsCall(void* ret_addr, void*& target) // (may help in tracking down memory corruption) return true; } - // .. CALL [r32] => FF 00-3F(!14/15) + + // CALL [r32 + r32*s] => FF 14 SIB + if(len >= 3 && c[-3] == 0xFF && c[-2] == 0x14) + return true; + // CALL [r32] => FF 00-3F(!14/15) if(len >= 2 && c[-2] == 0xFF && c[-1] < 0x40 && c[-1] != 0x14 && c[-1] != 0x15) return true; - // .. CALL [r32 + r32*s + disp8] => FF 54 SIB disp8 + // CALL [r32 + r32*s + disp8] => FF 54 SIB disp8 if(len >= 4 && c[-4] == 0xFF && c[-3] == 0x54) return true; - // .. CALL [r32 + disp8] => FF 50-57(!54) disp8 - if(len >= 3 && c[-3] == 0xFF && (c[-2] & 0xF8) == 0x50 && c[-2] != 0x54) - return true; - // .. CALL [r32 + r32*s + disp32] => FF 94 SIB disp32 + // CALL [r32 + r32*s + disp32] => FF 94 SIB disp32 if(len >= 7 && c[-7] == 0xFF && c[-6] == 0x94) return true; - // .. CALL [r32 + disp32] => FF 90-97(!94) disp32 + // CALL [r32 + disp32] => FF 90-97(!94) disp32 if(len >= 6 && c[-6] == 0xFF && (c[-5] & 0xF8) == 0x90 && c[-5] != 0x94) return true; - // .. CALL r32 => FF D0-D7 - if(len >= 2 && c[-2] == 0xFF && (c[-1] & 0xF8) == 0xD0) - return true; return false; } -LibError ia32_GetCallTarget(void* ret_addr, void** target) +LibError ia32_GetCallTarget(void* ret_addr, void*& target) { - if(IsCall(ret_addr, *target)) + if(IsCall(ret_addr, target)) + { + // follow the incremental linker's jump tables + const u8* c = (const u8*)target; + if(c && c[0] == 0xE9) + { + i32 offset; + memcpy(&offset, c+1, sizeof(offset)); + target = (void*)(intptr_t(c)+5 + intptr_t(offset)); + } + return INFO::OK; + } const u8* const instructionWindow = (const u8*)ret_addr - maxInstructionLength; if(memchr(instructionWindow, 0xCC, maxInstructionLength)) diff --git a/source/lib/sysdep/arch/ia32/ia32.h b/source/lib/sysdep/arch/ia32/ia32.h index 150503d5be..1073d3473d 100644 --- a/source/lib/sysdep/arch/ia32/ia32.h +++ b/source/lib/sysdep/arch/ia32/ia32.h @@ -36,6 +36,6 @@ * * this function is used for walking the call stack. **/ -LIB_API LibError ia32_GetCallTarget(void* ret_addr, void** target); +LIB_API LibError ia32_GetCallTarget(void* ret_addr, void*& target); #endif // #ifndef INCLUDED_IA32 diff --git a/source/lib/sysdep/os/win/tests/test_wdbg_sym.h b/source/lib/sysdep/os/win/tests/test_wdbg_sym.h index f9441c44d4..87bec435e1 100644 --- a/source/lib/sysdep/os/win/tests/test_wdbg_sym.h +++ b/source/lib/sysdep/os/win/tests/test_wdbg_sym.h @@ -22,17 +22,47 @@ #include "lib/self_test.h" -#include "lib/sysdep/os/win/win.h" // HWND -#include "lib/debug.h" // no wdbg_sym interface needed -#include "lib/sysdep/sysdep.h" -#include "lib/sysdep/os/win/win.h" - #include #include #include #include #include +#include "lib/sysdep/os/win/win.h" // HWND +#include "lib/sysdep/sysdep.h" +#include "lib/sysdep/os/win/wdbg_sym.h" +#include "lib/external_libraries/dbghelp.h" + + +static void* callers[100]; +static size_t numCallers; + +static LibError OnFrame(const _tagSTACKFRAME64* frame, uintptr_t UNUSED(cbData)) +{ + callers[numCallers++] = (void*)frame->AddrPC.Offset; + return INFO::CB_CONTINUE; +} + +// (these must be outside of TestWdbgSym so that we can simply +// search for the function's name as a substring within the ILT +// decorated name (which omits the :: scope resolution operator, +// while debug_ResolveSymbol for the function does not) +__declspec(noinline) static void Func1() +{ + wdbg_sym_WalkStack(OnFrame, 0, 0, L"wdbg_sym_WalkStack"); +} + +__declspec(noinline) static void Func2() +{ + Func1(); +} + +__declspec(noinline) static void Func3() +{ + Func2(); +} + + class TestWdbgSym : public CxxTest::TestSuite { #pragma optimize("", off) @@ -61,24 +91,16 @@ class TestWdbgSym : public CxxTest::TestSuite int ints[] = { 1,2,3,4,5 }; UNUSED2(ints); wchar_t chars[] = { 'w','c','h','a','r','s',0 }; UNUSED2(chars); - // note: prefer simple error (which also generates stack trace) to - // exception, because it is guaranteed to work (no issues with the - // debugger swallowing exceptions). - //DEBUG_DISPLAY_ERROR(L"wdbg_sym self test: check if stack trace below is ok."); - //RaiseException(0xf001,0,0,0); - // note: we don't want any kind of dialog to be raised, because // this test now always runs. therefore, just make sure a decent // amount of text (not just "(failed)" error messages) was produced. - // - // however, we can't call debug_DumpStack directly because - // it'd be reentered if an actual error comes up. - // therefore, use debug_DisplayError with DE_HIDE_DIALOG. - // unfortunately this means we can no longer get at the error text. - // a sanity check of the text length has been added to debug_DisplayError ErrorMessageMem emm = {0}; - const wchar_t* text = debug_BuildErrorMessage(L"dummy", 0,0,0, 0,0, &emm); + const wchar_t* text = debug_BuildErrorMessage(L"dummy", 0,0,0, 0,L"debug_BuildErrorMessage", &emm); TS_ASSERT(wcslen(text) > 500); + { + std::wofstream s(L"d:\\out.txt"); + s << text; + } debug_FreeErrorMessage(&emm); } @@ -271,7 +293,23 @@ class TestWdbgSym : public CxxTest::TestSuite public: void test_stack_trace() { - // TODO: restore this when it doesn't cause annoying assertion failures -// m_test_addrs(123, 3.1415926535897932384626, "pchar string", 0xf00d); + m_test_addrs(123, 3.1415926535897932384626, "pchar string", 0xf00d); + } + + void test_stack_walk() + { + Func3(); + TS_ASSERT(numCallers >= 3); + void* funcAddresses[3] = { (void*)&Func1, (void*)&Func2, (void*)&Func3 }; + for(size_t i = 0; i < 3; i++) + { + wchar_t func1[DBG_SYMBOL_LEN], func2[DBG_SYMBOL_LEN]; + LibError ret; + ret = debug_ResolveSymbol(callers[i], func1, 0, 0); + TS_ASSERT_OK(ret); + ret = debug_ResolveSymbol(funcAddresses[i], func2, 0, 0); + TS_ASSERT_OK(ret); + TS_ASSERT_WSTR_CONTAINS(func2, func1); + } } }; diff --git a/source/lib/sysdep/os/win/wdbg.cpp b/source/lib/sysdep/os/win/wdbg.cpp index f17f7e110a..27ec106bfc 100644 --- a/source/lib/sysdep/os/win/wdbg.cpp +++ b/source/lib/sysdep/os/win/wdbg.cpp @@ -95,7 +95,7 @@ void debug_puts(const wchar_t* text) void wdbg_printf(const wchar_t* fmt, ...) { - wchar_t buf[1024]; // as required by wvsprintf + wchar_t buf[1024/sizeof(wchar_t)]; // as required by wvsprintf va_list ap; va_start(ap, fmt); wvsprintfW(buf, fmt, ap); diff --git a/source/lib/sysdep/os/win/wdbg_heap.cpp b/source/lib/sysdep/os/win/wdbg_heap.cpp index 12d03653d2..61a5db824c 100644 --- a/source/lib/sysdep/os/win/wdbg_heap.cpp +++ b/source/lib/sysdep/os/win/wdbg_heap.cpp @@ -842,7 +842,7 @@ static void PrintCallStack(const uintptr_t* callers, size_t numCallers) return; } - wdbg_printf(L"\n partial call stack:\n"); + wdbg_printf(L"\n partial, unordered call stack:\n"); for(size_t i = 0; i < numCallers; i++) { wchar_t name[DBG_SYMBOL_LEN] = {'\0'}; wchar_t file[DBG_FILE_LEN] = {'\0'}; int line = -1; diff --git a/source/lib/sysdep/os/win/wdbg_sym.cpp b/source/lib/sysdep/os/win/wdbg_sym.cpp index 8be4966e1c..c3bf095da5 100644 --- a/source/lib/sysdep/os/win/wdbg_sym.cpp +++ b/source/lib/sysdep/os/win/wdbg_sym.cpp @@ -276,7 +276,7 @@ static LibError ia32_walk_stack(_tagSTACKFRAME64* sf) return ERR::FAIL; // NOWARN (invalid address) void* target; - LibError err = ia32_GetCallTarget(ret_addr, &target); + LibError err = ia32_GetCallTarget(ret_addr, target); RETURN_ERR(err); if(target) // were able to determine it from the call instruction { @@ -284,9 +284,10 @@ static LibError ia32_walk_stack(_tagSTACKFRAME64* sf) return ERR::FAIL; // NOWARN (invalid address) } - sf->AddrFrame .Offset = (DWORD64)fp; - sf->AddrPC .Offset = (DWORD64)target; - sf->AddrReturn.Offset = (DWORD64)ret_addr; + sf->AddrFrame .Offset = DWORD64(fp); + sf->AddrStack .Offset = DWORD64(prev_fp)+8; // +8 reverts effects of call + push ebp + sf->AddrPC .Offset = DWORD64(target); + sf->AddrReturn.Offset = DWORD64(ret_addr); return INFO::OK; } @@ -1024,7 +1025,7 @@ static LibError dump_sym_base_type(DWORD type_id, const u8* p, DumpState state) case btBool: debug_assert(size == sizeof(bool)); if(data == 0 || data == 1) - out(L"%hs", data? "true " : "false"); + out(L"%ls", data? L"true " : L"false"); else out(L"(bool)0x%02I64X", data); break; @@ -1229,7 +1230,7 @@ static LibError dump_sym_function_type(DWORD UNUSED(type_id), const u8* p, DumpS if(state.indirection == 0) out(L"0x%p ", p); if(err == INFO::OK) - out(L"(%hs)", name); + out(L"(%ls)", name); return INFO::OK; } diff --git a/source/lib/sysdep/os/win/wprintf.cpp b/source/lib/sysdep/os/win/wprintf.cpp index d985197acb..f2c812eded 100644 --- a/source/lib/sysdep/os/win/wprintf.cpp +++ b/source/lib/sysdep/os/win/wprintf.cpp @@ -441,14 +441,14 @@ finished_reading: #endif // Highly efficient buffer to store the rearranged copy of the stack - std::tstring newstack; + std::string newstack; std::vector< std::pair > stackitems; va_list arglist = argptr; //va_start(arglist, format); - const TCHAR* newstackptr; + const u8* newstackptr; if (varsizes.size()) { @@ -474,23 +474,24 @@ finished_reading: for (ChunkIt it = specs.begin(); it != specs.end(); ++it) { - if ((*it)->ChunkType() == 0) + FormatChunk* chunk = *it; + if (chunk->ChunkType() == 0) { - FormatVariable* s = static_cast(*it); + FormatVariable* s = static_cast(chunk); if (s->position <= 0) { debug_assert(0); // Invalid use of positional elements - make sure all variable things are positional and defined return -1; } - newstack += std::tstring( stackitems[s->position-1].first, stackitems[s->position-1].second ); + newstack += std::string( stackitems[s->position-1].first, stackitems[s->position-1].second ); } } - newstackptr = newstack.c_str(); + newstackptr = (const u8*)newstack.c_str(); } else { - newstackptr = (const TCHAR*)arglist; + newstackptr = (const u8*)arglist; } for (ChunkIt it = specs.begin(); it != specs.end(); ++it)