Add the ability to break on call-site locations, improve inline stepping (#112939)

Previously lldb didn't support setting breakpoints on call site
locations. This patch adds that ability.

It would be very slow if we did this by searching all the debug
information for every inlined subroutine record looking for a call-site
match, so I added one restriction to the call-site support. This change
will find all call sites for functions that also supply at least one
line to the regular line table. That way we can use the fact that the
line table search will move the location to that subsequent line (but
only within the same function). When we find an actually moved source
line match, we can search in the function that contained that line table
entry for the call-site, and set the breakpoint location back to that.

When I started writing tests for this new ability, it quickly became
obvious that our support for virtual inline stepping was pretty buggy.
We didn't print the right file & line number for the breakpoint, and we
didn't set the position in the "virtual inlined stack" correctly when we
hit the breakpoint. We also didn't step through the inlined frames
correctly. There was code to try to detect the right inlined stack
position, but it had been refactored a while back with the comment that
it was super confusing and the refactor was supposed to make it clearer,
but the refactor didn't work either.

That code was made much clearer by abstracting the job of "handling the
stack readjustment" to the various StopInfo's. Previously, there was a
big (and buggy) switch over stop info's. Moving the responsibility to
the stop info made this code much easier to reason about.

We also had no tests for virtual inlined stepping (our inlined stepping
test was actually written specifically to avoid the formation of a
virtual inlined stack... So I also added tests for that along with the
tests for setting the call-site breakpoints.

Author: jimingham

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/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/ThreadPlanStepInRange.h

@@ -80,8 +80,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;
   ThreadPlanStepInRange(const ThreadPlanStepInRange &) = delete;
   const ThreadPlanStepInRange &

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);

lldb/source/Breakpoint/BreakpointSite.cpp

@@ -87,6 +87,23 @@ void BreakpointSite::GetDescription(Stream *s, lldb::DescriptionLevel level) {
   m_constituents.GetDescription(s, level);
 }
 
+std::optional<uint32_t> BreakpointSite::GetSuggestedStackFrameIndex() {
+
+  std::optional<uint32_t> result;
+  std::lock_guard<std::recursive_mutex> guard(m_constituents_mutex);
+  for (BreakpointLocationSP loc_sp : m_constituents.BreakpointLocations()) {
+    std::optional<uint32_t> loc_frame_index =
+        loc_sp->GetSuggestedStackFrameIndex();
+    if (loc_frame_index) {
+      if (result)
+        result = std::max(*loc_frame_index, *result);
+      else
+        result = loc_frame_index;
+    }
+  }
+  return result;
+}
+
 bool BreakpointSite::IsInternal() const { return m_constituents.IsInternal(); }
 
 uint8_t *BreakpointSite::GetTrapOpcodeBytes() { return &m_trap_opcode[0]; }

lldb/source/Core/Declaration.cpp

@@ -70,8 +70,9 @@ int Declaration::Compare(const Declaration &a, const Declaration &b) {
   return 0;
 }
 
-bool Declaration::FileAndLineEqual(const Declaration &declaration) const {
-  int file_compare = FileSpec::Compare(this->m_file, declaration.m_file, true);
+bool Declaration::FileAndLineEqual(const Declaration &declaration,
+                                   bool full) const {
+  int file_compare = FileSpec::Compare(this->m_file, declaration.m_file, full);
   return file_compare == 0 && this->m_line == declaration.m_line;
 }
 

lldb/source/Symbol/Block.cpp

@@ -230,7 +230,7 @@ Block *Block::GetContainingInlinedBlockWithCallSite(
     const auto *function_info = inlined_block->GetInlinedFunctionInfo();
 
     if (function_info &&
-        function_info->GetCallSite().FileAndLineEqual(find_call_site))
+        function_info->GetCallSite().FileAndLineEqual(find_call_site, true))
       return inlined_block;
     inlined_block = inlined_block->GetInlinedParent();
   }