jrfonseca / drmingw

Postmortem debugging tools for MinGW.
GNU Lesser General Public License v2.1
279 stars 56 forks source link

Stack overflow #7

Closed CyberShadow closed 9 years ago

CyberShadow commented 9 years ago

After integrating Dr. MinGW with Very Sleepy, a problem quickly surfaced - mgwhelp.dll is crashing with a stack overflow on this program: https://github.com/VerySleepy/verysleepy/issues/9#issuecomment-76184001

Here's a partial stack trace:

msvcrt_malloc
allocate_ts_entry
tsearch_inner
_dwarf_tsearch
_dwarf_get_alloc
dwarf_siblingof_b
dwarf_siblingof
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
search_func
jrfonseca commented 9 years ago

The stack overflow happens on downward-debug.exe offset 0x218b7, but using DrMingw's addr2line.exe like

addr2line.exe downward-debug.exe 0x218b7        

works fine. In other words, the bug only happens when mgwhelp.dll is used inside sleepy.exe.

It turns out (looking at the output of dumpbin) that sleepy.exe only has 1 MB stack. And if I modify sleepy.exe to have a bigger stack, using editbin like

editbin /stack:8388608 sleepy.exe

VerySleepy also works fine.

Maybe mgwhelp.dll implementation is a big stack hungry. After all search_func was based from elftoolchain from Linux, and Linux has a 8MB stack by default unlike the Windows 1MB default. Not sure how easy to improve this.

On the other hand, trying to make do with 1MB stack, on a 64-bit process, and when computers come with 4GB/8GB standard, is a tad cheap ;-) I'd suggest bumping the verysleep stack to something larger, as a temporary workaround if nothing else.

For reference, MinGW's executables appear to use a 2MB stack by default, which was why I didn't spot the issue with DrMingw's addr2line.exe.

jrfonseca commented 9 years ago

Not sure how easy to improve this.

Actually it's easy to fix search_func -- just replace the tail-recursion with a loop.

In fact, I suspect that on release builds of mgwhelp.dll this is not a problem, as the compiler should always do the tail-recursion. At any rate, it's probably better not to rely on the compiler for this, particularly when debugging.

CyberShadow commented 9 years ago

Oh, right. I "shipped" debug versions. My bad.

jrfonseca commented 9 years ago

One other thing from verysleepy bug report: from the screenshot, it looks like verysleepy expects mgwhelp to automatically demangle functions, but mgwhelp is not meeting that expectation. It should be trivial to fix though.

CyberShadow commented 9 years ago

Oh, right. I "shipped" debug versions.

Sorry. I didn't.These were built with:

cmake -Wno-dev -G "MinGW Makefiles" -H. -Bbuild
cmake --build build
CyberShadow commented 9 years ago

Is the default target is a debug build? Do I need to specify something else to make a release build?

jrfonseca commented 9 years ago

No, the default should be a release build. I suspect that gcc failed to optimize the tail-recursion away.

CyberShadow commented 9 years ago

I've increased the stack size to 8MB for now. I agree it's not much on modern systems.

One other thing from verysleepy bug report: from the screenshot, it looks like verysleepy expects mgwhelp to automatically demangle functions, but mgwhelp is not meeting that expectation.

Hmm, but in the provided example program, I can see demangled C++ symbols, e.g.:

_M_range_insert<__gnu_cxx::__normal_iterator<const GlobalOperator**, std::vector<const GlobalOperator*> > >

Perhaps it's a matter of different mangling schemes?

jrfonseca commented 9 years ago

I've increased the stack size to 8MB for now. I agree it's not much on modern systems.

I've fixed the search_func's hunger for stack space FWIW. So you can revert this.

Hmm, but in the provided example program, I can see demangled C++ symbols [...]

Indeed.

Perhaps it's a matter of different mangling schemes?

Yes, that's possible.