Bug in StackFrame::UpdateCurrentFrameFromPreviousFrame

I was going through doing some routine StringRef changes and I ran across this function:

std::lock_guardstd::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.

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

Looks incorrect to me. It was introduced with this change. Adding Greg.

Author: Greg Clayton <gclayton@apple.com>

    Made it so we update the current frames from the previous frames by doing STL
    swaps on the variable list, value object list, and disassembly. This avoids
    us having to try and update frame indexes and other things that were getting
    out of sync.
    
    git-svn-id: https://llvm.org/svn/llvm-project/llvdb/trunk@112301 91177308-0d34-0410-b5e6-96231b3b80d8

If the swap is correct, then wouldn’t we also need to swap the variable list?

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

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

Yea, sorry, some of my local changes were mixed in there. But the original code that you posted above still has the same issue.

If the swap is correct, then wouldn't we also need to swap the variable list?

That would make things more symmetrical, though all your doing is skipping the shared pointer ref count manipulations so it isn't terribly important.

Jim