Skip to content

Stacktraces with CRTDBG memory leaks on Windows - #3202

Merged
carlosmn merged 2 commits into
libgit2:masterfrom
jeffhostetler:windows_stack_trace
Jul 6, 2015
Merged

Stacktraces with CRTDBG memory leaks on Windows#3202
carlosmn merged 2 commits into
libgit2:masterfrom
jeffhostetler:windows_stack_trace

Conversation

@jeffhostetler

Copy link
Copy Markdown
Contributor

Added routines to generate a stacktrace within libgit2 on Windows using the system debugger APIs.
Updated CRTDBG memory leak reporting to include de-duped stack traces of each unique leak.

To simplify things I just combined these new features under the original MSVC_CRTDBG define.

@jeffhostetler jeffhostetler changed the title WIP Add MSVC_STACKTRACE. Stacktrace on Windows. WIP Print stacktrace for memory leaks on Windows. Jun 16, 2015
@jeffhostetler jeffhostetler changed the title WIP Print stacktrace for memory leaks on Windows. Stacktraces with CRTDBG memory leaks on Windows Jun 17, 2015
@jeffhostetler

Copy link
Copy Markdown
Contributor Author

I think this is ready for review now.

@carlosmn

Copy link
Copy Markdown
Member

This is pretty exciting. Love seeing the leak output on AppVeyor. It looks like the leaks test needs an #if defined() around it, but otherwise, we can probably just merge it.

Comment thread src/win32/w32_stack.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be a bit more idiomatic libgit2 to declare our own struct with SYMBOL_INFO as the first field and the buffer just after that, though I suppose we don't really do quite this anywhere else.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's clearer. I'll update it.

@nulltoken

Copy link
Copy Markdown
Member

@jeffhostetler 💎 😍 ‼️

@jeffhostetler

Copy link
Copy Markdown
Contributor Author

Thanks. I've got one more thing I want to try/fix with checkpointing before I wrap this up. I'm on vacation for the next few days, so I might not get back to it until then.

Also, anyone have interest in having the containing C# stack trace when called by LG2# and included in the de-dup calculation? I don't currently know how to get this natively from C, I can investigate this if there's interest while here.

@nulltoken

Copy link
Copy Markdown
Member

Also, anyone have interest in having the containing C# stack trace when called by LG2# and included in the de-dup calculation? I don't currently know how to get this natively from C, I can investigate this if there's interest while here.

I do. I'm not sure we could run this as part of the CI build, though. However, I'd happily volunteer to write a .ps1 script that would clone libgit2, build it with the special flags and run the libgit2sharp against it. This script may be run locally for now.

@ethomson @jamill Thoughts?

@jeffhostetler

Copy link
Copy Markdown
Contributor Author

I've updated this to include routines to do check points. You might use this around an individual test, for example. I've a commented out example in clar_libgit2_trace.c showing it used around a "suite". It does suffer from a few false positives because of TLS data that isn't freed until the thread/process finishes, but it might be helpful in some instances.

Also, I've included a set of "aux" routines to let a consumer (like LibGit2Sharp) register callbacks and give us the C# stack trace of allocs / leaks (since my C code doesn't cross the PInvoke boundary). This allows the C# data to be independently de-duped in C#. It makes no assumptions about C#, so it should be usable by other interpreted languages.

I'll push up a PR over in LG2# in a minute that uses this.

@jeffhostetler

Copy link
Copy Markdown
Contributor Author

I just rebased this onto the current master. It now shows only 3 leaks remaining when running the clar tests.

carlosmn added a commit that referenced this pull request Jul 6, 2015
Stacktraces with CRTDBG memory leaks on Windows
@carlosmn
carlosmn merged commit 3c83111 into libgit2:master Jul 6, 2015
@jeffhostetler
jeffhostetler deleted the windows_stack_trace branch July 9, 2015 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants