Fix for PrintStackTraces

[ravitheja]

Summary

The issue arises due to the wrong unwinder used for the first
stack frame, where the default unwinder returns erroneous frame
whereas the fallback would have given the correct frame had it
been used.
The following fix consists of two parts ->

1. The first part changes the unwinding strategy, earlier the default unwinder was used to get 2 more stack frames and if it failed a fallback unwinder was used. Now we try to obtain as many frames (max 10) as we can get from default unwinder and also fallback unwinder and use the one that gives more number of frames.
2. Normally unwindplans are assosciated with functions and the row to be used is obtained from the offset (obtained from the low_pc of the function symbol). Sometimes it may occur that the unwindplan is assosciated to the complete Elf section in which case the offset calculation would be wrong as the debugger uses the same offset originally obtained from the function symbol. Hence this offset is recalculated.

[labath]

So, I'll let Jason be the definitive judge here, but I have to say I don't like this solution.

Computing 10 frames significantly increases the amount of processing needed to get some result. Most importantly, it increases the latency, as it will cause a lot more data to be pulled from the server in case of remote debugging and may force a lot more eh_frame parsing. Also, it makes the unwinder a lot more nondeterministic and harder to test as now the frame you get shown can depend on something happening 10 levels down.

I'd really prefer a solution where we can get default unwinder to do the right thing, so we could just avoid needing to rely on the fallback.

[jingham]

I agree with Pavel. Remember that most stops in the course of stepping will require an unwind (to tell step-in vrs. lateral step vrs. step out). This can happen many times in the course of a "user level" step. So for stepping, particularly on slow devices, unwinding is a bit part of the performance. We don't want to do any more unwinding than we have to.

[ravitheja]

Well its not possible for the assembly unwinder to do the correct thing in this situation , as the function is entered through a stub function (which has push instruction) and then a jump. The point of this patch is to catch when to use the CFI or the assembly unwinder.

[ravitheja]

Making EH Frame CFI the full unwinder
when artificial symbols are found.

[ravitheja]

Checks for nullptr

[labath]

I think this idea in general is more acceptable, but we'll need an OK from Jason on the details.

Also, we should figure out a way to add a test for this. We should definitely add a more deterministic test and not rely on TestPrintStackTraces to check this functionality. Is it possible to write a self-contained test replicating these conditions, i.e. without depending on unwinding from standard library functions?

In D21221#455818, @ravitheja wrote:

Well its not possible for the assembly unwinder to do the correct thing in this situation , as the function is entered through a stub function (which has push instruction) and then a jump. The point of this patch is to catch when to use the CFI or the assembly unwinder.

Note I said the "default" unwinder. If the assembly unwinder does not work, then maybe it should not be the default in this case.
Also, I seem to recall that the first plan being tried is the "augmented CFI", but that is very conservative in what it accepts as valid input. So maybe if we made it smarter (as in, recognize that in this case it does not need to do any augmentation and just pass through the eh_frame plan), then this and a lot of other problems would go away.

source/Plugins/Process/Utility/RegisterContextLLDB.cpp

1612

What is this supposed to do?

[jasonmolenda]

Hi Ravi, sorry for not having time to look at this patch yet - I was out of the office Friday and was catching up on everything today. I'll look at this tomorrow. My initial reactions are to be a little worried. It sounds like you have functions that are non-ABI conforming -- which is fine for functions that don't have external linkage -- and you're tweaking the unwinder until it works for this case. When we were discussing this on lldb-dev a couple of weeks ago, the functions in question had no eh_frame code. Why are you forcing eh_frame for frame 0? Is this a different case than the one you were debugging ten days ago? One initial comment is I don't know about adding AlwaysRelyOnEHUnwindInfo to the DynamicLoader - the section of code you added this check to in RegisterContextLLDB is there because we had one or two solibs on darwin where we had to use eh_frame, hence it made sense to ask the DynamicLoader plugin. If we were to add something like AlwaysRelyOnEHUnwindInfo for an entire platform, I'd probably put it at FuncUnwinders::GetUnwindPlanAtNonCallSite. But I really don't think that's a good idea unless there's a concrete example where a non-ABI-conforming function actually has eh_frame that describes how to correctly unwind from it.

(my initial opinion is that a debugger trying to unwind out of non-ABI conforming functions without hand-written eh_frame describing how to do it, are in the "it would be nice if it worked" camp; I'd be reluctant to make changes that could compromise lldb otherwise to support this kind of scenario.)

But I really haven't given the patch or the discussion a fair read-through and thought about it more - I'll do that tomorrow.

[ravitheja]

source/Plugins/Process/Utility/RegisterContextLLDB.cpp

1612

It stops the the TryFallbackUnwindplan to use the assembly unwinder.

[ravitheja]

so regarding this particular situation I want to give little more insight ->
It starts out from here

0x40143a <+346>: movabsq $0x403e32, %rdi           ; imm = 0x403E32 
0x401444 <+356>: movb   $0x0, %al
0x401446 <+358>: callq  0x400d30                  ; symbol stub for: printf
0x40144b <+363>: movq   0x6071c0, %rdi
0x401453 <+371>: movl   %eax, -0xdc(%rbp)
0x401459 <+377>: callq  0x400ed0                  ; symbol stub for: fflush  (jump to next disassembly block 1)
0x40145e <+382>: movl   $0x40, %esi
0x401463 <+387>: leaq   -0xb0(%rbp), %rdi
0x40146a <+394>: movq   0x607158, %rdx
0x401472 <+402>: movl   %eax, -0xe0(%rbp)

(lldb) disassemble (disassembly block1)
a.out`fflush:

0x400ed0 <+0>:  jmpq   *0x206212(%rip)           ; _GLOBAL_OFFSET_TABLE_ + 232
0x400ed6 <+6>:  pushq  $0x1a
0x400edb <+11>: jmp    0x400d20                      ;  (jump to **disassembly block 2**)

(lldb) disassemble (disassembly block 2)

0x400d20:       pushq  0x2062e2(%rip)            ; _GLOBAL_OFFSET_TABLE_ + 8
0x400d26:       jmpq   *0x2062e4(%rip)           ; _GLOBAL_OFFSET_TABLE_ + 16   I think this jump goes to  0x7ffff7df04a0

ld-linux-x86-64.so.2`___lldb_unnamed_symbol95$$ld-linux-x86-64.so.2:

0x7ffff7df04a0 <+0>:  subq   $0x38, %rsp    -> The testcase tries to unwind out of here and fails.
0x7ffff7df04a4 <+4>:  movq   %rax, (%rsp)
0x7ffff7df04a8 <+8>:  movq   %rcx, 0x8(%rsp)
0x7ffff7df04ad <+13>: movq   %rdx, 0x10(%rsp)
0x7ffff7df04b2 <+18>: movq   %rsi, 0x18(%rsp)
0x7ffff7df04b7 <+23>: movq   %rdi, 0x20(%rsp)
0x7ffff7df04bc <+28>: movq   %r8, 0x28(%rsp)
0x7ffff7df04c1 <+33>: movq   %r9, 0x30(%rsp)
0x7ffff7df04c6 <+38>: movq   0x40(%rsp), %rsi

Now as you can see, from inside fflush its not possible for the assembly unwind to figure out the situation.
@jasonmolenda The functions I posted in the lldb-dev are the same, here i am just posting how it got there.
There is eh_frame information for these functions, that is able to correctly point out the CFA.

lldb) image show-unwind --address 0x7ffff7df04a0
UNWIND PLANS for ld-linux-x86-64.so.2`___lldb_unnamed_symbol95$$ld-linux-x86-64.so.2 (start addr 0x7ffff7df04a0)

Asynchronous (not restricted to call-sites) UnwindPlan is 'assembly insn profiling'
Synchronous (restricted to call-sites) UnwindPlan is 'eh_frame CFI'

Assembly language inspection UnwindPlan:
This UnwindPlan originally sourced from assembly insn profiling
This UnwindPlan is sourced from the compiler: no.
This UnwindPlan is valid at all instruction locations: yes.
Address range of this UnwindPlan: [ld-linux-x86-64.so.2..text + 88512-0x0000000000015a30)
row[0]: 0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
row[1]: 4: CFA=rsp+64 => rsp=CFA+0 rip=[CFA-8]
row[2]: 94: CFA=rsp -8 => rsp=CFA+0 rip=[CFA-8]

eh_frame UnwindPlan:
This UnwindPlan originally sourced from eh_frame CFI
This UnwindPlan is sourced from the compiler: yes.
This UnwindPlan is valid at all instruction locations: no.
Address range of this UnwindPlan: [ld-linux-x86-64.so.2..text + 88512-0x0000000000015a21)
row[0]: 0: CFA=rsp+24 => rip=[CFA-8]
row[1]: 4: CFA=rsp+80 => rip=[CFA-8]
row[2]: 94: CFA=rsp +8 => rip=[CFA-8]

Arch default UnwindPlan:
This UnwindPlan originally sourced from x86_64 default unwind plan
This UnwindPlan is sourced from the compiler: no.
This UnwindPlan is valid at all instruction locations: no.
row[0]: 0: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]

Arch default at entry point UnwindPlan:
This UnwindPlan originally sourced from x86_64 at-func-entry default
This UnwindPlan is sourced from the compiler: no.
This UnwindPlan is valid at all instruction locations: not specified.
row[0]: 0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]

As you can see the eh_frame UnwindPlan is correct here.

[ravitheja]

@labath In order to reproduce this situation without the help of standard library, I would have to write handwritten assembly and the CFI directives for that, is that fine ?

[labath]

In D21221#457329, @ravitheja wrote:

@labath In order to reproduce this situation without the help of standard library, I would have to write handwritten assembly and the CFI directives for that, is that fine ?

Yes, I think that's fine. Obviously that will make the test x86-specific (and probably linux-specific, although it would be great if that can be avoided (*)), but at least it will be well focused, and not relying on random timings in other tests. A less preferred but-still-better-than-status-quo option would be to keep the standard library dependency but remove the timing issues (e.g., by setting the breakpoint in fflush, instruction-stepping 100 times, and making sure you unwind correctly from each place).

(*) One way to do that is to avoid running code. If you can write the test in a way that it does not need a running process then you can just check in a tiny (linux) module, load it and query some properties of the contained functions and their unwind plans...

[jasonmolenda]

Thanks Ravi, I see the problem here and I agree that lldb should use eh_frame to unwind from this function - that's the only way this is going to work. I'm surprised that there is accurate eh_frame instructions for this function, it's great to see it.

I'm wondering if we can come up with a way to detect this scenario - where we're unwinding from a function that doesn't conform to the ABI. I know it's a little special-case-y but what if we modified FuncUnwinders::GetUnwindPlanAtNonCallSite to:

Get the assembly unwind plan, eh_frame unwind plan, the ABI ArchitectureDefaultAtFunctionEntry unwind plan, and the ArchitectureDefault unwind plan.

If the eh_frame's row 0 (initial set of unwind instructions) has a pc/lr restore rule that doesn't match any of the other three unwind plans, then we assume that the eh_frame instructions have knowledge that the others do not have, and we use eh_frame for this function.

I'll write this up tomorrow and see if it works on mac (although we've almost completely stopped using eh_frame on our platform so I'm not a very good test bed at all) - I can imagine how the code would look and I think it's possible to do.

If eh_frame accurately describes the prologue instructions (I think I mentioned in the lldb-dev emails a couple weeks ago -- it is not required to describe the prologue), it the pc unwind rule will be the same as the ABI's ArchitectureDefaultAtFunctionEntry. Or it will be the same as the assembly unwind rule. If the eh_frame doesn't describe the prologue -- if the 0th row is the register state after the prologue has executed -- it will match the ABI ArchitectureDefault's unwind rule to restore the pc.

The main oddity here are architectures like arm where the restore rule for pc may be in terms of the pc, or it may be in terms of the link register ("return address register" in lldb's terminology). Different UnwindPlans may specify how to restore the pc register value, or how to restore the ra register value -- they're inconsistent -- and that needs to be taken into account.

I know this isn't the approach you took with the patch, but I think it would be an improvement to lldb (especially if systems are getting eh_frame like this) and it will solve the problem I believe. What do you think? As I said, I'm happy to write up what I'm suggesting here, I don't want to ask you to do extra work to try out this idea. I'm in and out of the office this week because of an apple conference but I'll try to get something together over the next few days.

As for writing test cases, these are tricky, and I need to write unwind test cases too, it's something I haven't had time to do yet for lldb and we really need these test cases, the unwinder is VERY hard to work on without a rich testsuite.

They end up being architecture (x86_64) and platform (mac) specific... you write a top-level driver in C which calls hand-written assembly functions (either inlined or a .s file) and the little details / ABI vary from platform to platform, I'm not sure if it's worth trying to make them portable - we can rely on buildbots to run them on the matrix of systems if nothing else. The only alternative are core files of processes in the correct stopped state - but then you have to store gigantic core files and possibly binaries along with them and I don't think we're going to head down that route any time soon.

[jasonmolenda]

Regarding the part of the patch where eh_frame is used unconditionally on Linux systems, I'm hesitant to make a change like that (beyond the notes I mentioned earlier about how I'd do it in FuncUnwinders instead of down in RegisterContextLLDB). Modern gcc's put out eh_frame with prologue and epilogue descriptions on x86_64 - but there's no guarantee that eh_frame we see on a linux or bsd system will come from a modern gcc. For instance, clang (last I checked) does not emit epilogue information in the eh_frame (as did gcc until a few years ago). gdb does pretty well relying exclusively on eh_frame, but the design of the lldb unwinder was to do better than gdb, by using multiple sources of unwind information.

Of all the unwind info sources, eh_frame is the most frustrating, IMO. It *can* be everything a debugger could ever need -- but it isn't guaranteed to be that. It's only guaranteed to be enough for exception handling, which is a very different set of requirements from what a debugger needs. And there's no reliable way for a debugger to tell how descriptive an eh_frame entry is.

[ravitheja]

@jasonmolenda The approach suggested seems promising, I look forward to it, if u want I can test it at my end ? once I see it. But I must say this patch also fixes the row offset calculation, so the unwindplan specified in the eh_frame plan is specified for the complete PLT section and not w.r.t the function symbol, so the offset used for picking the row from the unwindplan should be calculated taking into consideration this point.

[clayborg]

Jason Molenda should be able to adequately review any patches. If he is OK, then I am OK.

[ravitheja]

Adding testcase

[ravitheja]

Renaming testcase

[ravitheja]

Removing other files.

[jasonmolenda]

Ravitheja and I had some discussions over email about a possible alternate approach to this issue - I've committed that approach as r274700 after testing help from Ravi. This patchset is tracking the very nice test case that was written to replicate the problem, thanks for doing that Ravitheja!

source/Plugins/Process/Utility/RegisterContextLLDB.cpp

if (m_full_unwind_plan_sp.get() == m_fallback_unwind_plan_sp.get()
         || m_full_unwind_plan_sp->GetSourceName() == m_fallback_unwind_plan_sp->GetSourceName())
     {
         return false;
     }
 
     // If a compiler generated unwind plan failed, trying the arch default unwindplan
     // isn't going to do any better.
     if (m_full_unwind_plan_sp->GetSourcedFromCompiler() == eLazyBoolYes)
         return false;
 
 
     // Get the caller's pc value and our own CFA value.
     // Swap in the fallback unwind plan, re-fetch the caller's pc value and CFA value.
     // If they're the same, then the fallback unwind plan provides no benefit.
 
     RegisterNumber pc_regnum (m_thread, eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC);
 
RegisterContextLLDB::TryFallbackUnwindPlan ()
     // We'll also need to save & restore the value of the m_cfa ivar.  Save is down below a bit in 'old_cfa'.
     UnwindPlanSP original_full_unwind_plan_sp = m_full_unwind_plan_sp;
     addr_t old_cfa = m_cfa;
 
     m_registers.clear();
 
     m_full_unwind_plan_sp = m_fallback_unwind_plan_sp;
 
-    UnwindPlan::RowSP active_row = m_fallback_unwind_plan_sp->GetRowForFunctionOffset (m_current_offset);
+    // The current offset should be recalculated here. The m_current_offset is
+    // calculated from the base address of the symbol. The symbol can lie in the PLT
+    // (Procedure Linkage Table) i.e its a symbol stub for external call. In this case
+    // the base address for the unwindplan and the base address of the symbol maybe different, hence
+    // the m_current_offset will be wrong.
+
+    int fallback_offset = m_current_offset;
+    AddressRange fall_back_unwind_address_range = m_fallback_unwind_plan_sp->GetAddressRange();
+    if (fall_back_unwind_address_range.ContainsFileAddress(m_current_pc))
+        fallback_offset = m_current_pc.GetOffset() - fall_back_unwind_address_range.GetBaseAddress().GetOffset();
+    else
+    {
+        UnwindLogMsg ("fallback unwindplan not valid for current pc");
+        m_full_unwind_plan_sp = original_full_unwind_plan_sp;
+        return false;
+    }
+
+    UnwindPlan::RowSP active_row = m_fallback_unwind_plan_sp->GetRowForFunctionOffset (fallback_offset);
     
     if (active_row && active_row->GetCFAValue().GetValueType() != UnwindPlan::Row::CFAValue::unspecified)
     {
         addr_t new_cfa;
         if (!ReadCFAValueForRow (m_fallback_unwind_plan_sp->GetRegisterKind(), active_row, new_cfa)
             || new_cfa == 0 || new_cfa == 1 || new_cfa == LLDB_INVALID_ADDRESS)
         {
             UnwindLogMsg ("failed to get cfa with fallback unwindplan");

source/Plugins/Process/Utility/UnwindLLDB.h

private:
     //-----------------------------------------------------------------
     void
     UpdateUnwindPlanForFirstFrameIfInvalid (ABI* abi);
 
     CursorSP
     GetOneMoreFrame (ABI* abi);
 
     bool
-    AddOneMoreFrame (ABI *abi);
+    AddMoreFrames (ABI *abi);
 
     bool
     AddFirstFrame ();
 
     //------------------------------------------------------------------
     // For UnwindLLDB only
     //------------------------------------------------------------------
     DISALLOW_COPY_AND_ASSIGN (UnwindLLDB);
 };
 
 } // namespace lldb_private
 
 #endif // lldb_UnwindLLDB_h_

source/Plugins/Process/Utility/UnwindLLDB.cpp

#define FRAME_COUNT 10000
         TimeValue time_value (TimeValue::Now());
 #endif
         if (!AddFirstFrame ())
             return 0;
 
         ProcessSP process_sp (m_thread.GetProcess());
         ABI *abi = process_sp ? process_sp->GetABI().get() : NULL;
 
-        while (AddOneMoreFrame (abi))
+        while (AddMoreFrames (abi))
         {
 #if DEBUG_FRAME_SPEED
             if ((m_frames.size() % FRAME_COUNT) == 0)
             {
                 TimeValue now(TimeValue::Now());
                 uint64_t delta_t = now - time_value;
                 printf ("%u frames in %" PRIu64 ".%09llu ms (%g frames/sec)\n",
                         FRAME_COUNT,

 UnwindLLDB::UpdateUnwindPlanForFirstFrameIfInvalid (ABI *abi)
 {
     // This function is called for First Frame only.
     assert (m_frames.size() == 1 && "No. of cursor frames are not 1");
 
     bool old_m_unwind_complete = m_unwind_complete;
     CursorSP old_m_candidate_frame = m_candidate_frame;
 
-    // Try to unwind 2 more frames using the Unwinder. It uses Full UnwindPlan
-    // and if Full UnwindPlan fails, then uses FallBack UnwindPlan. Also
-    // update the cfa of Frame 0 (if required).
-    AddOneMoreFrame(abi);
+    // Try to unwind as many frames as we can get using the
+    // Full Unwinder and the Fall back Unwinder and pick the
+    // one that gives more frames. Also update the cfa of Frame 0
+    // with the choosen Unwinder (if required).
+    AddMoreFrames(abi);
 
     // Remove all the frames added by above function as the purpose of
     // using above function was just to check whether Unwinder of Frame 0
     // works or not.
     for(uint32_t i=1; i<m_frames.size(); i++)
         m_frames.pop_back();
 
-    // Restore status after calling AddOneMoreFrame
+    // Restore status after calling AddMoreFrames
     m_unwind_complete = old_m_unwind_complete;
     m_candidate_frame = old_m_candidate_frame;
     return;
 }
 
 
 bool
-UnwindLLDB::AddOneMoreFrame (ABI *abi)
+UnwindLLDB::AddMoreFrames (ABI *abi)
 {
     Log *log(GetLogIfAllCategoriesSet (LIBLLDB_LOG_UNWIND));
 
     // Frame zero is a little different
     if (m_frames.empty())
         return false;
 
     // If we've already gotten to the end of the stack, don't bother to try again...
     if (m_unwind_complete)
         return false;
 
-    CursorSP new_frame = m_candidate_frame;
-    if (new_frame == nullptr)
-        new_frame = GetOneMoreFrame(abi);
+    if (m_candidate_frame == nullptr)
+        m_candidate_frame = GetOneMoreFrame(abi);
 
-    if (new_frame == nullptr)
+    if (m_candidate_frame == nullptr)
     {
         if (log)
             log->Printf ("th%d Unwind of this thread is complete.", m_thread.GetIndexID());
         m_unwind_complete = true;
         return false;
     }
 
-    m_frames.push_back(new_frame);
+    int num_frames_pushed = 0;
+    const int stack_depth_limit = 10;
+    int m_frames_last_frame_index = m_frames.size() - 1;
+
+    // Here we try to get as many frames as possible (max 10) with the Full Unwinder and
+    // the Fall Back Unwinder. Which ever Unwinder gives the most frames will be used.
 
-    // If we can get one more frame further then accept that we get back a correct frame.
+    // First checking with the Full Unwinder.
+    while (m_candidate_frame && (num_frames_pushed < stack_depth_limit))
+    {
+        m_frames.push_back(m_candidate_frame);
+        num_frames_pushed++;
         m_candidate_frame = GetOneMoreFrame(abi);
-    if (m_candidate_frame)
-        return true;
+    }
 
     // We can't go further from the frame returned by GetOneMore frame. Lets try to get a
     // different frame with using the fallback unwind plan.
-    if (!m_frames[m_frames.size() - 2]->reg_ctx_lldb_sp->TryFallbackUnwindPlan())
+    if (!m_frames[m_frames_last_frame_index]->reg_ctx_lldb_sp->TryFallbackUnwindPlan())
     {
         // We don't have a valid fallback unwind plan. Accept the frame as it is. This is a
         // valid situation when we are at the bottom of the stack.
         return true;
     }
 
-    // Remove the possibly incorrect frame from the frame list and try to add a different one with
-    // the newly selected fallback unwind plan.
-    m_frames.pop_back();
+    std::vector<CursorSP> popped_frames_save(num_frames_pushed);
+
+    // Now lets try to add frames from the Fallback Unwinder. We save the frames
+    // obtained from the Full Unwinder, in case we choose to go with it.
+    popped_frames_save.insert(popped_frames_save.begin(), (m_frames.begin() + m_frames_last_frame_index + 1), m_frames.end());
+    m_frames.erase((m_frames.begin() + m_frames_last_frame_index + 1), m_frames.end());
+
+    int num_frames_pushed_fallback = 0;
     CursorSP new_frame_v2 = GetOneMoreFrame(abi);
-    if (new_frame_v2 == nullptr)
+
+    while(new_frame_v2 && (num_frames_pushed_fallback < stack_depth_limit))
+    {
+        m_frames.push_back(new_frame_v2);
+        num_frames_pushed_fallback++;
+        new_frame_v2 = GetOneMoreFrame(abi);
+    }
+
+    if (num_frames_pushed_fallback < num_frames_pushed)
     {
-        // We haven't got a new frame from the fallback unwind plan. Accept the frame from the
-        // original unwind plan. This is a valid situation when we are at the bottom of the stack.
-        m_frames.push_back(new_frame);
+        m_frames.erase( (m_frames.begin() + m_frames_last_frame_index + 1), m_frames.end());
+        m_frames.insert( (m_frames.begin() + m_frames_last_frame_index + 1), popped_frames_save.begin(), popped_frames_save.end());
         return true;
     }
 
-    // Push the new frame to the list and try to continue from this frame. If we can get a new frame
-    // then accept it as the correct one.
-    m_frames.push_back(new_frame_v2);
-    m_candidate_frame = GetOneMoreFrame(abi);
-    if (m_candidate_frame)
+    if (!m_frames[m_frames_last_frame_index]->reg_ctx_lldb_sp->GetCFA(m_frames[m_frames_last_frame_index]->cfa))
     {
-        // If control reached here then TryFallbackUnwindPlan had succeeded for Cursor::m_frames[m_frames.size() - 2].
-        // It also succeeded to Unwind next 2 frames i.e. m_frames[m_frames.size() - 1] and a frame after that.
-        // For Cursor::m_frames[m_frames.size() - 2], reg_ctx_lldb_sp field was already updated during TryFallbackUnwindPlan
-        // call above. However, cfa field still needs to be updated. Hence updating it here and then returning.
-        if ( !(m_frames[m_frames.size() - 2]->reg_ctx_lldb_sp->GetCFA(m_frames[m_frames.size() - 2]->cfa)))
-            return false;
-        return true;
+        m_unwind_complete = true;
     }
 
-    // The new frame hasn't helped in unwinding. Fall back to the original one as the default unwind
-    // plan is usually more reliable then the fallback one.
-    m_frames.pop_back();
-    m_frames.push_back(new_frame);
     return true;
 }
 
 bool
 UnwindLLDB::DoGetFrameInfoAtIndex (uint32_t idx, addr_t& cfa, addr_t& pc)
 {
     if (m_frames.size() == 0)
     {
         if (!AddFirstFrame())
             return false;
     }
 
     ProcessSP process_sp (m_thread.GetProcess());
     ABI *abi = process_sp ? process_sp->GetABI().get() : NULL;
 
-    while (idx >= m_frames.size() && AddOneMoreFrame (abi))
-        ;
+    if (idx >= m_frames.size())
+        AddMoreFrames (abi);
 
     if (idx < m_frames.size ())
     {
         cfa = m_frames[idx]->cfa;
         pc = m_frames[idx]->start_pc;
         return true;
     }
     return false;
UnwindLLDB::DoCreateRegisterContextForFrame (StackFrame *frame)
     {
         if (!AddFirstFrame())
             return reg_ctx_sp;
     }
 
     ProcessSP process_sp (m_thread.GetProcess());
     ABI *abi = process_sp ? process_sp->GetABI().get() : NULL;
 
-    while (idx >= m_frames.size())
-    {
-        if (!AddOneMoreFrame (abi))
-            break;
-    }
+    if (idx >= m_frames.size())
+        AddMoreFrames (abi);
 
     const uint32_t num_frames = m_frames.size();
     if (idx < num_frames)
     {
         Cursor *frame_cursor = m_frames[idx].get();
         reg_ctx_sp = frame_cursor->reg_ctx_lldb_sp;
     }
     return reg_ctx_sp;