Add doxygen comments to the StackFrameList API (NFC)

[vsk]

Summary

Clarify how StackFrameList works by documenting its methods. Also,
delete some dead code and insert some TODOs.

[labath]

I am not too familiar with this code, but the descriptions seem to make sense.

However, since you have kind of opened up the Optional discussion, I'll use this opportunity to give my take on it:

I've wanted to use Optional<IntType> in this way in the past, but the thing that has always stopped me is that this essentially doubles the amount of storage required for the value (you only need one bit for the IsValid field, but due to alignment constraints, this will usually consume the same amount of space as the underlying int type). This may not matter for the StackFrameList class, but it does matter for classes which have a lot of instances floating around. I suspect this is also why LLVM does not generally use this idiom (the last example where I ran into this is the VersionTuple class, which hand-rolls a packed optional by stealing a bit from the int type, and then converts this to Optional<unsigned> for the public accessors).

For this reason, I think it would be useful to have a specialized class (OptionalInt?), which has the same interface as Optional, but does not increase the storage size. It could be implemented the same way as we hand-roll the invalid values, only now the invalid values would be a part of the type. So, you could do something like:

using tid_t = OptionalInt<uint64_t, LLDB_INVALID_THREAD_ID>;

tid_t my_tid; // initialized to "invalid";
my_tid = get_thread_id();
if (my_tid) // no need to remember what is the correct "invalid" value here
  do_stuff(*my_tid);
else
  printf("no thread");

I am sure this is more than what you wanted to hear in this patch, but I wanted to make sure consider the tradeoffs before we start putting Optionals everywhere.

[vsk]

In D50087#1183982, @labath wrote:

I am not too familiar with this code, but the descriptions seem to make sense.

However, since you have kind of opened up the Optional discussion, I'll use this opportunity to give my take on it:

I've wanted to use Optional<IntType> in this way in the past, but the thing that has always stopped me is that this essentially doubles the amount of storage required for the value (you only need one bit for the IsValid field, but due to alignment constraints, this will usually consume the same amount of space as the underlying int type). This may not matter for the StackFrameList class, but it does matter for classes which have a lot of instances floating around. I suspect this is also why LLVM does not generally use this idiom (the last example where I ran into this is the VersionTuple class, which hand-rolls a packed optional by stealing a bit from the int type, and then converts this to Optional<unsigned> for the public accessors).

For this reason, I think it would be useful to have a specialized class (OptionalInt?), which has the same interface as Optional, but does not increase the storage size. It could be implemented the same way as we hand-roll the invalid values, only now the invalid values would be a part of the type. So, you could do something like:

using tid_t = OptionalInt<uint64_t, LLDB_INVALID_THREAD_ID>;

tid_t my_tid; // initialized to "invalid";
my_tid = get_thread_id();
if (my_tid) // no need to remember what is the correct "invalid" value here
  do_stuff(*my_tid);
else
  printf("no thread");

I am sure this is more than what you wanted to hear in this patch, but I wanted to make sure consider the tradeoffs before we start putting Optionals everywhere.

No, I think that's a good point. I held off on introducing llvm::Optional here because of similar reservations. I think it'd make sense to introduce an OptionalInt facility, or possibly something more general that supports non-POD types & more exotic invalid values. Definitely something to revisit.

lldb/trunk/include/lldb/Target/StackFrameList.h

 //===-- StackFrameList.h ----------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_StackFrameList_h_
 #define liblldb_StackFrameList_h_
 
-// C Includes
-// C++ Includes
 #include <memory>
 #include <mutex>
 #include <vector>
 
-// Other libraries and framework includes
-// Project includes
 #include "lldb/Target/StackFrame.h"
 
 namespace lldb_private {
 
 class StackFrameList {
 public:
   //------------------------------------------------------------------
   // Constructors and Destructors
   //------------------------------------------------------------------
   StackFrameList(Thread &thread, const lldb::StackFrameListSP &prev_frames_sp,
                  bool show_inline_frames);
 
   ~StackFrameList();
 
+  /// Get the number of visible frames. Frames may be created if \p can_create
+  /// is true. Synthetic (inline) frames expanded from the concrete frame #0
+  /// (aka invisible frames) are not included in this count.
   uint32_t GetNumFrames(bool can_create = true);
 
+  /// Get the frame at index \p idx. Invisible frames cannot be indexed.
   lldb::StackFrameSP GetFrameAtIndex(uint32_t idx);
 
+  /// Get the first concrete frame with index greater than or equal to \p idx.
+  /// Unlike \ref GetFrameAtIndex, this cannot return a synthetic frame.
   lldb::StackFrameSP GetFrameWithConcreteFrameIndex(uint32_t unwind_idx);
 
+  /// Retrieve the stack frame with the given ID \p stack_id.
   lldb::StackFrameSP GetFrameWithStackID(const StackID &stack_id);
 
-  // Mark a stack frame as the current frame
+  /// Mark a stack frame as the currently selected frame and return its index.
   uint32_t SetSelectedFrame(lldb_private::StackFrame *frame);
 
+  /// Get the currently selected frame index.
   uint32_t GetSelectedFrameIndex() const;
 
-  // Mark a stack frame as the current frame using the frame index
+  /// Mark a stack frame as the currently selected frame using the frame index
+  /// \p idx. Like \ref GetFrameAtIndex, invisible frames cannot be selected.
   bool SetSelectedFrameByIndex(uint32_t idx);
 
+  /// If the current inline depth (i.e the number of invisible frames) is valid,
+  /// subtract it from \p idx. Otherwise simply return \p idx.
   uint32_t GetVisibleStackFrameIndex(uint32_t idx) {
     if (m_current_inlined_depth < UINT32_MAX)
       return idx - m_current_inlined_depth;
     else
       return idx;
   }
 
+  /// Calculate and set the current inline depth. This may be used to update
+  /// the StackFrameList's set of inline frames when execution stops, e.g when
+  /// a breakpoint is hit.
   void CalculateCurrentInlinedDepth();
 
+  /// If the currently selected frame comes from the currently selected thread,
+  /// point the default file and line of the thread's target to the location
+  /// specified by the frame.
   void SetDefaultFileAndLineToSelectedFrame();
 
+  /// Clear the cache of frames.
   void Clear();
 
-  void InvalidateFrames(uint32_t start_idx);
-
   void Dump(Stream *s);
 
+  /// If \p stack_frame_ptr is contained in this StackFrameList, return its
+  /// wrapping shared pointer.
   lldb::StackFrameSP
   GetStackFrameSPForStackFramePtr(StackFrame *stack_frame_ptr);
 
   size_t GetStatus(Stream &strm, uint32_t first_frame, uint32_t num_frames,
                    bool show_frame_info, uint32_t num_frames_with_source,
                    bool show_unique = false,
                    const char *frame_marker = nullptr);
 
protected:
   uint32_t GetCurrentInlinedDepth();
 
   void SetCurrentInlinedDepth(uint32_t new_depth);
 
   typedef std::vector<lldb::StackFrameSP> collection;
   typedef collection::iterator iterator;
   typedef collection::const_iterator const_iterator;
 
+  /// The thread this frame list describes.
   Thread &m_thread;
+
+  /// The old stack frame list.
+  // TODO: The old stack frame list is used to fill in missing frame info
+  // heuristically when it's otherwise unavailable (say, because the unwinder
+  // fails). We should have stronger checks to make sure that this is a valid
+  // source of information.
   lldb::StackFrameListSP m_prev_frames_sp;
+
+  /// A mutex for this frame list.
+  // TODO: This mutex may not always be held when required. In particular, uses
+  // of the StackFrameList APIs in lldb_private::Thread look suspect. Consider
+  // passing around a lock_guard reference to enforce proper locking.
   mutable std::recursive_mutex m_mutex;
+
+  /// A cache of frames. This may need to be updated when the program counter
+  /// changes.
   collection m_frames;
+
+  /// The currently selected frame.
   uint32_t m_selected_frame_idx;
+
+  /// The number of concrete frames fetched while filling the frame list. This
+  /// is only used when synthetic frames are enabled.
   uint32_t m_concrete_frames_fetched;
+
+  /// The number of synthetic function activations (invisible frames) expanded
+  /// from the concrete frame #0 activation.
+  // TODO: Use an optional instead of UINT32_MAX to denote invalid values.
   uint32_t m_current_inlined_depth;
+
+  /// The program counter value at the currently selected synthetic activation.
+  /// This is only valid if m_current_inlined_depth is valid.
+  // TODO: Use an optional instead of UINT32_MAX to denote invalid values.
   lldb::addr_t m_current_inlined_pc;
+
+  /// Whether or not to show synthetic (inline) frames. Immutable.
   const bool m_show_inlined_frames;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(StackFrameList);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_StackFrameList_h_

lldb/trunk/source/Target/StackFrameList.cpp

 }
 
 uint32_t StackFrameList::GetNumFrames(bool can_create) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
 
   if (can_create)
     GetFramesUpTo(UINT32_MAX);
 
-  uint32_t inlined_depth = GetCurrentInlinedDepth();
-  if (inlined_depth == UINT32_MAX)
-    return m_frames.size();
-  else
-    return m_frames.size() - inlined_depth;
+  return GetVisibleStackFrameIndex(m_frames.size());
 }
 
 void StackFrameList::Dump(Stream *s) {
   if (s == nullptr)
     return;
 
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
 
if (pos->get() == frame) {
         m_selected_frame_idx -= inlined_depth;
       break;
     }
   }
   SetDefaultFileAndLineToSelectedFrame();
   return m_selected_frame_idx;
 }
 
-// Mark a stack frame as the current frame using the frame index
 bool StackFrameList::SetSelectedFrameByIndex(uint32_t idx) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   StackFrameSP frame_sp(GetFrameAtIndex(idx));
   if (frame_sp) {
     SetSelectedFrame(frame_sp.get());
     return true;
   } else
     return false;

 // The thread has been run, reset the number stack frames to zero so we can
 // determine how many frames we have lazily.
 void StackFrameList::Clear() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   m_frames.clear();
   m_concrete_frames_fetched = 0;
 }
 
-void StackFrameList::InvalidateFrames(uint32_t start_idx) {
-  std::lock_guard<std::recursive_mutex> guard(m_mutex);
-  if (m_show_inlined_frames) {
-    Clear();
-  } else {
-    const size_t num_frames = m_frames.size();
-    while (start_idx < num_frames) {
-      m_frames[start_idx].reset();
-      ++start_idx;
-    }
-  }
-}
-
 void StackFrameList::Merge(std::unique_ptr<StackFrameList> &curr_ap,
                            lldb::StackFrameListSP &prev_sp) {
   std::unique_lock<std::recursive_mutex> current_lock, previous_lock;
   if (curr_ap)
     current_lock = std::unique_lock<std::recursive_mutex>(curr_ap->m_mutex);
   if (prev_sp)
     previous_lock = std::unique_lock<std::recursive_mutex>(prev_sp->m_mutex);