Page MenuHomePhabricator

Don't use an atexit handler for cleaning up process specific temp dir
ClosedPublic

Authored by zturner on Feb 18 2016, 3:13 PM.

Details

Summary

For seemingly no reason today, I started getting crashes on exit in CleanupProcessSpecificTempDirectory when I ran certain tests. I determined this wasn't related to a code change, and when I looked in my temp lldb directory, I had close to 1500 process specific temp directories that had never been cleaned up, some dating back over 1 year ago.

Best I can figure out is that in the case of running the test suite, we've installed an atexit handler for the python executable itself. Which means it will call this atexit handler after it has already (presumably) unloaded our extension module, leading to undefined behavior.

Whatever the cause, atexit handlers are dangerous because of the restrictions on what kind of code you can run in them. I moved this to an explicit cleanup on Terminate, and verified that there are no more crashes on exit, and additionally the process specific temp directory is now correctly cleaned up after every test.

I don't know how (or even if) this was ever working on other platforms besides Windows, but you guys should check your temp/lldb directory and see if there are thousands of directories in there.

Diff Detail

Event Timeline

zturner updated this revision to Diff 48410.Feb 18 2016, 3:13 PM
zturner retitled this revision from to Don't use an atexit handler for cleaning up process specific temp dir.
zturner updated this object.
zturner added reviewers: clayborg, labath, amccarth.
zturner added a subscriber: lldb-commits.
amccarth added inline comments.Feb 18 2016, 3:28 PM
source/Host/common/HostInfoBase.cpp
88

There's a difference in behavior in that this now happens even if we haven't (yet) created a temp directory. As a side effect, CleanupProcessSpecificLLDBTempDir will first _create_ the temp directory and then immediately delete it, which seems wasteful.

zturner updated this revision to Diff 48414.Feb 18 2016, 3:35 PM

This should make sure it happens if and only if the directory was successfully created earlier during program execution.

zturner marked an inline comment as done.Feb 18 2016, 3:35 PM
amccarth accepted this revision.Feb 18 2016, 3:36 PM
amccarth edited edge metadata.
This revision is now accepted and ready to land.Feb 18 2016, 3:36 PM
clayborg accepted this revision.Feb 18 2016, 5:22 PM
clayborg edited edge metadata.

The HostInfoBase::Initialize() and HostInfoBase::Terminate() might be good places for your PDB calls like ::CoUninitialize(); to be placed...

labath accepted this revision.Feb 19 2016, 2:21 AM
labath edited edge metadata.

So, linux actually manages to invoke the atexit handlers (through some deep magic, no doubt) upon shared library unload, so we are ok here (plus linux likes to nuke /tmp after every reboot). LGTM, with a small RAII request.

source/Host/common/HostInfoBase.cpp
86

I think we should move this into the destructor of g_fields. Then you can simply do delete g_fields (or even g_fields.reset()) here.

This revision was automatically updated to reflect the committed changes.