Yea, sorry, some of my local changes were mixed in there. But the original
code that you posted above still has the same issue.
On Mon, Nov 14, 2016 at 1:52 PM Jason Molenda <jmole...@apple.com> wrote:
> 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 <ztur...@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 <jing...@apple.com> wrote:
> >
> > > On Nov 13, 2016, at 4:48 PM, Zachary Turner via lldb-dev <
> lldb-dev@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@lists.llvm.org
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> >
>
>
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
