This is an archive of the discontinued LLVM Phabricator instance.

[dsymutil] Remove penultimate recursive call in lookForChildDIEsToKeep (NFC)
ClosedPublic

Authored by JDevlieghere on Dec 3 2019, 3:31 PM.

Details

Summary

The functions lookForDIEsToKeep and keepDIEAndDependencies are mutually
recursive and can cause a stackoverflow for large projects. While this has
always been the case, it became a bigger issue when we parallelized dsymutil,
because threads get only a fraction of the stack space.

In an attempt to tackle this issue, we removed part of the recursion in r338536
by introducing a worklist. Processing of child DIEs was no longer recursive.
However, we still received bug reports where we'd run out of stack space.

This patch removes another recursive call from lookForDIEsToKeep. The call was
used to look at DIEs that reference the current DIE. To make this possible, we
inlined keepDIEAndDependencies and added this work to the existing worklist.
Because the function is not tail recursive, we needed to add two more types of
worklist entries to perform the subsequent work.

Although one more recursive call remains, this was sufficient to address the
stack space issue for our current reports.

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 3 2019, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2019, 3:31 PM

The generated DWARF for ToT clang built in debug is byte-identical.

Build result: pass - 60440 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

aprantl added inline comments.Dec 3 2019, 4:17 PM
llvm/tools/dsymutil/DwarfLinker.cpp
794

///

814

///

821

switch?

829

these two lines are redundant. Are they useful for readability?

901

It's confusing that this break refers to a different block than the continue above :-)
no need to change anything

964

SmallVector now?

JDevlieghere marked 6 inline comments as done.

Feedback Adrian

aprantl accepted this revision.Dec 4 2019, 9:31 AM
This revision is now accepted and ready to land.Dec 4 2019, 9:31 AM

I really can't guarantee that I found no logic errors, but from a high-level point of view this seems logical.

This revision was automatically updated to reflect the committed changes.