[lldb-dev] Bug in StackFrame::UpdateCurrentFrameFromPreviousFrame

[Jason Molenda via lldb-dev]

For reference, the original code that Greg wrote in r112301 was

+    if (!m_disassembly.GetString().empty())
+        m_disassembly.GetString().swap (m_disassembly.GetString());




> On Nov 14, 2016, at 1:44 PM, Zachary Turner <zturner at google.com> wrote:
> 
> If the swap is correct, then wouldn't we also need to swap the variable list?
> 
> On Mon, Nov 14, 2016 at 10:58 AM Jim Ingham <jingham at apple.com> wrote:
> 
> > On Nov 13, 2016, at 4:48 PM, Zachary Turner via lldb-dev <lldb-dev at lists.llvm.org> wrote:
> >
> > I was going through doing some routine StringRef changes and I ran across this function:
> >
> >   std::lock_guard<std::recursive_mutex> guard(m_mutex);
> >   assert(GetStackID() ==
> >          prev_frame.GetStackID()); // TODO: remove this after some testing
> >   m_variable_list_sp = prev_frame.m_variable_list_sp;
> >   m_variable_list_value_objects.Swap(prev_frame.m_variable_list_value_objects);
> >   if (!m_disassembly.GetString().empty()) {
> >     m_disassembly.Clear();
> >     m_disassembly.GetString().swap(m_disassembly.GetString());
> >   }
> >
> > Either I'm crazy or that bolded line is a bug.  Is it supposed to be prev_frame.m_disassembly.GetString()?
> >
> > What would the implications of this bug be?  i.e. how can we write a test for this?
> >
> > Also, as a matter of curiosity, why is it swapping?  That means it's modifying the input frame, when it seems like it really should just be modifying the current frame.
> 
> What lldb does is store the stack frame list it calculated from a previous stop, and copy as much as is relevant into the new stack frame when it stops, which will then become the stack frame list that gets used.  So this is a transfer of information from the older stop's stack frame to the new one.  Thus the swap.
> 
> To be clear, current here means "the stack frame we are calculating from this stop" and previous here means "the stack frame from the last stop".  That's confusing because previous & next also get used for up and down the current stack frame list.  That's why I always try to use "younger" and "older" for ordering in one stack (that and it makes the ordering unambiguous.)
> 
> So while this is definitely a bug, this is just going to keep the frames in the newly calculated stack frame list from taking advantage of any disassembly that was done on frames from the previous stop.  Since this will get created on demand if left empty, it should have no behavioral effect.  To test this you would have to count the number of times you disassembled the code for a given frame.  If this were working properly, you'd only do it once for the time that frame lived on the stack.  With this bug you will do it every time you stop and ask for disassembly for this frame.
> 
> Jim
> 
> 
> > _______________________________________________
> > lldb-dev mailing list
> > lldb-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>