Retrieve a function PDB symbol correctly from nested blocks

[aleksandr.urakov]

Summary

This patch fixes a problem with retrieving a function symbol by an address in a nested block. In the current implementation of ResolveSymbolContext function it retrieves a symbol with PDB_SymType::None and then checks if found symbol's tag equals to PDB_SymType::Function. So, if nested block's symbol was found, ResolveSymbolContext does not resolve a function.

It is very simple to reproduce this. For example, in the next program

int main() {
  auto r = 0;
  for (auto i = 1; i <= 10; i++) {
    r += i & 1 + (i - 1) & 1 - 1;
  }

  return r;
}

if we will stop inside the cycle and will do a backtrace, the top element will be broken. But how we can test this? I thought to add an option to lldb-test to allow search a function by address, but the address may change when the compiler will be changed.

[labath]

Yea, relying on addresses inside compiler-generated binaries would be extremely brittle. However, line numbers should be fairly stable. Maybe we could have lldb-test take an file+line argument, resolve that into an address, and then look up the relevant symbol context? As a bonus we also get to test line number resolution :P.

Would that work for your needs?

[aleksandr.urakov]

In D47939#1126538, @labath wrote:

Yea, relying on addresses inside compiler-generated binaries would be extremely brittle. However, line numbers should be fairly stable. Maybe we could have lldb-test take an file+line argument, resolve that into an address, and then look up the relevant symbol context? As a bonus we also get to test line number resolution :P.

Would that work for your needs?

Yes, it's a good idea, thank you!

But lldb used DWARF-like (one based) files numeration, so resolving file+line into an address didn't work. I have fixed that too in the patch.

[asmith]

Few small comments otherwise LGTM.

tools/lldb-test/lldb-test.cpp

150

Would "Search source file for function" be a better description?

152

Would "Search source file by line number and function" be a better description?

594

Would you move the opening brace '{' to the end of the previous line and the ones following.

[labath]

Looks great. Thanks.

For the formatting, I'd recommend getting into the habit of running clang-format over your change before upload.

[aleksandr.urakov]

Hello!

Excuse me for a long delay with the reply, I had a vacation.

I have fixed mentioned problems in the patch, the only thing is that I have written "Line to search." description for the -line parameter (by analogy with the -file parameter, which Pavel has added). Do not you mind?

[labath]

This looks fine to me, but let's also wait for Aaron.

[aleksandr.urakov]

Thank you!

Please, commit this, because I have no commit access.

[stella.stamenova]

This test is actually failing on Windows with the Visual Studio Generator and the latest DIA SDK:

E:\_work\1\s\llvm\tools\lldb\lit\SymbolFile\PDB\function-nested-block.test(7,8): error G8E235623: expected string not found in input [E:\_work\1\b\LLVMBuild\tools\lldb\lit\check-lldb-lit.vcxproj]
          
          CHECK: name = "{{.*}}", mangled = "_main"
          
                 ^
          
          <stdin>:3:1: note: scanning from here
          
          0x000000e40bf8f180: SymbolContextList
          
          ^
          
          <stdin>:6:31: note: possible intended match here
          
           Function: id = {0x00000003}, name = "main", mangled = "main", range = function-nested-block.test.tmp.exe[0x0000000140001000-0x000000014000105b)

[aleksandr.urakov]

Thanks for catching that!

I have attached a diff with a fix for the test.

[stella.stamenova]

I can confirm that this fixes the test. I just ran the tests on Windows in our setup and it passed. Thanks!

[aleksandr.urakov]

Please, commit this for me, because I have no commit access.

lit/SymbolFile/PDB/Inputs/FunctionNestedBlockTest.cpp

+int main() {
+  auto r = 0;
+  for (auto i = 1; i <= 10; i++) {
+    r += i & 1 + (i - 1) & 1 - 1;
+  }
+
+  return r;
+}

lit/SymbolFile/PDB/function-nested-block.test

+REQUIRES: windows, lld
+RUN: clang-cl /c /Zi %S/Inputs/FunctionNestedBlockTest.cpp /o %t.obj
+RUN: lld-link /debug:full /nodefaultlib /entry:main %t.obj /out:%t.exe
+RUN: lldb-test symbols -find=function -file FunctionNestedBlockTest.cpp -line 4 %t.exe | FileCheck %s
+
+CHECK: Found 1 functions:
+CHECK: name = "{{.*}}", mangled = "_main"

source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

bool SymbolFilePDB::ParseCompileUnitSupportFiles(
   auto files = m_session_up->getSourceFilesForCompiland(*compiland_up);
   if (!files || files->getChildCount() == 0)
     return false;
 
   while (auto file = files->getNext()) {
     FileSpec spec(file->getFileName(), false, FileSpec::Style::windows);
     support_files.AppendIfUnique(spec);
   }
+
+  // LLDB uses the DWARF-like file numeration (one based),
+  // the zeroth file is the compile unit itself
+  support_files.Insert(0, *sc.comp_unit);
+
   return true;
 }
 
 bool SymbolFilePDB::ParseImportedModules(
     const lldb_private::SymbolContext &sc,
     std::vector<lldb_private::ConstString> &imported_modules) {
   // PDB does not yet support module debug info
   return false;
SymbolFilePDB::ResolveSymbolContext(const lldb_private::Address &so_addr,
                                     uint32_t resolve_scope,
                                     lldb_private::SymbolContext &sc) {
   uint32_t resolved_flags = 0;
   if (resolve_scope & eSymbolContextCompUnit ||
       resolve_scope & eSymbolContextVariable ||
       resolve_scope & eSymbolContextFunction ||
       resolve_scope & eSymbolContextBlock ||
       resolve_scope & eSymbolContextLineEntry) {
-    addr_t file_vm_addr = so_addr.GetFileAddress();
-    auto symbol_up =
-        m_session_up->findSymbolByAddress(file_vm_addr, PDB_SymType::None);
-    if (!symbol_up)
-      return 0;
-
     auto cu_sp = GetCompileUnitContainsAddress(so_addr);
     if (!cu_sp) {
       if (resolved_flags | eSymbolContextVariable) {
         // TODO: Resolve variables
       }
       return 0;
     }
     sc.comp_unit = cu_sp.get();
     resolved_flags |= eSymbolContextCompUnit;
     lldbassert(sc.module_sp == cu_sp->GetModule());
+  }
 
-    switch (symbol_up->getSymTag()) {
-    case PDB_SymType::Function:
-      if (resolve_scope & eSymbolContextFunction) {
-        auto *pdb_func = llvm::dyn_cast<PDBSymbolFunc>(symbol_up.get());
-        assert(pdb_func);
-        auto func_uid = pdb_func->getSymIndexId();
-        sc.function = sc.comp_unit->FindFunctionByUID(func_uid).get();
-        if (sc.function == nullptr)
-          sc.function = ParseCompileUnitFunctionForPDBFunc(*pdb_func, sc);
-        if (sc.function) {
-          resolved_flags |= eSymbolContextFunction;
-          if (resolve_scope & eSymbolContextBlock) {
-            Block &block = sc.function->GetBlock(true);
-            sc.block = block.FindBlockByID(sc.function->GetID());
-            if (sc.block)
-              resolved_flags |= eSymbolContextBlock;
-          }
-        }
-      }
-      break;
-    default:
-      break;
-    }
+    addr_t file_vm_addr = so_addr.GetFileAddress();
+    auto symbol_up =
+        m_session_up->findSymbolByAddress(file_vm_addr, PDB_SymType::Function);
+    if (symbol_up) {
 
-    if (resolve_scope & eSymbolContextLineEntry) {
-      if (auto *line_table = sc.comp_unit->GetLineTable()) {
-        Address addr(so_addr);
-        if (line_table->FindLineEntryByAddress(addr, sc.line_entry))
-          resolved_flags |= eSymbolContextLineEntry;
-      }
-    }
-  }
+
   return resolved_flags;
 }
 
 uint32_t SymbolFilePDB::ResolveSymbolContext(
     const lldb_private::FileSpec &file_spec, uint32_t line, bool check_inlines,
     uint32_t resolve_scope, lldb_private::SymbolContextList &sc_list) {
   const size_t old_size = sc_list.GetSize();
   if (resolve_scope & lldb::eSymbolContextCompUnit) {
void SymbolFilePDB::BuildSupportFileIdToSupportFileIndexMap(
   // This is a hack, but we need to convert the source id into an index into
   // the support files array.  We don't want to do path comparisons to avoid
   // basename / full path issues that may or may not even be a problem, so we
   // use the globally unique source file identifiers.  Ideally we could use the
   // global identifiers everywhere, but LineEntry currently assumes indices.
   auto source_files = m_session_up->getSourceFilesForCompiland(compiland);
   if (!source_files)
     return;
-  int index = 0;
+
+  // LLDB uses the DWARF-like file numeration (one based)
+  int index = 1;
 
   while (auto file = source_files->getNext()) {
     uint32_t source_id = file->getUniqueId();
     index_map[source_id] = index++;
   }
 }
 
 lldb::CompUnitSP SymbolFilePDB::GetCompileUnitContainsAddress(

tools/lldb-test/lldb-test.cpp

 #include "lldb/Target/Target.h"
 #include "lldb/Utility/CleanUp.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/StreamString.h"
 
 #include "llvm/ADT/IntervalMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
-#include "llvm/Support/MathExtras.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/WithColor.h"
 #include <cstdio>
 #include <thread>
 
 using namespace lldb;
enum class FindType {
   Function,
   Namespace,
   Type,
   Variable,
 };
 static cl::opt<FindType> Find(
     "find", cl::desc("Choose search type:"),
     cl::values(
-        clEnumValN(FindType::None, "none",
-                   "No search, just dump the module."),
+        clEnumValN(FindType::None, "none", "No search, just dump the module."),
         clEnumValN(FindType::Function, "function", "Find functions."),
         clEnumValN(FindType::Namespace, "namespace", "Find namespaces."),
         clEnumValN(FindType::Type, "type", "Find types."),
         clEnumValN(FindType::Variable, "variable", "Find global variables.")),
     cl::sub(SymbolsSubcommand));
 
 static cl::opt<std::string> Name("name", cl::desc("Name to find."),
                                  cl::sub(SymbolsSubcommand));
static FunctionNameType getFunctionNameFlags() {
   for (FunctionNameType Flag : FunctionNameFlags)
     Result = FunctionNameType(Result | Flag);
   return Result;
 }
 
 static cl::opt<bool> Verify("verify", cl::desc("Verify symbol information."),
                             cl::sub(SymbolsSubcommand));
 
 static cl::opt<std::string> File("file",
                                  cl::desc("File (compile unit) to search."),
                                  cl::sub(SymbolsSubcommand));
+static cl::opt<int> Line("line", cl::desc("Line to search."),
+                         cl::sub(SymbolsSubcommand));
 
 static Expected<CompilerDeclContext> getDeclContext(SymbolVendor &Vendor);
 
 static Error findFunctions(lldb_private::Module &Module);
 static Error findNamespaces(lldb_private::Module &Module);
 static Error findTypes(lldb_private::Module &Module);
 static Error findVariables(lldb_private::Module &Module);
 static Error dumpModule(lldb_private::Module &Module);
 static Error verify(lldb_private::Module &Module);
 
 static Expected<Error (*)(lldb_private::Module &)> getAction();
 static int dumpSymbols(Debugger &Dbg);
-}
+} // namespace symbols
 
 namespace irmemorymap {
 static cl::opt<std::string> Target(cl::Positional, cl::desc("<target>"),
                                    cl::Required,
                                    cl::sub(IRMemoryMapSubcommand));
 static cl::opt<std::string> CommandFile(cl::Positional,
                                         cl::desc("<command-file>"),
                                         cl::init("-"),
                                         cl::sub(IRMemoryMapSubcommand));
 static cl::opt<bool> UseHostOnlyAllocationPolicy(
     "host-only", cl::desc("Use the host-only allocation policy"),
     cl::init(false), cl::sub(IRMemoryMapSubcommand));
 
 using AllocationT = std::pair<addr_t, addr_t>;
 using AddrIntervalMap =
-      IntervalMap<addr_t, unsigned, 8, IntervalMapHalfOpenInfo<addr_t>>;
 
 struct IRMemoryMapTestState {
   TargetSP Target;
   IRMemoryMap Map;
 
   AddrIntervalMap::Allocator IntervalMapAllocator;
   AddrIntervalMap Allocations;
 
opts::symbols::getDeclContext(SymbolVendor &Vendor) {
   if (List.GetSize() > 1)
     return make_string_error("Context search found multiple matches.");
   return List.GetVariableAtIndex(0)->GetDeclContext();
 }
 
 Error opts::symbols::findFunctions(lldb_private::Module &Module) {
   SymbolVendor &Vendor = *Module.GetSymbolVendor();
   SymbolContextList List;
-  if (Regex) {
+  if (!File.empty()) {
+    assert(Line != 0);
+
+    FileSpec src_file(File, false);
+    size_t cu_count = Module.GetNumCompileUnits();
+    for (size_t i = 0; i < cu_count; i++) {
+      lldb::CompUnitSP cu_sp = Module.GetCompileUnitAtIndex(i);
+      if (!cu_sp)
+        continue;
+
+      LineEntry le;
+      cu_sp->FindLineEntry(0, Line, &src_file, false, &le);
+      if (!le.IsValid())
+        continue;
+
+      auto addr = le.GetSameLineContiguousAddressRange().GetBaseAddress();
+      if (!addr.IsValid())
+        continue;
+
+      SymbolContext sc;
+      uint32_t resolved =
+          addr.CalculateSymbolContext(&sc, eSymbolContextFunction);
+      if (resolved & eSymbolContextFunction)
+        List.Append(sc);
+    }
+  } else if (Regex) {
     RegularExpression RE(Name);
     assert(RE.IsValid());
     Vendor.FindFunctions(RE, true, false, List);
   } else {
     Expected<CompilerDeclContext> ContextOr = getDeclContext(Vendor);
     if (!ContextOr)
       return ContextOr.takeError();
     CompilerDeclContext *ContextPtr =
         ContextOr->IsValid() ? &*ContextOr : nullptr;
 
     Vendor.FindFunctions(ConstString(Name), ContextPtr, getFunctionNameFlags(),
-                          true, false, List);
   }
   outs() << formatv("Found {0} functions:\n", List.GetSize());
   StreamString Stream;
   List.Dump(&Stream, nullptr);
   outs() << Stream.GetData() << "\n";
   return Error::success();
 }
 
if (!ContextOr)
     return ContextOr.takeError();
   CompilerDeclContext *ContextPtr =
       ContextOr->IsValid() ? &*ContextOr : nullptr;
 
   SymbolContext SC;
   DenseSet<SymbolFile *> SearchedFiles;
   TypeMap Map;
   Vendor.FindTypes(SC, ConstString(Name), ContextPtr, true, UINT32_MAX,
-                    SearchedFiles, Map);
 
   outs() << formatv("Found {0} types:\n", Map.GetSize());
   StreamString Stream;
   Map.Dump(&Stream, false);
   outs() << Stream.GetData() << "\n";
   return Error::success();
 }
 
Error opts::symbols::verify(lldb_private::Module &Module) {
 
   outs() << "Found " << comp_units_count << " compile units.\n";
 
   for (uint32_t i = 0; i < comp_units_count; i++) {
     lldb::CompUnitSP comp_unit = symfile->ParseCompileUnitAtIndex(i);
     if (!comp_unit)
       return make_string_error("Connot parse compile unit {0}.", i);
 
-    outs() << "Processing '" << comp_unit->GetFilename().AsCString() <<
-      "' compile unit.\n";
+    outs() << "Processing '" << comp_unit->GetFilename().AsCString()
+           << "' compile unit.\n";
 
     LineTable *lt = comp_unit->GetLineTable();
     if (!lt)
       return make_string_error("Can't get a line table of a compile unit.");
 
     uint32_t count = lt->GetSize();
 
     outs() << "The line table contains " << count << " entries.\n";
 
     if (count == 0)
       continue;
 
     LineEntry le;
     if (!lt->GetLineEntryAtIndex(0, le))
       return make_string_error("Can't get a line entry of a compile unit.");
 
     for (uint32_t i = 1; i < count; i++) {
       lldb::addr_t curr_end =
           le.range.GetBaseAddress().GetFileAddress() + le.range.GetByteSize();
 
       if (!lt->GetLineEntryAtIndex(i, le))
         return make_string_error("Can't get a line entry of a compile unit");
 
       if (curr_end > le.range.GetBaseAddress().GetFileAddress())
         return make_string_error(
             "Line table of a compile unit is inconsistent.");
     }
   }
 
   outs() << "The symbol information is verified.\n";
 
   return Error::success();
 }
 
 Expected<Error (*)(lldb_private::Module &)> opts::symbols::getAction() {
   if (Verify) {
     if (Find != FindType::None)
       return make_string_error(
           "Cannot both search and verify symbol information.");
-    if (Regex || !Context.empty() || !Name.empty() || !File.empty())
-      return make_string_error("-regex, -context, -name and -file options are not "
-                               "applicable for symbol verification.");
+    if (Regex || !Context.empty() || !Name.empty() || !File.empty() ||
+        Line != 0)
+      return make_string_error(
+          "-regex, -context, -name, -file and -line options are not "
     return verify;
   }
 
   if (Regex && !Context.empty())
     return make_string_error(
         "Cannot search using both regular expressions and context.");
 
   if (Regex && !RegularExpression(Name).IsValid())
     return make_string_error("`{0}` is not a valid regular expression.", Name);
 
   if (Regex + !Context.empty() + !File.empty() >= 2)
     return make_string_error(
         "Only one of -regex, -context and -file may be used simultaneously.");
   if (Regex && Name.empty())
     return make_string_error("-regex used without a -name");
 
   switch (Find) {
   case FindType::None:
-    if (!Context.empty() || !Name.empty() || !File.empty())
+    if (!Context.empty() || !Name.empty() || !File.empty() || Line != 0)
       return make_string_error(
           "Specify search type (-find) to use search options.");
     return dumpModule;
 
   case FindType::Function:
-    if (!File.empty())
-      return make_string_error("Cannot search for functions by file name.");
-    if (Regex && getFunctionNameFlags() != 0)
-      return make_string_error("Cannot search for functions using both regular "
-                               "expressions and function-flags.");
+    if (!File.empty() + (Line != 0) == 1)
+      return make_string_error("Both file name and line number must be "
+                               "specified when searching a function "
+                               "by file position.");
+    if (Regex + (getFunctionNameFlags() != 0) + !File.empty() >= 2)
+      return make_string_error("Only one of regular expression, function-flags "
+                               "and file position may be used simultaneously "
+                               "when searching a function.");
     return findFunctions;
 
   case FindType::Namespace:
-    if (Regex || !File.empty())
+    if (Regex || !File.empty() || Line != 0)
       return make_string_error("Cannot search for namespaces using regular "
-                               "expressions or file names.");
+                               "expressions, file names or line numbers.");
     return findNamespaces;
 
   case FindType::Type:
-    if (Regex || !File.empty())
+    if (Regex || !File.empty() || Line != 0)
       return make_string_error("Cannot search for types using regular "
-                               "expressions or file names.");
+                               "expressions, file names or line numbers.");
     return findTypes;
 
   case FindType::Variable:
+    if (Line != 0)
+      return make_string_error("Cannot search for variables "
+                               "using line numbers.");
     return findVariables;
   }
 }
 
 int opts::symbols::dumpSymbols(Debugger &Dbg) {
   auto ActionOr = getAction();
   if (!ActionOr) {
     logAllUnhandledErrors(ActionOr.takeError(), WithColor::error(), "");
     return 1;
   }
   auto Action = *ActionOr;
 
   int HadErrors = 0;
   for (const auto &File : InputFilenames) {
     outs() << "Module: " << File << "\n";
     ModuleSpec Spec{FileSpec(File, false)};
     Spec.GetSymbolFileSpec().SetFile(File, false, FileSpec::Style::native);
 
     auto ModulePtr = std::make_shared<lldb_private::Module>(Spec);
     SymbolVendor *Vendor = ModulePtr->GetSymbolVendor();