diff --git a/source/lib/debug.cpp b/source/lib/debug.cpp index 7234e5c79d..a1d4b0dc95 100644 --- a/source/lib/debug.cpp +++ b/source/lib/debug.cpp @@ -216,6 +216,11 @@ void debug_wprintf(const wchar_t* fmt, ...) LibError debug_write_crashlog(const wchar_t* text) { + // avoid potential infinite loop if an error occurs here. + static uintptr_t in_progress; + if(!CAS(&in_progress, 0, 1)) + return ERR_REENTERED; // NOWARN + // note: we go through some gyrations here (strcpy+strcat) to avoid // dependency on file code (path_append). char N_path[PATH_MAX]; @@ -223,7 +228,10 @@ LibError debug_write_crashlog(const wchar_t* text) strcat_s(N_path, ARRAY_SIZE(N_path), "crashlog.txt"); FILE* f = fopen(N_path, "w"); if(!f) + { + in_progress = 0; WARN_RETURN(ERR_FILE_ACCESS); + } fputwc(0xfeff, f); // BOM fwprintf(f, L"%ls\n", text); @@ -235,6 +243,7 @@ LibError debug_write_crashlog(const wchar_t* text) fwprintf(f, L"Last known activity:\n\n %ls\n", debug_log); fclose(f); + in_progress = 0; return INFO_OK; } @@ -475,19 +484,63 @@ static bool should_suppress_error(u8* suppress) return false; } -static const wchar_t* build_error_message(wchar_t* buf, size_t max_chars, +static wchar_t* alloc_mem(void* alloca_buf, size_t alloca_buf_size, + void*& heap_mem, size_t& max_chars) +{ + void* chosen_buf; + size_t chosen_size; + + // rationale: + // - this needs to be quite large, so preallocating is undesirable. + // - prefer malloc to alloca because it allows returning larger + // buffers (stack space may be quite limited). + // - do not rely on malloc because we might be called upon to report + // heap corruption errors. therefore, the caller should allocate some + // scratch memory via alloca, which is used as an (optional) backup. + // - note: we can't alloca here because it'd be lost after + // function return, but must be passed on to debug_display_error. + + // try allocating from heap. + chosen_size = 500*KiB; // 'enough' + chosen_buf = heap_mem = malloc(chosen_size); + // .. failed; use alloca_buf. + if(!chosen_buf) + { + // caller didn't set it up => we have no memory to return; abort. + if(!alloca_buf) + return 0; + + chosen_buf = alloca_buf; + chosen_size = alloca_buf_size; + } + + max_chars = chosen_size / sizeof(wchar_t); + return (wchar_t*)chosen_buf; +} + + +void debug_error_message_free(ErrorMessageMem* emm) +{ + // note: no-op if wasn't allocated from heap. + free(emm->heap_mem); +} + +// split out of debug_display_error because it's used by the self-test. +const wchar_t* debug_error_message_build( const wchar_t* description, const char* fn_only, int line, const char* func, uint skip, void* context, - bool is_nested_error) + ErrorMessageMem* emm) { + size_t max_chars; + wchar_t* buf = alloc_mem(emm->alloca_buf, emm->alloca_buf_size, emm->heap_mem, max_chars); if(!buf) return L"(insufficient memory to generate error message)"; - char errno_description[100] = {'?'}; + char description_buf[100] = {'?'}; LibError errno_equiv = LibError_from_errno(false); if(errno_equiv != ERR_FAIL) // meaningful translation - error_description_r(errno_equiv, errno_description, ARRAY_SIZE(errno_description)); + error_description_r(errno_equiv, description_buf, ARRAY_SIZE(description_buf)); char os_error[100]; if(sys_error_description_r(0, os_error, ARRAY_SIZE(os_error)) != INFO_OK) @@ -504,7 +557,7 @@ static const wchar_t* build_error_message(wchar_t* buf, size_t max_chars, int len = swprintf(buf,max_chars,fmt, description, fn_only, line, func, - errno, errno_description, + errno, description_buf, os_error ); if(len < 0) @@ -512,21 +565,23 @@ static const wchar_t* build_error_message(wchar_t* buf, size_t max_chars, // add stack trace to end of message wchar_t* pos = buf+len; const size_t chars_left = max_chars-len; - if(!is_nested_error) - { - if(!context) - skip++; // skip this frame - debug_dump_stack(pos, chars_left, skip, context); - } - // .. except when a stack trace is currently already in progress - // (debug_dump_stack is not reentrant due to use of global buffer!) - else + if(!context) + skip += 2; // skip debug_error_message_build and debug_display_error + LibError ret = debug_dump_stack(pos, chars_left, skip, context); + if(ret == ERR_REENTERED) { wcscpy_s(pos, chars_left, L"(cannot start a nested stack trace; what probably happened is that " L"an debug_assert/debug_warn/CHECK_ERR fired during the current trace.)" ); } + else if(ret != INFO_OK) + { + swprintf(pos, chars_left, + L"(error while dumping stack: %hs)", + error_description_r(ret, description_buf, ARRAY_SIZE(description_buf)) + ); + } return buf; } @@ -606,49 +661,25 @@ ErrorReaction debug_display_error(const wchar_t* description, // which is rather long. we only display the base name for clarity. const char* fn_only = path_name_only(file); - // display in output window; double-click will navigate to error location. debug_wprintf(L"%hs(%d): %ls\n", fn_only, line, description); - // allocate memory for the error message. this needs to be quite large, - // so preallocating is undesirable. - // note: this code can't be moved into a subroutine due to alloca. - wchar_t* buf = 0; - size_t max_chars = 256*KiB; - // .. try allocating from heap. can't rely on this because we might - // be called upon to report heap corruption errors. - void* heap_mem = malloc((max_chars+1)*sizeof(wchar_t)); - buf = (wchar_t*)heap_mem; - // .. heap alloc failed; try allocating from stack. if this fails, - // we give up and simply display a static error message. - if(!buf) - { - max_chars = 128*KiB; // (stack limit is usually 1 MiB) - buf = (wchar_t*)alloca((max_chars+1)*sizeof(wchar_t)); - } - - static uintptr_t already_in_progress; - const bool is_nested = !CAS(&already_in_progress, 0, 1); - - const wchar_t* text = build_error_message(buf, max_chars, description, - fn_only, line, func, skip, context, is_nested); - - if(!is_nested) // avoids potential infinite loop - debug_write_crashlog(text); + ErrorMessageMem emm; + emm.alloca_buf_size = 50000; + emm.alloca_buf = alloca(emm.alloca_buf_size); + const wchar_t* text = debug_error_message_build(description, + fn_only, line, func, skip, context, &emm); + debug_write_crashlog(text); ErrorReaction er = call_display_error(text, flags); - // note: debug_break-ing here to make sure the app doesn't continue // running is no longer necessary. debug_display_error now determines our // window handle and is modal. // must happen before carry_out_ErrorReaction because that may exit. - // note: no-op if not allocated from heap. - free(heap_mem); - - already_in_progress = 0; + debug_error_message_free(&emm); return carry_out_ErrorReaction(er, flags, suppress); } diff --git a/source/lib/debug.h b/source/lib/debug.h index 7c3703d41c..2827036277 100644 --- a/source/lib/debug.h +++ b/source/lib/debug.h @@ -284,7 +284,14 @@ extern void debug_filter_clear(); extern void debug_wprintf_mem(const wchar_t* fmt, ...); /** - * write all logs and out to crashlog.txt (unicode format). + * write an error description and all logs into crashlog.txt + * (in unicode format). + * + * @param text description of the error (including stack trace); + * typically generated by debug_error_message_build. + * + * @return LibError; ERR_REENTERED if reentered via recursion or + * multithreading (not allowed since an infinite loop may result). **/ extern LibError debug_write_crashlog(const wchar_t* text); @@ -509,12 +516,10 @@ extern LibError debug_resolve_symbol(void* ptr_of_interest, char* sym_name, char * @param context platform-specific representation of execution state * (e.g. Win32 CONTEXT). if not NULL, tracing starts there; this is useful * for exceptions. otherwise, tracing starts from the current call stack. - * @return buf for convenience; writes an error string into it if - * something goes wrong. - * - * not reentrant! (pointer to buf is stored in static variable) + * @return LibError; ERR_REENTERED if reentered via recursion or + * multithreading (not allowed since static data is used). **/ -extern const wchar_t* debug_dump_stack(wchar_t* buf, size_t max_chars, uint skip, void* context); +extern LibError debug_dump_stack(wchar_t* buf, size_t max_chars, uint skip, void* context); //----------------------------------------------------------------------------- @@ -582,6 +587,48 @@ extern void debug_set_thread_name(const char* name); extern const char* debug_get_thread_name(); +/** + * holds memory for an error message. + **/ +struct ErrorMessageMem +{ + // passed to debug_error_message_build from caller + void* alloca_buf; + size_t alloca_buf_size; + + // allocated within debug_error_message_build + void* heap_mem; +}; + +/** + * free memory from the error message. + * + * @param ErrorMessageMem* + **/ +extern void debug_error_message_free(ErrorMessageMem* emm); + +/** + * build a string describing the given error. + * + * this is a helper function used by debug_dump_stack and is made available + * so that the self-test doesn't have to display the error dialog. + * + * @param description: general description of the problem. + * @param fn_only filename (no path) of source file that triggered the error. + * @param line, func: exact position of the error. + * @param skip, context: see debug_dump_stack. + * @param emm memory for the error message. caller should allocate + * stack memory and set alloc_buf*; if not, there will be no + * fallback in case heap alloc fails. should be freed via + * debug_error_message_free when no longer needed. + **/ +extern const wchar_t* debug_error_message_build( + const wchar_t* description, + const char* fn_only, int line, const char* func, + uint skip, void* context, + ErrorMessageMem* emm); + + /** * call at exit to avoid some leaks. * not strictly necessary. diff --git a/source/lib/lib_errors.cpp b/source/lib/lib_errors.cpp index fe46577809..de7ffc2467 100644 --- a/source/lib/lib_errors.cpp +++ b/source/lib/lib_errors.cpp @@ -55,7 +55,7 @@ static const char* LibError_description(LibError err) // stores up to in the given buffer. // if error is unknown/invalid, the string will be something like // "Unknown error (65536, 0x10000)". -void error_description_r(LibError err, char* buf, size_t max_chars) +char* error_description_r(LibError err, char* buf, size_t max_chars) { // lib error const char* str = LibError_description(err); @@ -64,11 +64,14 @@ void error_description_r(LibError err, char* buf, size_t max_chars) // was one of our error codes (chosen so as not to conflict // with any others), so we're done. strcpy_s(buf, max_chars, str); - return; + } + // unknown + else + { + snprintf(buf, max_chars, "Unknown error (%d, 0x%X)", err, err); } - // fallback - snprintf(buf, max_chars, "Unknown error (%d, 0x%X)", err, err); + return buf; } diff --git a/source/lib/lib_errors.h b/source/lib/lib_errors.h index f115f43d37..de8d4420b1 100644 --- a/source/lib/lib_errors.h +++ b/source/lib/lib_errors.h @@ -153,8 +153,9 @@ enum LibError { * "Unknown error (65536, 0x10000)". * @param buf destination buffer * @param max_chars size of buffer [characters] + * @return buf (allows using this function in expressions) **/ -extern void error_description_r(LibError err, char* buf, size_t max_chars); +extern char* error_description_r(LibError err, char* buf, size_t max_chars); //----------------------------------------------------------------------------- @@ -365,7 +366,8 @@ ERR(106, INFO_ALREADY_EXISTS, "6 (not an error)") ERR(-100000, ERR_LOGIC, "Logic error in code") ERR(-100001, ERR_TIMED_OUT, "Timed out") -ERR(-100002, ERR_STRING_NOT_TERMINATED, "Invalid string (no 0 terminator found in buffer)") +ERR(-100002, ERR_REENTERED, "Single-call function was reentered") +ERR(-100010, ERR_STRING_NOT_TERMINATED, "Invalid string (no 0 terminator found in buffer)") // these are for cases where we just want a distinct value to display and diff --git a/source/lib/sysdep/ia32.cpp b/source/lib/sysdep/ia32.cpp index 7390b270ab..feafd539d3 100644 --- a/source/lib/sysdep/ia32.cpp +++ b/source/lib/sysdep/ia32.cpp @@ -644,7 +644,7 @@ LibError ia32_get_call_target(void* ret_addr, void** target) if(len >= 3 && c[-3] == 0xFF && c[-2] == 0x14) return INFO_OK; // .. CALL [disp32] => FF 15 disp32 - if(len >= 6 && c[6] == 0xFF && c[-5] == 0x15) + if(len >= 6 && c[-6] == 0xFF && c[-5] == 0x15) { void* addr_of_target = *(void**)(c-4); // there are no meaningful checks we can perform: we're called from diff --git a/source/lib/sysdep/unix/udbg.cpp b/source/lib/sysdep/unix/udbg.cpp index 324646f277..745739e5e2 100644 --- a/source/lib/sysdep/unix/udbg.cpp +++ b/source/lib/sysdep/unix/udbg.cpp @@ -116,7 +116,7 @@ void* debug_get_nth_caller(uint n, void *UNUSED(context)) return bt[n+1]; // n==1 => bt[2], and so forth } -const wchar_t* debug_dump_stack(wchar_t* buf, size_t max_chars, uint skip, void* UNUSED(context)) +LibError debug_dump_stack(wchar_t* buf, size_t max_chars, uint skip, void* UNUSED(context)) { ++skip; // Skip ourselves too @@ -159,7 +159,7 @@ const wchar_t* debug_dump_stack(wchar_t* buf, size_t max_chars, uint skip, void* bufpos += len; } - return buf; + return INFO_OK; } static int slurp_symtab(symbol_file_context *ctx) diff --git a/source/lib/sysdep/win/tests/test_wdbg_sym.h b/source/lib/sysdep/win/tests/test_wdbg_sym.h index d98d1ec0a7..bbad14f624 100644 --- a/source/lib/sysdep/win/tests/test_wdbg_sym.h +++ b/source/lib/sysdep/win/tests/test_wdbg_sym.h @@ -53,9 +53,16 @@ class TestWdbgSym : public CxxTest::TestSuite // 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. - wchar_t buf[60000] = {'\0'}; - debug_dump_stack(buf, ARRAY_SIZE(buf), 0, 0); - TS_ASSERT(wcslen(buf) > 500); + // + // however, we can't call debug_dump_stack directly because + // it'd be reentered if an actual error comes up. + // therefore, use debug_display_error 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_display_error + ErrorMessageMem emm = {0,0,0}; + const wchar_t* text = debug_error_message_build(L"dummy", 0,0,0, 0,0, &emm); + TS_ASSERT(wcslen(text) > 500); + debug_error_message_free(&emm); } // also used by test_stl as an element type diff --git a/source/lib/sysdep/win/wdbg.cpp b/source/lib/sysdep/win/wdbg.cpp index 752b2e8353..bf11cc3b49 100644 --- a/source/lib/sysdep/win/wdbg.cpp +++ b/source/lib/sysdep/win/wdbg.cpp @@ -804,7 +804,7 @@ bool debug_is_stack_ptr(void* p) // not aligned if(addr % sizeof(void*)) return false; - // out of bounds (note: IA32 stack grows downwards) + // out of bounds (note: IA-32 stack grows downwards) NT_TIB* tib = get_tib(); if(!(tib->StackLimit < p && p < tib->StackBase)) return false; diff --git a/source/lib/sysdep/win/wdbg_sym.cpp b/source/lib/sysdep/win/wdbg_sym.cpp index 145a853e39..a04ee29d29 100644 --- a/source/lib/sysdep/win/wdbg_sym.cpp +++ b/source/lib/sysdep/win/wdbg_sym.cpp @@ -65,7 +65,6 @@ static void lock() { win_lock(WDBG_CS); } - static void unlock() { win_unlock(WDBG_CS); @@ -158,22 +157,15 @@ struct TI_FINDCHILDREN_PARAMS2 }; -// read and return symbol information for the given address. all of the -// output parameters are optional; we pass back as much information as is -// available and desired. return 0 iff any information was successfully -// retrieved and stored. -// sym_name and file must hold at least the number of chars above; -// file is the base name only, not path (see rationale in wdbg_sym). -// the PDB implementation is rather slow (~500µs). -LibError debug_resolve_symbol(void* ptr_of_interest, char* sym_name, char* file, int* line) +// actual implementation; made available so that functions already under +// the lock don't have to unlock (slow) to avoid recursive locking. +static LibError debug_resolve_symbol_lk(void* ptr_of_interest, char* sym_name, char* file, int* line) { sym_init(); const DWORD64 addr = (DWORD64)ptr_of_interest; int successes = 0; - lock(); - // get symbol name (if requested) if(sym_name) { @@ -217,10 +209,24 @@ LibError debug_resolve_symbol(void* ptr_of_interest, char* sym_name, char* file, } } - unlock(); return (successes != 0)? INFO_OK : ERR_FAIL; } +// read and return symbol information for the given address. all of the +// output parameters are optional; we pass back as much information as is +// available and desired. return 0 iff any information was successfully +// retrieved and stored. +// sym_name and file must hold at least the number of chars above; +// file is the base name only, not path (see rationale in wdbg_sym). +// the PDB implementation is rather slow (~500µs). +LibError debug_resolve_symbol(void* ptr_of_interest, char* sym_name, char* file, int* line) +{ + lock(); + LibError ret = debug_resolve_symbol_lk(ptr_of_interest, sym_name, file, line); + unlock(); + return ret; +} + //---------------------------------------------------------------------------- // stack walk @@ -319,6 +325,12 @@ static LibError ia32_walk_stack(STACKFRAME64* sf) // dump_frame_cb needs the frame pointer for reg-relative variables. typedef LibError (*StackFrameCallback)(const STACKFRAME64*, void*); +static void skip_this_frame(uint& skip, void* context) +{ + if(!context) + skip++; +} + // iterate over a call stack, calling back for each frame encountered. // if != 0, we start there; otherwise, at the current context. // return an error if callback never succeeded (returned 0). @@ -338,7 +350,7 @@ static LibError walk_stack(StackFrameCallback cb, void* user_arg = 0, uint skip // .. need to determine context ourselves. else { - skip++; // skip this frame + skip_this_frame(skip, (void*)pcontext); // there are 4 ways to do so, in order of preference: // - asm (easy to use but currently only implemented on IA32) @@ -418,7 +430,7 @@ static LibError walk_stack(StackFrameCallback cb, void* user_arg = 0, uint skip // no more frames found - abort. note: also test FP because // StackWalk64 sometimes erroneously reports success. - void* fp = (void*)(uintptr_t)sf.AddrFrame .Offset; + void* fp = (void*)(uintptr_t)sf.AddrFrame.Offset; if(err != INFO_OK || !fp) return ret; @@ -429,9 +441,12 @@ static LibError walk_stack(StackFrameCallback cb, void* user_arg = 0, uint skip } ret = cb(&sf, user_arg); + // callback is allowing us to continue + if(ret == INFO_CB_CONTINUE) + ret = INFO_OK; // make sure this is never returned // callback reports it's done; stop calling it and return that value. // (can be either success or failure) - if(ret != INFO_CB_CONTINUE) + else { debug_assert(ret <= 0); // shouldn't return > 0 return ret; @@ -465,12 +480,10 @@ static LibError nth_caller_cb(const STACKFRAME64* sf, void* user_arg) // this is fast enough to allow that. void* debug_get_nth_caller(uint skip, void* pcontext) { - if(!pcontext) - skip++; // skip this frame - lock(); void* func; + skip_this_frame(skip, pcontext); LibError err = walk_stack(nth_caller_cb, &func, skip, (const CONTEXT*)pcontext); unlock(); @@ -1196,9 +1209,7 @@ static LibError dump_sym_data(DWORD id, const u8* p, DumpState state) DWORD type_id; if(!SymGetTypeInfo(hProcess, mod_base, id, TI_GET_TYPEID, &type_id)) WARN_RETURN(ERR_SYM_TYPE_INFO_UNAVAILABLE); - LibError ret = determine_symbol_address(id, type_id, &p); - if(ret != 0) - return ret; + RETURN_ERR(determine_symbol_address(id, type_id, &p)); // display value recursively return dump_sym(type_id, p, state); @@ -1285,12 +1296,8 @@ static LibError dump_sym_function_type(DWORD UNUSED(type_id), const u8* p, DumpS // unfortunately the one thing we care about, its name, // isn't exposed via TI_GET_SYMNAME, so we resolve it ourselves. - unlock(); // prevent recursive lock - char name[DBG_SYMBOL_LEN]; - LibError err = debug_resolve_symbol((void*)p, name, 0, 0); - - lock(); + LibError err = debug_resolve_symbol_lk((void*)p, name, 0, 0); out(L"0x%p", p); if(err == INFO_OK) @@ -1826,10 +1833,11 @@ static LibError dump_frame_cb(const STACKFRAME64* sf, void* UNUSED(user_arg)) void* func = (void*)sf->AddrPC.Offset; char func_name[DBG_SYMBOL_LEN]; char file[DBG_FILE_LEN]; int line; - if(debug_resolve_symbol(func, func_name, file, &line) == 0) + LibError ret = debug_resolve_symbol_lk(func, func_name, file, &line); + if(ret == INFO_OK) { // don't trace back further than the app's entry point - // (noone wants to see this frame). checking for the + // (no one wants to see this frame). checking for the // function name isn't future-proof, but not stopping is no big deal. // an alternative would be to check if module=kernel32, but // that would cut off callbacks as well. @@ -1858,31 +1866,23 @@ static LibError dump_frame_cb(const STACKFRAME64* sf, void* UNUSED(user_arg)) } -// write a complete stack trace (including values of local variables) into -// the specified buffer. if is nonzero, it is assumed to be a -// platform-specific representation of execution state (e.g. Win32 CONTEXT) -// and tracing starts there; this is useful for exceptions. -// otherwise, tracing starts at the current stack position, and the given -// number of stack frames (i.e. functions) above the caller are skipped. -// this prevents functions like debug_assert_failed from -// cluttering up the trace. returns the buffer for convenience. -const wchar_t* debug_dump_stack(wchar_t* buf, size_t max_chars, uint skip, void* pcontext) +LibError debug_dump_stack(wchar_t* buf, size_t max_chars, uint skip, void* pcontext) { - if(!pcontext) - skip++; // skip this frame - + static uintptr_t already_in_progress; + if(!CAS(&already_in_progress, 0, 1)) + return ERR_REENTERED; // NOWARN lock(); out_init(buf, max_chars); ptr_reset_visited(); - LibError err = walk_stack(dump_frame_cb, 0, skip, (const CONTEXT*)pcontext); - if(err < 0) - out(L"(error while building stack trace: %d)", err); + skip_this_frame(skip, pcontext); + LibError ret = walk_stack(dump_frame_cb, 0, skip, (const CONTEXT*)pcontext); unlock(); + already_in_progress = 0; - return buf; + return ret; }