[Lldb-commits] [PATCH] D114861: Don't consider frames with different PCs as a loop

Mon Dec 6 01:46:12 PST 2021

labath added a comment.

It would be better if Jason reviewed this, but my first thoughts after seeing this are:

If we're going to be using both the PC and the CFA for frame comparison, do we really need to also skip a frame when doing the comparing? It seems like it should be sufficient to compare the `(cfa, pc)` pair of /this/ frame with the /next/ one.

If they are the same, then we're definitely looping, as the unwind on the next will proceed in exactly the same way. And if they're different then we're most likely making some kind of progress (as in, one can still create artificial examples where the stack alternates between two PCs, but that is also true for the other implementation).

I am also a bit worried about these frames with no CFA. I fear that a lot of other things could break when it is absent, it is sort of expected that every frame has one. Upon re-reading the dwarf spec, I couldn't find a place where it would explicitly say it's mandatory (which isn't completely surprising as dwarf rarely makes anything mandatory), but I definitely got the feeling that is what the authors intended. I don't know if it's possible to change the compiler, but it doesn't seem like putting an extra `cfa = sp` line into the CIE would waste too much space.

It would also be nice to have a test for funky functions like this. It should be possible to create something similar to the existing tests in `test/Shell/Unwind`. The form still leaves a lot to be desired, but it is the best we have right now.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114861/new/

https://reviews.llvm.org/D114861