Add the ability to break on call-site locations, improve inline stepping

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,18 @@ 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.
+  void SetPreferredLineEntry(const LineEntry &line_entry) {
+    m_preferred_line_entry = line_entry;
+  }
+
+  const std::optional<LineEntry> GetPreferredLineEntry() {
+    return m_preferred_line_entry;
+  }
+
 protected:
   friend class BreakpointSite;
   friend class BreakpointLocationList;
@@ -306,6 +320,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 +393,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/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 (GetPreferredLineEntry()) {
+        sc.line_entry = *GetPreferredLineEntry();
+        // 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 (GetPreferredLineEntry())
+            GetPreferredLineEntry()->DumpStopContext(s, true);
+          else
+            sc.line_entry.DumpStopContext(s, true);
         }
 
       } else {
@@ -656,6 +671,49 @@ void BreakpointLocation::SendBreakpointLocationChangedEvent(
   }
 }
 
+std::optional<uint32_t> BreakpointLocation::GetSuggestedStackFrameIndex() {
+  if (!GetPreferredLineEntry())
+    return {};
+  LineEntry preferred = *GetPreferredLineEntry();
+  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,18 @@ 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.
+    bp_loc_sp->SetPreferredLineEntry(sc.line_entry);
+  }
   if (log && bp_loc_sp && !GetBreakpoint()->IsInternal()) {
     StreamString s;
     bp_loc_sp->GetDescription(&s, lldb::eDescriptionLevelVerbose);

lldb/source/Breakpoint/BreakpointSite.cpp

@@ -87,6 +87,22 @@ 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> this_result = loc_sp->GetSuggestedStackFrameIndex();
+    if (this_result) {
+      if (!result)
+        result = this_result;
+      else
+        result = std::max(*this_result, *result);
+    }
+  }
+  return result;
+}
+
 bool BreakpointSite::IsInternal() const { return m_constituents.IsInternal(); }
 
 uint8_t *BreakpointSite::GetTrapOpcodeBytes() { return &m_trap_opcode[0]; }

lldb/source/Core/Declaration.cpp

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

lldb/source/Symbol/CompileUnit.cpp

@@ -251,7 +251,10 @@ void CompileUnit::ResolveSymbolContext(
     SymbolContextItem resolve_scope, SymbolContextList &sc_list,
     RealpathPrefixes *realpath_prefixes) {
   const FileSpec file_spec = src_location_spec.GetFileSpec();
-  const uint32_t line = src_location_spec.GetLine().value_or(0);
+  const uint32_t line =
+      src_location_spec.GetLine().value_or(LLDB_INVALID_LINE_NUMBER);
+  const uint32_t column_num =
+      src_location_spec.GetColumn().value_or(LLDB_INVALID_COLUMN_NUMBER);
   const bool check_inlines = src_location_spec.GetCheckInlines();
 
   // First find all of the file indexes that match our "file_spec". If
@@ -312,6 +315,112 @@ void CompileUnit::ResolveSymbolContext(
         0, file_indexes, src_location_spec, &line_entry);
   }
 
+  // If we didn't manage to find a breakpoint that matched the line number
+  // requested, that might be because it is only an inline call site, and
+  // doesn't have a line entry in the line table.  Scan for that here.
+  //
+  // We are making the assumption that if there was an inlined function it will
+  // contribute at least 1 non-call-site entry to the line table.  That's handy
+  // because we don't move line breakpoints over function boundaries, so if we
+  // found a hit, and there were also a call site entry, it would have to be in
+  // the function containing the PC of the line table match.  That way we can
+  // limit the call site search to that function.
+  // We will miss functions that ONLY exist as a call site entry.
+
+  if (line_entry.IsValid() &&
+      (line_entry.line != line || line_entry.column != column_num) &&
+      resolve_scope & eSymbolContextLineEntry && check_inlines) {
+    // We don't move lines over function boundaries, so the address in the
+    // line entry will be the in function that contained the line that might
+    // be a CallSite, and we can just iterate over that function to find any
+    // inline records, and dig up their call sites.
+    Address start_addr = line_entry.range.GetBaseAddress();
+    Function *function = start_addr.CalculateSymbolContextFunction();
+
+    Declaration sought_decl(file_spec, line, column_num);
+    // We use this recursive function to descend the block structure looking
+    // for a block that has this Declaration as in it's CallSite info.
+    // This function recursively scans the sibling blocks of the incoming
+    // block parameter.
+    std::function<void(Block &)> examine_block =
+        [&sought_decl, &sc_list, &src_location_spec, resolve_scope,
+         &examine_block](Block &block) -> void {
+      // Iterate over the sibling child blocks of the incoming block.
+      Block *sibling_block = block.GetFirstChild();
+      while (sibling_block) {
+        // We only have to descend through the regular blocks, looking for
+        // immediate inlines, since those are the only ones that will have this
+        // callsite.
+        const InlineFunctionInfo *inline_info =
+            sibling_block->GetInlinedFunctionInfo();
+        if (inline_info) {
+          // If this is the call-site we are looking for, record that:
+          // We need to be careful because the call site from the debug info
+          // will generally have a column, but the user might not have specified
+          // it.
+          Declaration found_decl = inline_info->GetCallSite();
+          uint32_t sought_column = sought_decl.GetColumn();
+          if (found_decl.FileAndLineEqual(sought_decl) &&
+              (sought_column == LLDB_INVALID_COLUMN_NUMBER ||
+               sought_column == found_decl.GetColumn())) {
+            // If we found a call site, it belongs not in this inlined block,
+            // but in the parent block that inlined it.
+            Address parent_start_addr;
+            if (sibling_block->GetParent()->GetStartAddress(
+                    parent_start_addr)) {
+              SymbolContext sc;
+              parent_start_addr.CalculateSymbolContext(&sc, resolve_scope);
+              // Now swap out the line entry for the one we found.
+              LineEntry call_site_line = sc.line_entry;
+              call_site_line.line = found_decl.GetLine();
+              call_site_line.column = found_decl.GetColumn();
+              bool matches_spec = true;
+              // If the user asked for an exact match, we need to make sure the
+              // call site we found actually matches the location.
+              if (src_location_spec.GetExactMatch()) {
+                matches_spec = false;
+                if ((src_location_spec.GetFileSpec() ==
+                     sc.line_entry.GetFile()) &&
+                    (src_location_spec.GetLine() &&
+                     *src_location_spec.GetLine() == call_site_line.line) &&
+                    (src_location_spec.GetColumn() &&
+                     *src_location_spec.GetColumn() == call_site_line.column))
+                  matches_spec = true;
+              }
+              if (matches_spec &&
+                  sibling_block->GetRangeAtIndex(0, call_site_line.range)) {
+                SymbolContext call_site_sc(sc.target_sp, sc.module_sp,
+                                           sc.comp_unit, sc.function, sc.block,
+                                           &call_site_line, sc.symbol);
+                sc_list.Append(call_site_sc);
+              }
+            }
+          }
+        }
+
+        // Descend into the child blocks:
+        examine_block(*sibling_block);
+        // Now go to the next sibling:
+        sibling_block = sibling_block->GetSibling();
+      }
+    };
+
+    if (function) {
+      // We don't need to examine the function block, it can't be inlined.
+      Block &func_block = function->GetBlock(true);
+      examine_block(func_block);
+    }
+    // If we found entries here, we are done.  We only get here because we
+    // didn't find an exact line entry for this line & column, but if we found
+    // an exact match from the call site info that's strictly better than
+    // continuing to look for matches further on in the file.
+    // FIXME: Should I also do this for "call site line exists between the
+    // given line number and the later line we found in the line table"?  That's
+    // a closer approximation to our general sliding algorithm.
+    if (sc_list.GetSize())
+      return;
+  }
+
   // If "exact == true", then "found_line" will be the same as "line". If
   // "exact == false", the "found_line" will be the closest line entry
   // with a line number greater than "line" and we will use this for our