Merge pull request swiftlang#9747 from jimingham/call-site-bkpts

Call site bkpts

Author: JDevlieghere

lldb/include/lldb/Breakpoint/BreakpointLocation.h

@@ -11,10 +11,12 @@
 
 #include <memory>
 #include <mutex>
+#include <optional>
 
 #include "lldb/Breakpoint/BreakpointOptions.h"
 #include "lldb/Breakpoint/StoppointHitCounter.h"
 #include "lldb/Core/Address.h"
+#include "lldb/Symbol/LineEntry.h"
 #include "lldb/Utility/UserID.h"
 #include "lldb/lldb-private.h"
 
@@ -282,6 +284,25 @@ class BreakpointLocation
   /// Returns the breakpoint location ID.
   lldb::break_id_t GetID() const { return m_loc_id; }
 
+  /// Set the line entry that should be shown to users for this location.
+  /// It is up to the caller to verify that this is a valid entry to show.
+  /// The current use of this is to distinguish among line entries from a
+  /// virtual inlined call stack that all share the same address.
+  /// The line entry must have the same start address as the address for this
+  /// location.
+  bool SetPreferredLineEntry(const LineEntry &line_entry) {
+    if (m_address == line_entry.range.GetBaseAddress()) {
+      m_preferred_line_entry = line_entry;
+      return true;
+    }
+    assert(0 && "Tried to set a preferred line entry with a different address");
+    return false;
+  }
+
+  const std::optional<LineEntry> GetPreferredLineEntry() {
+    return m_preferred_line_entry;
+  }
+
 protected:
   friend class BreakpointSite;
   friend class BreakpointLocationList;
@@ -306,6 +327,16 @@ class BreakpointLocation
   /// If it returns false we should continue, otherwise stop.
   bool IgnoreCountShouldStop();
 
+  /// If this location knows that the virtual stack frame it represents is
+  /// not frame 0, return the suggested stack frame instead.  This will happen
+  /// when the location's address contains a "virtual inlined call stack" and
+  /// the breakpoint was set on a file & line that are not at the bottom of that
+  /// stack.  For now we key off the "preferred line entry" - looking for that
+  /// in the blocks that start with the stop PC.
+  /// This version of the API doesn't take an "inlined" parameter because it
+  /// only changes frames in the inline stack.
+  std::optional<uint32_t> GetSuggestedStackFrameIndex();
+
 private:
   void SwapLocation(lldb::BreakpointLocationSP swap_from);
 
@@ -369,6 +400,11 @@ class BreakpointLocation
   lldb::break_id_t m_loc_id; ///< Breakpoint location ID.
   StoppointHitCounter m_hit_counter; ///< Number of times this breakpoint
                                      /// location has been hit.
+  /// If this exists, use it to print the stop description rather than the
+  /// LineEntry m_address resolves to directly.  Use this for instance when the
+  /// location was given somewhere in the virtual inlined call stack since the
+  /// Address always resolves to the lowest entry in the stack.
+  std::optional<LineEntry> m_preferred_line_entry;
 
   void SetShouldResolveIndirectFunctions(bool do_resolve) {
     m_should_resolve_indirect_functions = do_resolve;

lldb/include/lldb/Breakpoint/BreakpointSite.h

@@ -170,6 +170,11 @@ class BreakpointSite : public std::enable_shared_from_this<BreakpointSite>,
   /// \see lldb::DescriptionLevel
   void GetDescription(Stream *s, lldb::DescriptionLevel level);
 
+  // This runs through all the breakpoint locations owning this site and returns
+  // the greatest of their suggested stack frame indexes.  This only handles
+  // inlined stack changes.
+  std::optional<uint32_t> GetSuggestedStackFrameIndex();
+
   /// Tell whether a breakpoint has a location at this site.
   ///
   /// \param[in] bp_id

lldb/include/lldb/Core/Declaration.h

@@ -84,10 +84,14 @@ class Declaration {
   /// \param[in] declaration
   ///     The const Declaration object to compare with.
   ///
+  /// \param[in] full
+  ///     Same meaning as Full in FileSpec::Equal.  True means an empty
+  ///     directory is not equal to a specified one, false means it is equal.
+  ///
   /// \return
   ///     Returns \b true if \b declaration is at the same file and
   ///     line, \b false otherwise.
-  bool FileAndLineEqual(const Declaration &declaration) const;
+  bool FileAndLineEqual(const Declaration &declaration, bool full) const;
 
   /// Dump a description of this object to a Stream.
   ///

lldb/include/lldb/Target/StackFrameList.h

@@ -11,6 +11,7 @@
 
 #include <memory>
 #include <mutex>
+#include <shared_mutex>
 #include <vector>
 
 #include "lldb/Target/StackFrame.h"
@@ -94,24 +95,36 @@ class StackFrameList {
                    bool show_unique = false, bool show_hidden = false,
                    const char *frame_marker = nullptr);
 
+  /// Returns whether we have currently fetched all the frames of a stack.
+  bool WereAllFramesFetched() const;
+
 protected:
   friend class Thread;
   friend class ScriptedThread;
 
+  /// Use this API to build a stack frame list (used for scripted threads, for
+  /// instance.)  This API is not meant for StackFrameLists that have unwinders
+  /// and partake in lazy stack filling (using GetFramesUpTo).  Rather if you
+  /// are building StackFrameLists with this API, you should build the entire
+  /// list before making it available for use.
   bool SetFrameAtIndex(uint32_t idx, lldb::StackFrameSP &frame_sp);
 
-  /// Realizes frames up to (and including) end_idx (which can be greater than  
-  /// the actual number of frames.)  
+  /// Ensures that frames up to (and including) `end_idx` are realized in the
+  /// StackFrameList.  `end_idx` can be larger than the actual number of frames,
+  /// in which case all the frames will be fetched.  Acquires the writer end of
+  /// the list mutex.
   /// Returns true if the function was interrupted, false otherwise.
-  bool GetFramesUpTo(uint32_t end_idx, 
-      InterruptionControl allow_interrupt = AllowInterruption);
-
-  void GetOnlyConcreteFramesUpTo(uint32_t end_idx, Unwind &unwinder);
-
-  void SynthesizeTailCallFrames(StackFrame &next_frame);
-
-  bool GetAllFramesFetched() { return m_concrete_frames_fetched == UINT32_MAX; }
+  /// Callers should first check (under the shared mutex) whether we need to
+  /// fetch frames or not.
+  bool GetFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt);
+
+  // This should be called with either the reader or writer end of the list
+  // mutex held:
+  bool GetAllFramesFetched() const {
+    return m_concrete_frames_fetched == UINT32_MAX;
+  }
 
+  // This should be called with the writer end of the list mutex held.
   void SetAllFramesFetched() { m_concrete_frames_fetched = UINT32_MAX; }
 
   bool DecrementCurrentInlinedDepth();
@@ -122,6 +135,9 @@ class StackFrameList {
 
   void SetCurrentInlinedDepth(uint32_t new_depth);
 
+  /// Calls into the stack frame recognizers and stop info to set the most
+  /// relevant frame.  This can call out to arbitrary user code so it can't
+  /// hold the StackFrameList mutex.
   void SelectMostRelevantFrame();
 
   typedef std::vector<lldb::StackFrameSP> collection;
@@ -138,11 +154,16 @@ class StackFrameList {
   // 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 mutex for this frame list.  The only public API that requires the
+  /// unique lock is Clear.  All other clients take the shared lock, though
+  /// if we need more frames we may swap shared for unique to fulfill that
+  /// requirement.
+  mutable std::shared_mutex m_list_mutex;
+
+  // Setting the inlined depth should be protected against other attempts to
+  // change it, but since it doesn't mutate the list itself, we can limit the
+  // critical regions it produces by having a separate mutex.
+  mutable std::mutex m_inlined_depth_mutex;
 
   /// A cache of frames. This may need to be updated when the program counter
   /// changes.
@@ -171,6 +192,21 @@ class StackFrameList {
   const bool m_show_inlined_frames;
 
 private:
+  uint32_t SetSelectedFrameNoLock(lldb_private::StackFrame *frame);
+  lldb::StackFrameSP
+  GetFrameAtIndexNoLock(uint32_t idx,
+                        std::shared_lock<std::shared_mutex> &guard);
+
+  /// These two Fetch frames APIs and SynthesizeTailCallFrames are called in
+  /// GetFramesUpTo, they are the ones that actually add frames.  They must be
+  /// called with the writer end of the list mutex held.
+
+  /// Returns true if fetching frames was interrupted, false otherwise.
+  bool FetchFramesUpTo(uint32_t end_idx, InterruptionControl allow_interrupt);
+  /// Not currently interruptible so returns void.
+  void FetchOnlyConcreteFramesUpTo(uint32_t end_idx);
+  void SynthesizeTailCallFrames(StackFrame &next_frame);
+
   StackFrameList(const StackFrameList &) = delete;
   const StackFrameList &operator=(const StackFrameList &) = delete;
 };

lldb/include/lldb/Target/StopInfo.h

@@ -77,6 +77,18 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> {
       m_description.clear();
   }
 
+  /// This gives the StopInfo a chance to suggest a stack frame to select.
+  /// Passing true for inlined_stack will request changes to the inlined
+  /// call stack.  Passing false will request changes to the real stack
+  /// frame.  The inlined stack gets adjusted before we call into the thread
+  /// plans so they can reason based on the correct values.  The real stack
+  /// adjustment is handled after the frame recognizers get a chance to adjust
+  /// the frame.
+  virtual std::optional<uint32_t>
+  GetSuggestedStackFrameIndex(bool inlined_stack) {
+    return {};
+  }
+
   virtual bool IsValidForOperatingSystemThread(Thread &thread) { return true; }
 
   /// A Continue operation can result in a false stop event

lldb/include/lldb/Target/ThreadPlanStack.h

@@ -14,6 +14,8 @@
 #include <unordered_map>
 #include <vector>
 
+#include "llvm/Support/RWMutex.h"
+
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/lldb-private-forward.h"
@@ -102,9 +104,12 @@ class ThreadPlanStack {
   void SetTID(lldb::tid_t tid);
 
 private:
-  void PrintOneStack(Stream &s, llvm::StringRef stack_name,
-                     const PlanStack &stack, lldb::DescriptionLevel desc_level,
-                     bool include_internal) const;
+  lldb::ThreadPlanSP DiscardPlanNoLock();
+  lldb::ThreadPlanSP GetCurrentPlanNoLock() const;
+  void PrintOneStackNoLock(Stream &s, llvm::StringRef stack_name,
+                           const PlanStack &stack,
+                           lldb::DescriptionLevel desc_level,
+                           bool include_internal) const;
 
   PlanStack m_plans;           ///< The stack of plans this thread is executing.
   PlanStack m_completed_plans; ///< Plans that have been completed by this
@@ -116,8 +121,8 @@ class ThreadPlanStack {
   size_t m_completed_plan_checkpoint = 0; // Monotonically increasing token for
                                           // completed plan checkpoints.
   std::unordered_map<size_t, PlanStack> m_completed_plan_store;
-  mutable std::recursive_mutex m_stack_mutex;
-  
+  mutable llvm::sys::RWMutex m_stack_mutex;
+
   // ThreadPlanStacks shouldn't be copied.
   ThreadPlanStack(ThreadPlanStack &rhs) = delete;
 };

lldb/include/lldb/Target/ThreadPlanStepInRange.h

@@ -100,8 +100,8 @@ class ThreadPlanStepInRange : public ThreadPlanStepRange,
   bool m_step_past_prologue; // FIXME: For now hard-coded to true, we could put
                              // a switch in for this if there's
                              // demand for that.
-  bool m_virtual_step; // true if we've just done a "virtual step", i.e. just
-                       // moved the inline stack depth.
+  LazyBool m_virtual_step;   // true if we've just done a "virtual step", i.e.
+                             // just moved the inline stack depth.
   ConstString m_step_into_target;
   std::vector<lldb::break_id_t> m_step_in_deep_bps; // Places where we might
                                                     // want to stop when we do a

lldb/source/Breakpoint/BreakpointLocation.cpp

@@ -508,8 +508,20 @@ void BreakpointLocation::GetDescription(Stream *s,
         s->PutCString("re-exported target = ");
       else
         s->PutCString("where = ");
+
+      // If there's a preferred line entry for printing, use that.
+      bool show_function_info = true;
+      if (auto preferred = GetPreferredLineEntry()) {
+        sc.line_entry = *preferred;
+        // FIXME: We're going to get the function name wrong when the preferred
+        // line entry is not the lowest one.  For now, just leave the function
+        // out in this case, but we really should also figure out how to easily
+        // fake the function name here.
+        show_function_info = false;
+      }
       sc.DumpStopContext(s, m_owner.GetTarget().GetProcessSP().get(), m_address,
-                         false, true, false, true, true, true);
+                         false, true, false, show_function_info,
+                         show_function_info, show_function_info);
     } else {
       if (sc.module_sp) {
         s->EOL();
@@ -537,7 +549,10 @@ void BreakpointLocation::GetDescription(Stream *s,
         if (sc.line_entry.line > 0) {
           s->EOL();
           s->Indent("location = ");
-          sc.line_entry.DumpStopContext(s, true);
+          if (auto preferred = GetPreferredLineEntry())
+            preferred->DumpStopContext(s, true);
+          else
+            sc.line_entry.DumpStopContext(s, true);
         }
 
       } else {
@@ -656,6 +671,50 @@ void BreakpointLocation::SendBreakpointLocationChangedEvent(
   }
 }
 
+std::optional<uint32_t> BreakpointLocation::GetSuggestedStackFrameIndex() {
+  auto preferred_opt = GetPreferredLineEntry();
+  if (!preferred_opt)
+    return {};
+  LineEntry preferred = *preferred_opt;
+  SymbolContext sc;
+  if (!m_address.CalculateSymbolContext(&sc))
+    return {};
+  // Don't return anything special if frame 0 is the preferred line entry.
+  // We not really telling the stack frame list to do anything special in that
+  // case.
+  if (!LineEntry::Compare(sc.line_entry, preferred))
+    return {};
+
+  if (!sc.block)
+    return {};
+
+  // Blocks have their line info in Declaration form, so make one here:
+  Declaration preferred_decl(preferred.GetFile(), preferred.line,
+                             preferred.column);
+
+  uint32_t depth = 0;
+  Block *inlined_block = sc.block->GetContainingInlinedBlock();
+  while (inlined_block) {
+    // If we've moved to a block that this isn't the start of, that's not
+    // our inlining info or call site, so we can stop here.
+    Address start_address;
+    if (!inlined_block->GetStartAddress(start_address) ||
+        start_address != m_address)
+      return {};
+
+    const InlineFunctionInfo *info = inlined_block->GetInlinedFunctionInfo();
+    if (info) {
+      if (preferred_decl == info->GetDeclaration())
+        return depth;
+      if (preferred_decl == info->GetCallSite())
+        return depth + 1;
+    }
+    inlined_block = inlined_block->GetInlinedParent();
+    depth++;
+  }
+  return {};
+}
+
 void BreakpointLocation::SwapLocation(BreakpointLocationSP swap_from) {
   m_address = swap_from->m_address;
   m_should_resolve_indirect_functions =

lldb/source/Breakpoint/BreakpointResolver.cpp

@@ -340,6 +340,21 @@ void BreakpointResolver::AddLocation(SearchFilter &filter,
   }
 
   BreakpointLocationSP bp_loc_sp(AddLocation(line_start));
+  // If the address that we resolved the location to returns a different
+  // LineEntry from the one in the incoming SC, we're probably dealing with an
+  // inlined call site, so set that as the preferred LineEntry:
+  LineEntry resolved_entry;
+  if (!skipped_prologue && bp_loc_sp &&
+      line_start.CalculateSymbolContextLineEntry(resolved_entry) &&
+      LineEntry::Compare(resolved_entry, sc.line_entry)) {
+    // FIXME: The function name will also be wrong here.  Do we need to record
+    // that as well, or can we figure that out again when we report this
+    // breakpoint location.
+    if (!bp_loc_sp->SetPreferredLineEntry(sc.line_entry)) {
+      LLDB_LOG(log, "Tried to add a preferred line entry that didn't have the "
+                    "same address as this location's address.");
+    }
+  }
   if (log && bp_loc_sp && !GetBreakpoint()->IsInternal()) {
     StreamString s;
     bp_loc_sp->GetDescription(&s, lldb::eDescriptionLevelVerbose);