[lldb] NFC: refactor CompileUnit::ResolveSymbolContext

[kwk]

Summary

I found the above named method hard to read because it had

a) many nested blocks,
b) one return statement at the end with some logic involved,
c) a duplicated while-loop with just small differences in it.

I decided to refactor this function by employing an early exit strategy.
In order to capture the logic in the return statement and to not have it
repeated more than once I chose to implement a very small lamda function
that captures all the variables it needs.
I also replaced the two while-loops with just one.

This is a non-functional change (NFC).

[kwk]

Small changes

[teemperor]

Let's just remove that return value. It's anyway only used in one place or so where we can just call GetSize() manually. Then we also don't need the lambda. Otherwise this LGTM, thanks!

[labath]

What we've started doing these days is to just remove the "number of things appended" return value from these kinds of functions, as they make it very easy to introduce bugs. (And most callers don't care about those, and those that do, can implement this easily on their own.)

It doesn't look like this function has that many callers, so it should be easy to do the same here...

[kwk]

In D70774#1761444, @teemperor wrote:

Let's just remove that return value. It's anyway only used in one place or so where we can just call GetSize() manually. Then we also don't need the lambda. Otherwise this LGTM, thanks!

Okay, fair enough. Let me do the changes.

[kwk]

• Have ResolveSymbolContext not return anything

[teemperor]

LGTM.

[kwk]

Reopening because I did find the logic error.

[kwk]

• Fixed logic error

[kwk]

Running tests locally.

lldb/include/lldb/Symbol/CompileUnit.h

public:
   ///     enumeration for a list of all available bits that can be
   ///     resolved. Only SymbolContext entries that can be resolved
   ///     using a LineEntry base address will be able to be resolved.
   ///
   /// \param[out] sc_list
   ///     A SymbolContext list class that will get any matching
   ///     entries appended to.
   ///
-  /// \return
-  ///     The number of new matches that were added to \a sc_list.
-  ///
   /// \see enum SymbolContext::Scope
-  uint32_t ResolveSymbolContext(const FileSpec &file_spec, uint32_t line,
-                                bool check_inlines, bool exact,
-                                lldb::SymbolContextItem resolve_scope,
-                                SymbolContextList &sc_list);
+  void ResolveSymbolContext(const FileSpec &file_spec, uint32_t line,
 
   /// Get whether compiler optimizations were enabled for this compile unit
   ///
   /// "optimized" means that the debug experience may be difficult for the
   /// user to understand.  Variables may not be available when the developer
   /// would expect them, stepping through the source lines in the function may
   /// appear strange, etc.
   ///

lldb/source/API/SBThread.cpp

if (exe_ctx.HasThreadScope()) {
 
     std::vector<addr_t> step_over_until_addrs;
     const bool abort_other_plans = false;
     const bool stop_other_threads = false;
     const bool check_inlines = true;
     const bool exact = false;
 
     SymbolContextList sc_list;
-    const uint32_t num_matches = frame_sc.comp_unit->ResolveSymbolContext(
-        step_file_spec, line, check_inlines, exact, eSymbolContextLineEntry,
-        sc_list);
+    frame_sc.comp_unit->ResolveSymbolContext(step_file_spec, line,
+                                             check_inlines, exact,
+                                             eSymbolContextLineEntry, sc_list);
+    const uint32_t num_matches = sc_list.GetSize();
     if (num_matches > 0) {
       SymbolContext sc;
       for (uint32_t i = 0; i < num_matches; ++i) {
         if (sc_list.GetContextAtIndex(i, sc)) {
           addr_t step_addr =
               sc.line_entry.range.GetBaseAddress().GetLoadAddress(target);
           if (step_addr != LLDB_INVALID_ADDRESS) {
             if (fun_range.ContainsLoadAddress(step_addr, target))

lldb/source/Core/AddressResolverFileLine.cpp

: AddressResolver(), m_file_spec(file_spec), m_line_number(line_no),
       m_inlines(check_inlines) {}
 
 AddressResolverFileLine::~AddressResolverFileLine() {}
 
 Searcher::CallbackReturn
 AddressResolverFileLine::SearchCallback(SearchFilter &filter,
                                         SymbolContext &context, Address *addr) {
   SymbolContextList sc_list;
-  uint32_t sc_list_size;
   CompileUnit *cu = context.comp_unit;
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
 
-  sc_list_size =
-      cu->ResolveSymbolContext(m_file_spec, m_line_number, m_inlines, false,
-                               eSymbolContextEverything, sc_list);
+  uint32_t sc_list_size = sc_list.GetSize();
   for (uint32_t i = 0; i < sc_list_size; i++) {
     SymbolContext sc;
     if (sc_list.GetContextAtIndex(i, sc)) {
       Address line_start = sc.line_entry.range.GetBaseAddress();
       addr_t byte_size = sc.line_entry.range.GetByteSize();
       if (line_start.IsValid()) {
         AddressRange new_range(line_start, byte_size);
         m_address_ranges.push_back(new_range);

lldb/source/Symbol/CompileUnit.cpp

uint32_t CompileUnit::FindLineEntry(uint32_t start_idx, uint32_t line,
   }
   LineTable *line_table = GetLineTable();
   if (line_table)
     return line_table->FindLineEntryIndexByFileIndex(start_idx, file_idx, line,
                                                      exact, line_entry_ptr);
   return UINT32_MAX;
 }
 
-uint32_t CompileUnit::ResolveSymbolContext(const FileSpec &file_spec,
-                                           uint32_t line, bool check_inlines,
-                                           bool exact,
-                                           SymbolContextItem resolve_scope,
-                                           SymbolContextList &sc_list) {
+void CompileUnit::ResolveSymbolContext(const FileSpec &file_spec,
   // First find all of the file indexes that match our "file_spec". If
   // "file_spec" has an empty directory, then only compare the basenames when
   // finding file indexes
   std::vector<uint32_t> file_indexes;
   const bool full_match = (bool)file_spec.GetDirectory();
   bool file_spec_matches_cu_file_spec =
       FileSpec::Equal(file_spec, *this, full_match);
 
   // If we are not looking for inlined functions and our file spec doesn't
   // match then we are done...
   if (!file_spec_matches_cu_file_spec && !check_inlines)
-    return 0;
+    return;
 
   uint32_t file_idx =
       GetSupportFiles().FindFileIndex(1, file_spec, true);
   while (file_idx != UINT32_MAX) {
     file_indexes.push_back(file_idx);
     file_idx = GetSupportFiles().FindFileIndex(file_idx + 1, file_spec, true);
   }
 
   const size_t num_file_indexes = file_indexes.size();
   if (num_file_indexes == 0)
-    return 0;
-
-  const uint32_t prev_size = sc_list.GetSize();
+    return;
 
   SymbolContext sc(GetModule());
   sc.comp_unit = this;
 
-  if (line != 0) {
-    LineTable *line_table = sc.comp_unit->GetLineTable();
+  if (line == 0) {
+    if (file_spec_matches_cu_file_spec && !check_inlines) {
+      // only append the context if we aren't looking for inline call sites by
+      // file and line and if the file spec matches that of the compile unit
+      sc_list.Append(sc);
+    }
+    return;
+  }
+
 
-    if (line_table != nullptr) {
-      uint32_t found_line;
-      uint32_t line_idx;
+  if (line_table == nullptr)
+    return;
+
+  LineEntry line_entry;
 
-      if (num_file_indexes == 1) {
-        // We only have a single support file that matches, so use the line
-        // table function that searches for a line entries that match a single
-        // support file index
-        LineEntry line_entry;
-        line_idx = line_table->FindLineEntryIndexByFileIndex(
-            0, file_indexes.front(), line, exact, &line_entry);
-
-        // 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
-        // subsequent line exact matches below.
-        found_line = line_entry.line;
-
-        while (line_idx != UINT32_MAX) {
-          // If they only asked for the line entry, then we're done, we can
-          // just copy that over. But if they wanted more than just the line
-          // number, fill it in.
-          if (resolve_scope == eSymbolContextLineEntry) {
-            sc.line_entry = line_entry;
-          } else {
-            line_entry.range.GetBaseAddress().CalculateSymbolContext(
-                &sc, resolve_scope);
-          }
-
-          sc_list.Append(sc);
-          line_idx = line_table->FindLineEntryIndexByFileIndex(
-              line_idx + 1, file_indexes.front(), found_line, true,
-              &line_entry);
-        }
-      } else {
-        // We found multiple support files that match "file_spec" so use the
-        // line table function that searches for a line entries that match a
-        // multiple support file indexes.
-        LineEntry line_entry;
-        line_idx = line_table->FindLineEntryIndexByFileIndex(
-            0, file_indexes, line, exact, &line_entry);
+    line_idx = line_table->FindLineEntryIndexByFileIndex(0, file_indexes, line,
+                                                         exact, &line_entry);
+  }
   
-        // 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
-        // subsequent line exact matches below.
-        found_line = line_entry.line;
+  uint32_t found_line = line_entry.line;
   
-        while (line_idx != UINT32_MAX) {
-          if (resolve_scope == eSymbolContextLineEntry) {
-            sc.line_entry = line_entry;
-          } else {
-            line_entry.range.GetBaseAddress().CalculateSymbolContext(
-                &sc, resolve_scope);
-          }
+    // If they only asked for the line entry, then we're done, we can
+    // just copy that over. But if they wanted more than just the line
+    // number, fill it in.
+      line_entry.range.GetBaseAddress().CalculateSymbolContext(&sc,
+                                                               resolve_scope);
 
-          sc_list.Append(sc);
-          line_idx = line_table->FindLineEntryIndexByFileIndex(
-              line_idx + 1, file_indexes, found_line, true, &line_entry);
-        }
-      }
-    }
-  } else if (file_spec_matches_cu_file_spec && !check_inlines) {
-    // only append the context if we aren't looking for inline call sites by
-    // file and line and if the file spec matches that of the compile unit
-    sc_list.Append(sc);
-  }
-  return sc_list.GetSize() - prev_size;
-}
+    if (num_file_indexes == 1)
+      line_idx = line_table->FindLineEntryIndexByFileIndex(
+          line_idx + 1, file_indexes.front(), found_line, true, &line_entry);
+    else
 
 bool CompileUnit::GetIsOptimized() {
   if (m_is_optimized == eLazyBoolCalculate) {
     m_is_optimized = eLazyBoolNo;
     if (SymbolFile *symfile = GetModule()->GetSymbolFile()) {
       if (symfile->ParseIsOptimized(*this))
         m_is_optimized = eLazyBoolYes;
     }