[lldb] Reimplement LineTable::FindLineEntryByAddress on top of lower_bound

[labath]

I think this should be equivalent to the original implementation for all line tables occurring in practice. One difference I'm aware of is that the original implementation tried to return the first line entry out of multiple ones for the same address. However, this is not possible (anymore?) because of the check in LineTable::AppendLineEntryToSequence.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

I think this should be equivalent to the original implementation for all line tables occurring in practice. One difference I'm aware of is that the original implementation tried to return the first line entry out of multiple ones for the same address. However, this is not possible (anymore?) because of the check in LineTable::AppendLineEntryToSequence.

---

Full diff: https://github.com/llvm/llvm-project/pull/127799.diff

1 Files Affected:

• (modified) lldb/source/Symbol/LineTable.cpp (+10-62)

diff --git a/lldb/source/Symbol/LineTable.cpp b/lldb/source/Symbol/LineTable.cpp
index aae4ab59ff156..c199398f31a4c 100644
--- a/lldb/source/Symbol/LineTable.cpp
+++ b/lldb/source/Symbol/LineTable.cpp
@@ -233,69 +233,17 @@ bool LineTable::FindLineEntryByAddress(const Address &so_addr,
   if (index_ptr != nullptr)
     *index_ptr = UINT32_MAX;
 
-  bool success = false;
-
-  if (so_addr.GetModule().get() == m_comp_unit->GetModule().get()) {
-    Entry search_entry;
-    search_entry.file_addr = so_addr.GetFileAddress();
-    if (search_entry.file_addr != LLDB_INVALID_ADDRESS) {
-      entry_collection::const_iterator begin_pos = m_entries.begin();
-      entry_collection::const_iterator end_pos = m_entries.end();
-      entry_collection::const_iterator pos = std::lower_bound(
-          begin_pos, end_pos, search_entry, Entry::EntryAddressLessThan);
-      if (pos != end_pos) {
-        if (pos != begin_pos) {
-          if (pos->file_addr != search_entry.file_addr)
-            --pos;
-          else if (pos->file_addr == search_entry.file_addr) {
-            // If this is a termination entry, it shouldn't match since entries
-            // with the "is_terminal_entry" member set to true are termination
-            // entries that define the range for the previous entry.
-            if (pos->is_terminal_entry) {
-              // The matching entry is a terminal entry, so we skip ahead to
-              // the next entry to see if there is another entry following this
-              // one whose section/offset matches.
-              ++pos;
-              if (pos != end_pos) {
-                if (pos->file_addr != search_entry.file_addr)
-                  pos = end_pos;
-              }
-            }
-
-            if (pos != end_pos) {
-              // While in the same section/offset backup to find the first line
-              // entry that matches the address in case there are multiple
-              while (pos != begin_pos) {
-                entry_collection::const_iterator prev_pos = pos - 1;
-                if (prev_pos->file_addr == search_entry.file_addr &&
-                    prev_pos->is_terminal_entry == false)
-                  --pos;
-                else
-                  break;
-              }
-            }
-          }
-        }
-        else
-        {
-          // There might be code in the containing objfile before the first
-          // line table entry.  Make sure that does not get considered part of
-          // the first line table entry.
-          if (pos->file_addr > so_addr.GetFileAddress())
-            return false;
-        }
+  uint32_t idx = lower_bound(so_addr);
+  if (idx >= GetSize())
+    return false;
 
-        // Make sure we have a valid match and that the match isn't a
-        // terminating entry for a previous line...
-        if (pos != end_pos && pos->is_terminal_entry == false) {
-          uint32_t match_idx = std::distance(begin_pos, pos);
-          success = ConvertEntryAtIndexToLineEntry(match_idx, line_entry);
-          if (index_ptr != nullptr && success)
-            *index_ptr = match_idx;
-        }
-      }
-    }
-  }
+  addr_t file_addr = so_addr.GetFileAddress();
+  if (m_entries[idx].file_addr > file_addr)
+    return false;
+
+  bool success = ConvertEntryAtIndexToLineEntry(idx, line_entry);
+  if (index_ptr != nullptr && success)
+    *index_ptr = idx;
   return success;
 }
 

[JDevlieghere]

LGTM.

Since you're here... how would you feel about a little renaissance and returning a std::optional<uint32_t> to eliminate the index_ptr?

[labath]

I'm not sure what to think about that. The principal "result" of this function is the LineEntry object (most of the call sites don't even use the index), so the functional programmer in me wants to make this return an optional<LineEntry>. However, that doesn't sit very well with the code currently calling that function (because they are usually filling in the field directly in the SymbolContext). And optional<uint_32_t> behaves very much like bool if you're not interested in the actual index value -- which is nice, but then that wouldn't be very consistent with the other functions which return UINT32_MAX as a "not found" value.

If, after reading this, you still think this is a good idea, let me know, and I'll whip up a patch.

[JDevlieghere]

Ack, I'll always pick consistency over a slightly nicer interface. Let's keep it the way it is. Thanks for considering it.