Don't consider frames with different PCs as a loop

[tatyana-krasnukha]

Summary

A compiler can produce FDE records with no CFA offset if it doesn't save anything on stack for a frame. This is what the MetaWare compiler does for some functions - it doesn't update CFA since it saves return address to a register.
In this case LLDB fails to unwind the stack complaining about a loop. This patch checks also the return addresses of the frames, there is no loop if they differ.

This is also consistent with how UnwindLLDB::GetOneMoreFrame identifies infinite loops.

[DavidSpickett]

Looking at GetOneMoreFrame I agree the logic matches up but don't understand the logic enough overall to approve. I'll leave that to others.

Also the early returns are a welcome improvement!

lldb/source/Target/RegisterContextUnwind.cpp

697

specail -> special
hanlders -> handlers
compiance -> compliance
more then 2 -> more than 2

[labath]

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.

lldb/source/Target/RegisterContextUnwind.cpp

UnwindLogMsg("initialized frame current pc is 0x%" PRIx64
                (uint64_t)m_afa);
 }
 
 bool RegisterContextUnwind::CheckIfLoopingStack() {
   // If we have a bad stack setup, we can get the same CFA value multiple times
   // -- or even more devious, we can actually oscillate between two CFA values.
   // Detect that here and break out to avoid a possible infinite loop in lldb
   // trying to unwind the stack. To detect when we have the same CFA value
-  // multiple times, we compare the
-  // CFA of the current
-  // frame with the 2nd next frame because in some specail case (e.g. signal
-  // hanlders, hand written assembly without ABI compliance) we can have 2
-  // frames with the same
-  // CFA (in theory we
-  // can have arbitrary number of frames with the same CFA, but more then 2 is
-  // very very unlikely)
+  // multiple times, we compare the CFA of the current frame with the 2nd next
+  // frame because in some specail case (e.g. signal hanlders, hand written
+  // assembly without ABI compiance) we can have 2 frames with the same CFA (in
+  // theory we can have arbitrary number of frames with the same CFA, but more
+  // then 2 is very very unlikely).
 
   RegisterContextUnwind::SharedPtr next_frame = GetNextFrame();
-  if (next_frame) {
-    RegisterContextUnwind::SharedPtr next_next_frame =
-        next_frame->GetNextFrame();
-    addr_t next_next_frame_cfa = LLDB_INVALID_ADDRESS;
-    if (next_next_frame && next_next_frame->GetCFA(next_next_frame_cfa)) {
-      if (next_next_frame_cfa == m_cfa) {
-        // We have a loop in the stack unwind
-        return true;
-      }
-    }
-  }
+  if (!next_frame)
     return false;
+
+  RegisterContextUnwind::SharedPtr next_next_frame = next_frame->GetNextFrame();
+  if (!next_next_frame)
+    return false;
+
+  // Since the return address value can be restored not only from the stack but
+  // also from a register, don't consider frames with the same CFAs and different
+  // PCs as a loop.
+  if (m_current_pc.IsValid() && (m_current_pc != next_next_frame->GetPC()))
+    return false;
+
+  addr_t next_next_frame_cfa = LLDB_INVALID_ADDRESS;
+  return next_next_frame->GetCFA(next_next_frame_cfa) &&
+         next_next_frame_cfa == m_cfa;
 }
 
 bool RegisterContextUnwind::IsFrameZero() const { return m_frame_number == 0; }
 
 bool RegisterContextUnwind::BehavesLikeZerothFrame() const {
   if (m_frame_number == 0)
     return true;
   if (m_behaves_like_zeroth_frame)