Page MenuHomePhabricator

Change the looping stack detection code

Authored by tberghammer on Sep 8 2015, 10:07 AM.



Change the looping stack detection code

In some special case (e.g. signal handlers, hand written assembly) it is
valid to have 2 stack frame with the same CFA value. This CL change the
looping stack detection code to report a loop only if at least 3
consecutive frames have the same CFA.

Note: I would prefer to get rid of this looping stack detection because the implementation is still a bit flaky and rely on the logic in UnwindLLDB::GetOneMoreFrame to stop the infinite loop in case of a looping stack, but I don't know how big the user impact will be in the cases where the unwind go wrong and start looping

Diff Detail


Event Timeline

tberghammer retitled this revision from to Change the looping stack detection code.
tberghammer updated this object.
tberghammer added a reviewer: jasonmolenda.
tberghammer added a subscriber: lldb-commits.
jasonmolenda accepted this revision.Sep 8 2015, 3:41 PM
jasonmolenda edited edge metadata.

I'm fine with this change. On x86, where the CALL instruction pushes the return address on the stack, you can't have two stack frames with the CFA. If we have a loop, I don't think it's a big problem if we only break out once it's a 3-stack-frame loop.

I'm not sure it's possible to have an arbitrary number of stack frames with the same CFA - the return addresses have to be saved somewhere. The register file only gives us so many volatile registers we can use to save multiple stack frame's return addresses before we need to write something to the stack.

This revision is now accepted and ready to land.Sep 8 2015, 3:41 PM

In ABI conforming code we will never have even 2 consecutive frames with the same CFA for all ABI I know about.
In practice, 2 consecutive frame with the same CFA can happen (for hand written assembly and signal handling), but more then 2 is highly unlikely.
In theory, arbitrary number of frames can have the same CFA if the code sets up a different stack for itself (in a different location) what we don't track, but it is just a hypothetical case.

This revision was automatically updated to reflect the committed changes.