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

151

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

153

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

541

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

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("Function search source file."),
+                                 cl::sub(SymbolsSubcommand));
+static cl::opt<int> Line("line", cl::desc("Function search line number."),
+                         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);
return make_error<StringError>("Context search found multiple matches.",
                                    inconvertibleErrorCode());
   }
   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;
+      uint32_t le_idx = 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 =
WithColor::error() << "Cannot search for types using both regular "
                           "expressions and function-flags.\n";
     return 1;
   }
   if (Regex && !RegularExpression(Name).IsValid()) {
     WithColor::error() << "`" << Name
                        << "` is not a valid regular expression.\n";
     return 1;
   }
+  if (!File.empty() && Line == 0 || File.empty() && Line != 0)
+  {
+    WithColor::error() << "Both the file and the line must be specified when "
+                          "functions are searched with a source location.\n";
+    return 1;
+  }
+  if (!File.empty() && Find != FindType::Function)
+  {
+    WithColor::error() << "Only functions can be searched "
+                          "with a source location.\n";
+    return 1;
+  }
+  if (!File.empty() && !Name.empty())
+  {
+    WithColor::error() << "Cannot search for functions using both "
+                          "a source location and a name.\n";
+    return 1;
+  }
 
   Error (*Action)(lldb_private::Module &);
   if (!Verify) {
     switch (Find) {
     case FindType::Function:
       Action = findFunctions;
       break;
     case FindType::Namespace: