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

@@ -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;
+		i32 offset;
-		memcpy(&offset, c-4, 4);
+		memcpy(&offset, c-sizeof(offset), sizeof(offset));
-		target = (void*)(uintptr_t(offset) + uintptr_t(ret_addr));
+		target = (void*)(intptr_t(ret_addr) + intptr_t(offset));
 		return true;
 	}
 
-	// CALL r/m32 (FF /2)
+	// CALL r32                    => FF D0-D7                 
-	// .. CALL [r32 + r32*s]          => FF 14 SIB
+	if(len >= 2 && c[-2] == 0xFF && (c[-1] & 0xF8) == 0xD0)
-	if(len >= 3 && c[-3] == 0xFF && c[-2] == 0x14)
 		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
+	// CALL [r32 + r32*s + disp32] => FF 94 SIB disp32
-	if(len >= 3 && c[-3] == 0xFF && (c[-2] & 0xF8) == 0x50 && c[-2] != 0x54)
-		return true;
-	// .. 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))

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

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 <queue>
 #include <deque>
 #include <list>
 #include <map>
 #include <stack>
 
+#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);
+		}
 	}
 };

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);

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;

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->AddrFrame .Offset = DWORD64(fp);
-	sf->AddrPC    .Offset = (DWORD64)target;
+	sf->AddrStack .Offset = DWORD64(prev_fp)+8;	// +8 reverts effects of call + push ebp
-	sf->AddrReturn.Offset = (DWORD64)ret_addr;
+	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;
 }
 

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<char*, char*> > 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<FormatVariable*>(*it);
+				FormatVariable* s = static_cast<FormatVariable*>(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)