[lldb] Refactor IRExecutionUnit::FindInSymbols (NFC)

[JDevlieghere]

Summary

As I was trying to understand what IRExecutionUnit::FindInSymbols was doing, I couldn't help myself but refactor the code a little bit:

• No else after return.
• Move the lambda out of the loop.
• Replace GetSize() == 0 with IsEmpty()
• Eliminate pitfall of not clearing the SymbolContextList by moving it into the lexical scope of the if-clause.

And probably some other small things that bothered me.

[bulbazord]

Yeah this is a lot cleaner. Thanks for taking the time to refactor this.

[shafik]

Thank you for refactoring this, besides my comment on the lambda the rest makes sense.

lldb/source/Expression/IRExecutionUnit.cpp

782

It feels like the lambda should really be a small class that tracks the state of load_address and best_internal_load_address over multiple calls and then the user can access the state.

The lambda is doing a lot of logic and there is state being carried across calls and that feels like we have crossed the boundary and a class would be better.

[JDevlieghere]

Convert lambda into a stateful class.

[JDevlieghere]

lldb/source/Expression/IRExecutionUnit.cpp

782

Thanks, I really like that idea. Updated the diff accordingly.

[shafik]

LGTM

lldb/source/Expression/IRExecutionUnit.cpp

880

Normally I wouldn't be crazy about a reference member but since this is a localized class I think this is an ok compromise.

I guess the real question would be why IRExecutionUnit::FindInSymbols is returning a value that way and perhaps it should be returning a struct w/ addr_t and bool but that would be a different change.

lldb/source/Expression/IRExecutionUnit.cpp

if (!lparen_loc)
       continue;
 
     llvm::StringRef base_name(demangled_cstr,
                               lparen_loc - demangled_cstr);
     fallback_specs.push_back(ConstString(base_name));
   }
 }
 
-lldb::addr_t IRExecutionUnit::FindInSymbols(
-    const std::vector<IRExecutionUnit::SearchSpec> &specs,
-    const lldb_private::SymbolContext &sc,
-    bool &symbol_was_missing_weak) {
-  symbol_was_missing_weak = false;
-  Target *target = sc.target_sp.get();
-
-  if (!target) {
-    // we shouldn't be doing any symbol lookup at all without a target
-    return LLDB_INVALID_ADDRESS;
-  }
-
-  for (const SearchSpec &spec : specs) {
-    SymbolContextList sc_list;
-
-    lldb::addr_t best_internal_load_address = LLDB_INVALID_ADDRESS;
+class LoadAddressResolver {
+public:
+  LoadAddressResolver(Target *target, bool &symbol_was_missing_weak)
+      : m_target(target), m_symbol_was_missing_weak(symbol_was_missing_weak) {}
 
-    std::function<bool(lldb::addr_t &, SymbolContextList &,
-                       const lldb_private::SymbolContext &)>
-        get_external_load_address = [&best_internal_load_address, target,
-                                     &symbol_was_missing_weak](
-            lldb::addr_t &load_address, SymbolContextList &sc_list,
-            const lldb_private::SymbolContext &sc) -> lldb::addr_t {
-      load_address = LLDB_INVALID_ADDRESS;
+  llvm::Optional<lldb::addr_t> Resolve(SymbolContextList &sc_list) {
+    if (sc_list.IsEmpty())
+      return llvm::None;
 
-      if (sc_list.GetSize() == 0)
-        return false;
+    lldb::addr_t load_address = LLDB_INVALID_ADDRESS;
 
-      // missing_weak_symbol will be true only if we found only weak undefined 
-      // references to this symbol.
-      symbol_was_missing_weak = true;      
-      for (auto candidate_sc : sc_list.SymbolContexts()) {        
-        // Only symbols can be weak undefined:
-        if (!candidate_sc.symbol)
-          symbol_was_missing_weak = false;
-        else if (candidate_sc.symbol->GetType() != lldb::eSymbolTypeUndefined
-                  || !candidate_sc.symbol->IsWeak())
-          symbol_was_missing_weak = false;
-        
-        const bool is_external =
-            (candidate_sc.function) ||
-            (candidate_sc.symbol && candidate_sc.symbol->IsExternal());
-        if (candidate_sc.symbol) {
-          load_address = candidate_sc.symbol->ResolveCallableAddress(*target);
-
-          if (load_address == LLDB_INVALID_ADDRESS) {
-            if (target->GetProcessSP())
-              load_address =
-                  candidate_sc.symbol->GetAddress().GetLoadAddress(target);
-            else
-              load_address = candidate_sc.symbol->GetAddress().GetFileAddress();
-          }
-        }
+    // Missing_weak_symbol will be true only if we found only weak undefined
+    m_symbol_was_missing_weak = true;
+
+    for (auto candidate_sc : sc_list.SymbolContexts()) {
+      // Only symbols can be weak undefined.
+      if (!candidate_sc.symbol ||
+          candidate_sc.symbol->GetType() != lldb::eSymbolTypeUndefined ||
+          !candidate_sc.symbol->IsWeak())
+        m_symbol_was_missing_weak = false;
+      // First try the symbol.
+        load_address = candidate_sc.symbol->ResolveCallableAddress(*m_target);
+          Address addr = candidate_sc.symbol->GetAddress();
+          load_address = m_target->GetProcessSP()
+                             ? addr.GetLoadAddress(m_target)
+                             : addr.GetFileAddress();
 
-        if (load_address == LLDB_INVALID_ADDRESS && candidate_sc.function) {
-          if (target->GetProcessSP())
-            load_address = candidate_sc.function->GetAddressRange()
-                               .GetBaseAddress()
-                               .GetLoadAddress(target);
-          else
-            load_address = candidate_sc.function->GetAddressRange()
-                               .GetBaseAddress()
-                               .GetFileAddress();
-        }
+      // If that didn't work, try the function.
+        Address addr =
+            candidate_sc.function->GetAddressRange().GetBaseAddress();
+        load_address = m_target->GetProcessSP() ? addr.GetLoadAddress(m_target)
+                                                : addr.GetFileAddress();
 
-        if (load_address != LLDB_INVALID_ADDRESS) {
-          if (is_external) {
-            return true;
-          } else if (best_internal_load_address == LLDB_INVALID_ADDRESS) {
-            best_internal_load_address = load_address;
-            load_address = LLDB_INVALID_ADDRESS;
-          }
-        }
-      }
+      // We found a load address.
+        // If the load address is external, we're done.
+        const bool is_external =
+            (candidate_sc.function) ||
+            (candidate_sc.symbol && candidate_sc.symbol->IsExternal());
+        if (is_external)
+          return load_address;
+
+        // Otherwise, remember the best internal load address.
+        if (m_best_internal_load_address == LLDB_INVALID_ADDRESS)
+          m_best_internal_load_address = load_address;
 
-      // You test the address of a weak symbol against NULL to see if it is
-      // present.  So we should return 0 for a missing weak symbol.
-      if (symbol_was_missing_weak) {
-        load_address = 0;
-        return true;
-      }
-      
-      return false;
-    };
+    // present. So we should return 0 for a missing weak symbol.
+    if (m_symbol_was_missing_weak)
+      return 0;
+
+    return llvm::None;
+  lldb::addr_t GetBestInternalLoadAddress() const {
+    return m_best_internal_load_address;
+  }
+
+private:
+  Target *m_target;
+  bool &m_symbol_was_missing_weak;
+  lldb::addr_t m_best_internal_load_address = LLDB_INVALID_ADDRESS;
 
-    ModuleFunctionSearchOptions function_options;
-    function_options.include_symbols = true;
-    function_options.include_inlines = false;
+lldb::addr_t IRExecutionUnit::FindInSymbols(
+    const std::vector<IRExecutionUnit::SearchSpec> &specs,
+    const lldb_private::SymbolContext &sc, bool &symbol_was_missing_weak) {
+  symbol_was_missing_weak = false;
+
+  Target *target = sc.target_sp.get();
+  if (!target) {
+    // We shouldn't be doing any symbol lookup at all without a target.
+    return LLDB_INVALID_ADDRESS;
+  }
+
+  LoadAddressResolver resolver(target, symbol_was_missing_weak);
+
 
+  for (const SearchSpec &spec : specs) {
     if (sc.module_sp) {
+      SymbolContextList sc_list;
       sc.module_sp->FindFunctions(spec.name, CompilerDeclContext(), spec.mask,
                                   function_options, sc_list);
+      if (auto load_addr = resolver.Resolve(sc_list))
+        return *load_addr;
     }
 
-    lldb::addr_t load_address = LLDB_INVALID_ADDRESS;
-
-    if (get_external_load_address(load_address, sc_list, sc)) {
-      return load_address;
-    } else {
-      sc_list.Clear();
-    }
-
-    if (sc_list.GetSize() == 0 && sc.target_sp) {
+    if (sc.target_sp) {
+      SymbolContextList sc_list;
       sc.target_sp->GetImages().FindFunctions(spec.name, spec.mask,
                                               function_options, sc_list);
+      if (auto load_addr = resolver.Resolve(sc_list))
+        return *load_addr;
     }
 
-    if (get_external_load_address(load_address, sc_list, sc)) {
-      return load_address;
-    } else {
-      sc_list.Clear();
-    }
-
-    if (sc_list.GetSize() == 0 && sc.target_sp) {
+    if (sc.target_sp) {
+      SymbolContextList sc_list;
       sc.target_sp->GetImages().FindSymbolsWithNameAndType(
           spec.name, lldb::eSymbolTypeAny, sc_list);
+      if (auto load_addr = resolver.Resolve(sc_list))
+        return *load_addr;
     }
 
-    if (get_external_load_address(load_address, sc_list, sc)) {
-      return load_address;
-    }
-    // if there are any searches we try after this, add an sc_list.Clear() in
-    // an "else" clause here
-
-    if (best_internal_load_address != LLDB_INVALID_ADDRESS) {
+    lldb::addr_t best_internal_load_address =
+        resolver.GetBestInternalLoadAddress();
+    if (best_internal_load_address != LLDB_INVALID_ADDRESS)
       return best_internal_load_address;
-    }
-  }
 
   return LLDB_INVALID_ADDRESS;
 }
 
 lldb::addr_t
 IRExecutionUnit::FindInRuntimes(const std::vector<SearchSpec> &specs,
                                 const lldb_private::SymbolContext &sc) {
   lldb::TargetSP target_sp = sc.target_sp;