[lldb/Symbol] Fix column breakpoint `move_to_nearest_code` match

[mib]

Summary

This patch fixes the column symbol resolution when creating a breakpoint
with the move_to_nearest_code flag set.

In order to achieve this, the patch adds column information handling in
the LineTable's LineEntry finder. After experimenting a little, it
turns out the most natural approach in case of an inaccurate column match,
is to move backward and match the previous LineEntry rather than going
forward like we do with simple line breakpoints.

The patch also reflows the function to reduce code duplication.

Finally, it updates the BreakpointResolver heuristic to align it with
the LineTable method.

rdar://73218201

Signed-off-by: Med Ismail Bennani <[e-mail]>

[JDevlieghere]

lldb/include/lldb/Symbol/LineTable.h

414

It seems like this method is only using m_entries, so I would turn this into a static (non-member) function in the cpp file and pass m_entries as an argument.

440–442

nit: reflow

447

I know you're just moving the code but maybe a good opportunity to simplify the else-after-continue and else-after-return below.

lldb/source/Breakpoint/BreakpointResolver.cpp

230–234 ↗

(On Diff #341813)

Do these comments need to be updated?

[mib]

lldb/include/lldb/Symbol/LineTable.h

414

Besides m_entries, the function also calls LineTable::ConvertEntryAtIndexToLineEntry which is declared protected. Making the function "static" makes it more convoluted, IMO. I'd rather leave it in the header file.

447

As discussed, I'll simplify these in a follow-up patch.

[mib]

Address @JDevlieghere comments.

[JDevlieghere]

LGTM

lldb/include/lldb/Symbol/LineTable.h

public:
   ///     line_entry is left untouched.
   ///
   /// \return
   ///     Returns \b true if a matching line entry is found in this
   ///     line table, \b false otherwise.
   ///
   /// \see CompileUnit::GetSupportFiles()
   /// \see FileSpecList::FindFileIndex (uint32_t, const FileSpec &) const
+  template <typename T>
   uint32_t
-  FindLineEntryIndexByFileIndex(uint32_t start_idx, uint32_t file_idx,
+  FindLineEntryIndexByFileIndex(uint32_t start_idx, T file_idx,
                                 const SourceLocationSpec &src_location_spec,
-                                LineEntry *line_entry_ptr);
+                                LineEntry *line_entry_ptr) {
+    const size_t count = m_entries.size();
+    size_t best_match = UINT32_MAX;
+
+    if (!line_entry_ptr)
+      return best_match;
+
+    const uint32_t line = src_location_spec.GetLine();
+    const auto column = src_location_spec.GetColumn();
+    const bool exact = src_location_spec.GetExactMatch();
+
+    std::function<bool(int)> file_idx_matcher;
+    if constexpr (std::is_integral<T>()) {
+      file_idx_matcher = [this, file_idx](int idx) {
+        return m_entries[idx].file_idx == file_idx;
+      };
+    } else {
+      file_idx_matcher = [this, file_idx](int idx) {
+        return llvm::is_contained(file_idx, m_entries[idx].file_idx);
+      };
+    }
 
-  uint32_t FindLineEntryIndexByFileIndex(
-      uint32_t start_idx, const std::vector<uint32_t> &file_indexes,
-      const SourceLocationSpec &src_location_spec, LineEntry *line_entry_ptr);
+    for (size_t idx = start_idx; idx < count; ++idx) {
+      // Skip line table rows that terminate the previous row (is_terminal_entry
+      // is non-zero)
+      if (m_entries[idx].is_terminal_entry)
+        continue;
+
+      if (!file_idx_matcher(idx))
+        continue;
+
+      // Exact match always wins.  Otherwise try to find the closest line > the
+      // desired line.
+      // FIXME: Maybe want to find the line closest before and the line closest
+      // after and
+      // if they're not in the same function, don't return a match.
+
+      if (!column) {
+        if (m_entries[idx].line < line) {
+          continue;
+        } else if (m_entries[idx].line == line) {
+          ConvertEntryAtIndexToLineEntry(idx, *line_entry_ptr);
+          return idx;
+        } else if (!exact) {
+          if (best_match == UINT32_MAX ||
+              m_entries[idx].line < m_entries[best_match].line)
+            best_match = idx;
+        }
+      } else {
+        if (m_entries[idx].line < line) {
+          continue;
+        } else if (m_entries[idx].line == line &&
+                   m_entries[idx].column == *column) {
+          ConvertEntryAtIndexToLineEntry(idx, *line_entry_ptr);
+          return idx;
+        } else if (!exact) {
+          if (best_match == UINT32_MAX)
+            best_match = idx;
+          else if (m_entries[idx].line == m_entries[best_match].line) {
+            if (std::abs(m_entries[idx].column - *column) <
+                std::abs(m_entries[best_match].column - *column))
+              best_match = idx;
+          }
+        }
+      }
+    }
+
+    if (best_match != UINT32_MAX) {
+      if (line_entry_ptr)
+        ConvertEntryAtIndexToLineEntry(best_match, *line_entry_ptr);
+      return best_match;
+    }
+    return UINT32_MAX;
+  }
 
   size_t FineLineEntriesForFileIndex(uint32_t file_idx, bool append,
                                      SymbolContextList &sc_list);
 
   /// Get the line entry from the line table at index \a idx.
   ///
   /// \param[in] idx
   ///     An index into the line table entry collection.
#undef SCALAR_COMPARE
 
   bool ConvertEntryAtIndexToLineEntry(uint32_t idx, LineEntry &line_entry);
 
 private:
   LineTable(const LineTable &) = delete;
   const LineTable &operator=(const LineTable &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SYMBOL_LINETABLE_H

lldb/source/Symbol/LineTable.cpp

 //===-- LineTable.cpp -----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Symbol/LineTable.h"
 #include "lldb/Core/Address.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Utility/Stream.h"
 #include <algorithm>
+#include <functional>
 
 using namespace lldb;
 using namespace lldb_private;
 
 // LineTable constructor
 LineTable::LineTable(CompileUnit *comp_unit)
     : m_comp_unit(comp_unit), m_entries() {}
 
bool LineTable::ConvertEntryAtIndexToLineEntry(uint32_t idx,
   line_entry.is_start_of_statement = entry.is_start_of_statement;
   line_entry.is_start_of_basic_block = entry.is_start_of_basic_block;
   line_entry.is_prologue_end = entry.is_prologue_end;
   line_entry.is_epilogue_begin = entry.is_epilogue_begin;
   line_entry.is_terminal_entry = entry.is_terminal_entry;
   return true;
 }
 
-uint32_t LineTable::FindLineEntryIndexByFileIndex(
-    uint32_t start_idx, const std::vector<uint32_t> &file_indexes,
-    const SourceLocationSpec &src_location_spec, LineEntry *line_entry_ptr) {
-
-  const size_t count = m_entries.size();
-  size_t best_match = UINT32_MAX;
-
-  for (size_t idx = start_idx; idx < count; ++idx) {
-    // Skip line table rows that terminate the previous row (is_terminal_entry
-    // is non-zero)
-    if (m_entries[idx].is_terminal_entry)
-      continue;
-
-    if (!llvm::is_contained(file_indexes, m_entries[idx].file_idx))
-      continue;
-
-    // Exact match always wins.  Otherwise try to find the closest line > the
-    // desired line.
-    // FIXME: Maybe want to find the line closest before and the line closest
-    // after and
-    // if they're not in the same function, don't return a match.
-
-    uint32_t line = src_location_spec.GetLine();
-
-    if (m_entries[idx].line < line) {
-      continue;
-    } else if (m_entries[idx].line == line) {
-      if (line_entry_ptr)
-        ConvertEntryAtIndexToLineEntry(idx, *line_entry_ptr);
-      return idx;
-    } else if (!src_location_spec.GetExactMatch()) {
-      if (best_match == UINT32_MAX)
-        best_match = idx;
-      else if (m_entries[idx].line < m_entries[best_match].line)
-        best_match = idx;
-    }
-  }
-
-  if (best_match != UINT32_MAX) {
-    if (line_entry_ptr)
-      ConvertEntryAtIndexToLineEntry(best_match, *line_entry_ptr);
-    return best_match;
-  }
-  return UINT32_MAX;
-}
-
-uint32_t LineTable::FindLineEntryIndexByFileIndex(
-    uint32_t start_idx, uint32_t file_idx,
-    const SourceLocationSpec &src_location_spec, LineEntry *line_entry_ptr) {
-  const size_t count = m_entries.size();
-  size_t best_match = UINT32_MAX;
-
-  for (size_t idx = start_idx; idx < count; ++idx) {
-    // Skip line table rows that terminate the previous row (is_terminal_entry
-    // is non-zero)
-    if (m_entries[idx].is_terminal_entry)
-      continue;
-
-    if (m_entries[idx].file_idx != file_idx)
-      continue;
-
-    // Exact match always wins.  Otherwise try to find the closest line > the
-    // desired line.
-    // FIXME: Maybe want to find the line closest before and the line closest
-    // after and
-    // if they're not in the same function, don't return a match.
-
-    uint32_t line = src_location_spec.GetLine();
-
-    if (m_entries[idx].line < line) {
-      continue;
-    } else if (m_entries[idx].line == line) {
-      if (line_entry_ptr)
-        ConvertEntryAtIndexToLineEntry(idx, *line_entry_ptr);
-      return idx;
-    } else if (!src_location_spec.GetExactMatch()) {
-      if (best_match == UINT32_MAX)
-        best_match = idx;
-      else if (m_entries[idx].line < m_entries[best_match].line)
-        best_match = idx;
-    }
-  }
-
-  if (best_match != UINT32_MAX) {
-    if (line_entry_ptr)
-      ConvertEntryAtIndexToLineEntry(best_match, *line_entry_ptr);
-    return best_match;
-  }
-  return UINT32_MAX;
-}
-
 size_t LineTable::FineLineEntriesForFileIndex(uint32_t file_idx, bool append,
                                               SymbolContextList &sc_list) {
 
   if (!append)
     sc_list.Clear();
 
   size_t num_added = 0;
   const size_t count = m_entries.size();