[PDB] Support PDB-backed expressions evaluation

[aleksandr.urakov]

Summary

This patch contains several small fixes, which makes it possible to evaluate expressions on Windows using information from PDB. The changes are:

• several sanitize checks;
• make IRExecutionUnit::MemoryManager::getSymbolAddress to not return a magic value on a failure, because callers wait 0 in this case;
• entry point required to be a file address, not RVA, in the ObjectFilePECOFF;
• do not crash on a debuggee second chance exception - it may be an expression evaluation crash;
• create parameter declarations for functions in AST to make it possible to call debugee functions from expressions;
• relax name searching rules for variables, functions, namespaces and types. Now it works just like in the DWARF plugin;
• fix endless recursion in SymbolFilePDB::ParseCompileUnitFunctionForPDBFunc.

Each change is small and it is hard to test each change separately, so I think that create one review for them all is not a bad idea, especially because they make together the test to work.

I remember about the new native PDB plugin, but these changes are old enough, for last two or three weeks I'm just releasing my stash :) And some of the changes may be useful for the new plugin too.

This review depends on D52461, D52618, D53368, x64 testing depends on D53753.

[Hui]

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

819 ↗

(On Diff #171284)

This still needs to be the offset into the section.

[stella.stamenova]

A couple of comments, but looks good otherwise. I'd wait for someone else to have a look as well.

source/Plugins/Process/Windows/Common/ProcessWindows.cpp

959 ↗

(On Diff #171284)

Are we going to determine later whether it is supposed to be a crash or just never crash on second chance exceptions?

source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

1085 ↗

(On Diff #171284)

Nit: Maybe add curly braces here. Since the if statement has them, the code is easier to read if the else does as well.

[aleksandr.urakov]

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

819 ↗

(On Diff #171284)

If we are here, then we can't retrieve a section list, then we can't specify a section offset here. Setting an offset without setting a section means that offset must be treated as a file address (see e.g. the implementation of the Address::GetFileAddress() function). I think that misleading here is the name of the variable offset, I'll fix it, thanks.

source/Plugins/Process/Windows/Common/ProcessWindows.cpp

959 ↗

(On Diff #171284)

Yes, it's a good question... I can't understand how to figure out at this point if the exception was thrown during an expression evaluation or a debuggee execution. On other hand, it seems that Visual Studio also doesn't crash an application in such situations:

int main() {
  char *buf = nullptr;
  buf[0] = 0; // Unhandled exception is here, but you can safely continue an execution if you will drag an arrow on the next line in Visual Studio
  int x = 5;
  return x - 5;
}

Also it was the only place where eStateCrashed was set, so it seems that on other platforms we do not crash a debuggee in such situations too. So may be it's not a bad idea to not set the crashed state here at all. But if someone don't agree with me and has an idea how to do it better, I'm ready to fix it.

source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

1085 ↗

(On Diff #171284)

Sure, thanks!

[aleksandr.urakov]

Update the diff according to comments.

[aleksandr.urakov]

Ping!

Can someone to have a look as well?

[asmith]

LGTM

[aleksandr.urakov]

Thanks!

[labath]

source/Core/RichManglingContext.cpp

134–135 ↗

(On Diff #171489)

Why is this necessary? It looks like somebody is misusing the returned StringRef by assuming that it always points to at least a single valid byte (which is definitely not the case in general, even for StringRefs with a non-null data()).

It would be better to fix the caller instead.

[aleksandr.urakov]

source/Core/RichManglingContext.cpp

134–135 ↗

(On Diff #171489)

Yes, you are right. This change was made much time ago, I've fixed in this way the same problem, which Aaron have fixed in D52626, and have forgot to remove this. Thanks for pointing to this!

[stella.stamenova]

It looks like after this change went in, the tests on Windows starting timing out instead of completing. The last run before the change took about 2 minutes and starting with the first run with this change, they're running for 40 minutes before being killed:

After:
http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1774

Before:
http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1773

Can you fix this today or pull the change out?

lldb/trunk/lit/SymbolFile/PDB/Inputs/ExpressionsTest.cpp

+namespace N0 {
+namespace N1 {
+
+char *buf0 = nullptr;
+char buf1[] = {0, 1, 2, 3, 4, 5, 6, 7};
+
+char sum(char *buf, int size) {
+  char result = 0;
+  for (int i = 0; i < size; i++)
+    result += buf[i];
+  return result;
+}
+
+} // namespace N1
+} // namespace N0
+
+int main() {
+  char result = N0::N1::sum(N0::N1::buf1, sizeof(N0::N1::buf1));
+  return 0;
+}

lldb/trunk/lit/SymbolFile/PDB/Inputs/ExpressionsTest0.script

+breakpoint set --file ExpressionsTest.cpp --line 19
+run
+print result
+print N0::N1::sum(N0::N1::buf1, sizeof(N0::N1::buf1))
+print N1::sum(N1::buf1, sizeof(N1::buf1))
+print sum(buf1, sizeof(buf1))
+print sum(buf1, 1000000000)

lldb/trunk/lit/SymbolFile/PDB/Inputs/ExpressionsTest1.script

+print sum(buf0, 1)

lldb/trunk/lit/SymbolFile/PDB/Inputs/ExpressionsTest2.script

+print sum(buf0, result - 28)
+print sum(buf1 + 3, 3)

lldb/trunk/lit/SymbolFile/PDB/expressions.test

+REQUIRES: system-windows, msvc
+RUN: %msvc_cl /Zi /GS- /c %S/Inputs/ExpressionsTest.cpp /Fo%t.obj
+RUN: %msvc_link /debug:full /nodefaultlib /entry:main %t.obj /out:%t.exe
+RUN: %lldb -b -s %S/Inputs/ExpressionsTest0.script -s %S/Inputs/ExpressionsTest1.script -s %S/Inputs/ExpressionsTest2.script -- %t.exe 2>&1 | FileCheck %s
+
+// Check the variable value through `print`
+CHECK: (lldb) print result
+CHECK: (char) $0 = '\x1c'
+
+// Call the function just like in the code
+CHECK: (lldb) print N0::N1::sum(N0::N1::buf1, sizeof(N0::N1::buf1))
+CHECK: (char) $1 = '\x1c'
+
+// Try the relaxed namespaces search
+CHECK: (lldb) print N1::sum(N1::buf1, sizeof(N1::buf1))
+CHECK: (char) $2 = '\x1c'
+
+// Try the relaxed variables and functions search
+CHECK: (lldb) print sum(buf1, sizeof(buf1))
+CHECK: (char) $3 = '\x1c'
+
+// Make a crash during expression calculation
+CHECK: (lldb) print sum(buf1, 1000000000)
+CHECK: The process has been returned to the state before expression evaluation.
+
+// Make one more crash
+CHECK: (lldb) print sum(buf0, 1)
+CHECK: The process has been returned to the state before expression evaluation.
+
+// Check if the process state was restored succesfully
+CHECK: (lldb) print sum(buf0, result - 28)
+CHECK: (char) $4 = '\0'
+
+// Call the function with arbitrary parameters
+CHECK: (lldb) print sum(buf1 + 3, 3)
+CHECK: (char) $5 = '\f'

lldb/trunk/source/Expression/IRExecutionUnit.cpp

IRExecutionUnit::MemoryManager::getSymbolAddress(const std::string &Name) {
 
   if (ret == LLDB_INVALID_ADDRESS) {
     if (log)
       log->Printf(
           "IRExecutionUnit::getSymbolAddress(Name=\"%s\") = <not found>",
           Name.c_str());
 
     m_parent.ReportSymbolLookupError(name_cs);
-    return 0xbad0bad0;
+    return 0;
   } else {
     if (log)
       log->Printf("IRExecutionUnit::getSymbolAddress(Name=\"%s\") = %" PRIx64,
                   Name.c_str(), ret);
     return ret;
   }
 }
 

lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

 lldb_private::Address ObjectFilePECOFF::GetEntryPointAddress() {
   if (m_entry_point_address.IsValid())
     return m_entry_point_address;
 
   if (!ParseHeader() || !IsExecutable())
     return m_entry_point_address;
 
   SectionList *section_list = GetSectionList();
-  addr_t offset = m_coff_header_opt.entry;
+  addr_t file_addr = m_coff_header_opt.entry + m_coff_header_opt.image_base;
 
   if (!section_list)
-    m_entry_point_address.SetOffset(offset);
+    m_entry_point_address.SetOffset(file_addr);
   else
-    m_entry_point_address.ResolveAddressUsingFileSections(offset, section_list);
+    m_entry_point_address.ResolveAddressUsingFileSections(file_addr, section_list);
   return m_entry_point_address;
 }
 
 //----------------------------------------------------------------------
 // Dump
 //
 // Dump the specifics of the runtime file container (such as any headers
 // segments, sections, etc).

lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp

if (!m_session_data) {
     LLDB_LOG(log,
              "Debugger thread reported exception {0:x} at address {1:x}, "
              "but there is no session.",
              record.GetExceptionCode(), record.GetExceptionAddress());
     return ExceptionResult::SendToApplication;
   }
 
   if (!first_chance) {
-    // Any second chance exception is an application crash by definition.
-    SetPrivateState(eStateCrashed);
+    // Not any second chance exception is an application crash by definition.
+    // It may be an expression evaluation crash.
+    SetPrivateState(eStateStopped);
   }
 
   ExceptionResult result = ExceptionResult::SendToApplication;
   switch (record.GetExceptionCode()) {
   case EXCEPTION_BREAKPOINT:
     // Handle breakpoints at the first chance.
     result = ExceptionResult::BreakInDebugger;
 

lldb/trunk/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp

 void RegisterContextWindows::InvalidateAllRegisters() {
   m_context_stale = true;
 }
 
 bool RegisterContextWindows::ReadAllRegisterValues(
     lldb::DataBufferSP &data_sp) {
   if (!CacheAllRegisterValues())
     return false;
-  if (data_sp->GetByteSize() < sizeof(m_context)) {
+  if (!data_sp || data_sp->GetByteSize() < sizeof(m_context)) {
     data_sp.reset(new DataBufferHeap(sizeof(CONTEXT), 0));
   }
   memcpy(data_sp->GetBytes(), &m_context, sizeof(m_context));
   return true;
 }
 
 bool RegisterContextWindows::WriteAllRegisterValues(
     const lldb::DataBufferSP &data_sp) {

lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.h

public:
   lldb_private::ClangASTImporter &GetClangASTImporter() {
     return m_ast_importer;
   }
 
 private:
   typedef llvm::DenseMap<clang::CXXRecordDecl *, lldb::user_id_t>
       CXXRecordDeclToUidMap;
   typedef llvm::DenseMap<lldb::user_id_t, clang::Decl *> UidToDeclMap;
-  typedef llvm::DenseMap<clang::DeclContext *, std::set<clang::NamespaceDecl *>>
+  typedef std::set<clang::NamespaceDecl *> NamespacesSet;
+  typedef llvm::DenseMap<clang::DeclContext *, NamespacesSet>
       ParentToNamespacesMap;
   typedef llvm::DenseMap<clang::DeclContext *, lldb::user_id_t>
       DeclContextToUidMap;
   typedef llvm::pdb::ConcreteSymbolEnumerator<llvm::pdb::PDBSymbolData>
       PDBDataSymbolEnumerator;
   typedef llvm::pdb::ConcreteSymbolEnumerator<llvm::pdb::PDBSymbolTypeBaseClass>
       PDBBaseClassSymbolEnumerator;
   typedef llvm::pdb::ConcreteSymbolEnumerator<llvm::pdb::PDBSymbolFunc>
AddRecordMethod(lldb_private::SymbolFile &symbol_file,
                   const llvm::pdb::PDBSymbolFunc &method) const;
 
   lldb_private::ClangASTContext &m_ast;
   lldb_private::ClangASTImporter m_ast_importer;
 
   CXXRecordDeclToUidMap m_forward_decl_to_uid;
   UidToDeclMap m_uid_to_decl;
   ParentToNamespacesMap m_parent_to_namespaces;
+  NamespacesSet m_namespaces;
   DeclContextToUidMap m_decl_context_to_uid;
 };
 
 #endif // LLDB_PLUGINS_SYMBOLFILE_PDB_PDBASTPARSER_H

lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

case PDB_SymType::Function: {
 
     auto storage = func->isStatic() ? clang::StorageClass::SC_Static
                                     : clang::StorageClass::SC_None;
 
     auto decl = m_ast.CreateFunctionDeclaration(
         decl_context, name.c_str(), type->GetForwardCompilerType(), storage,
         func->hasInlineAttribute());
 
+    std::vector<clang::ParmVarDecl *> params;
+    if (std::unique_ptr<PDBSymbolTypeFunctionSig> sig = func->getSignature()) {
+      if (std::unique_ptr<ConcreteSymbolEnumerator<PDBSymbolTypeFunctionArg>>
+              arg_enum = sig->findAllChildren<PDBSymbolTypeFunctionArg>()) {
+        while (std::unique_ptr<PDBSymbolTypeFunctionArg> arg =
+                   arg_enum->getNext()) {
+          Type *arg_type = symbol_file->ResolveTypeUID(arg->getTypeId());
+          if (!arg_type)
+            continue;
+
+          clang::ParmVarDecl *param = m_ast.CreateParameterDeclaration(
+              nullptr, arg_type->GetForwardCompilerType(), clang::SC_None);
+          if (param)
+            params.push_back(param);
+        }
+      }
+    }
+    if (params.size())
+      m_ast.SetFunctionParameters(decl, params.data(), params.size());
+
     m_uid_to_decl[sym_id] = decl;
 
     return decl;
   }
   default: {
     // It's not a variable and not a function, check if it's a type
     Type *type = symbol_file->ResolveTypeUID(sym_id);
     if (!type)
if (!has_type_or_function_parent) {
       const char *namespace_name_c_str =
           IsAnonymousNamespaceName(namespace_name) ? nullptr
                                                    : namespace_name.data();
       clang::NamespaceDecl *namespace_decl =
           m_ast.GetUniqueNamespaceDeclaration(namespace_name_c_str,
                                               curr_context);
 
       m_parent_to_namespaces[curr_context].insert(namespace_decl);
+      m_namespaces.insert(namespace_decl);
 
       curr_context = namespace_decl;
     }
   }
 
   return curr_context;
 }
 
void PDBASTParser::ParseDeclsForDeclContext(
   if (auto children = symbol->findAllChildren())
     while (auto child = children->getNext())
       GetDeclForSymbol(*child);
 }
 
 clang::NamespaceDecl *
 PDBASTParser::FindNamespaceDecl(const clang::DeclContext *parent,
                                 llvm::StringRef name) {
-  if (!parent)
-    parent = m_ast.GetTranslationUnitDecl();
-
-  auto it = m_parent_to_namespaces.find(parent);
-  if (it == m_parent_to_namespaces.end())
+  NamespacesSet *set;
+  if (parent) {
+    auto pit = m_parent_to_namespaces.find(parent);
+    if (pit == m_parent_to_namespaces.end())
       return nullptr;
 
-  for (auto namespace_decl : it->second)
+    set = &pit->second;
+  } else {
+    set = &m_namespaces;
+  }
+  assert(set);
+
+  for (clang::NamespaceDecl *namespace_decl : *set)
     if (namespace_decl->getName().equals(name))
       return namespace_decl;
 
-  for (auto namespace_decl : it->second)
+  for (clang::NamespaceDecl *namespace_decl : *set)
     if (namespace_decl->isAnonymousNamespace())
       return FindNamespaceDecl(namespace_decl, name);
 
   return nullptr;
 }
 
 bool PDBASTParser::AddEnumValue(CompilerType enum_type,
                                 const PDBSymbolData &enum_value) {

lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

if (!details)
     return lldb::eLanguageTypeUnknown;
   return TranslateLanguage(details->getLanguage());
 }
 
 lldb_private::Function *SymbolFilePDB::ParseCompileUnitFunctionForPDBFunc(
     const PDBSymbolFunc &pdb_func, const lldb_private::SymbolContext &sc) {
   lldbassert(sc.comp_unit && sc.module_sp.get());
 
+  if (FunctionSP result =
+          sc.comp_unit->FindFunctionByUID(pdb_func.getSymIndexId()))
+    return result.get();
+
   auto file_vm_addr = pdb_func.getVirtualAddress();
   if (file_vm_addr == LLDB_INVALID_ADDRESS || file_vm_addr == 0)
     return nullptr;
 
   auto func_length = pdb_func.getLength();
   AddressRange func_range =
       AddressRange(file_vm_addr, func_length, sc.module_sp->GetSectionList());
   if (!func_range.GetBaseAddress().IsValid())
SymbolFilePDB::ParseVariables(const lldb_private::SymbolContext &sc,
 
   return num_added;
 }
 
 uint32_t SymbolFilePDB::FindGlobalVariables(
     const lldb_private::ConstString &name,
     const lldb_private::CompilerDeclContext *parent_decl_ctx,
     uint32_t max_matches, lldb_private::VariableList &variables) {
-  if (!parent_decl_ctx)
-    parent_decl_ctx = m_tu_decl_ctx_up.get();
   if (!DeclContextMatchesThisSymbolFile(parent_decl_ctx))
     return 0;
   if (name.IsEmpty())
     return 0;
 
   auto results = m_global_scope_up->findAllChildren<PDBSymbolData>();
   if (!results)
     return 0;
if (!name.GetStringRef().equals(
             MSVCUndecoratedNameParser::DropScope(pdb_data->getName())))
       continue;
 
     sc.comp_unit = ParseCompileUnitForUID(GetCompilandId(*pdb_data)).get();
     // FIXME: We are not able to determine the compile unit.
     if (sc.comp_unit == nullptr)
       continue;
 
-    auto actual_parent_decl_ctx =
-        GetDeclContextContainingUID(result->getSymIndexId());
-    if (actual_parent_decl_ctx != *parent_decl_ctx)
+    if (parent_decl_ctx && GetDeclContextContainingUID(
+                               result->getSymIndexId()) != *parent_decl_ctx)
       continue;
 
     ParseVariables(sc, *pdb_data, &variables);
     matches = variables.GetSize() - old_size;
   }
 
   return matches;
 }
uint32_t SymbolFilePDB::FindFunctions(
 
   auto old_size = sc_list.GetSize();
   if (name_type_mask & eFunctionNameTypeFull ||
       name_type_mask & eFunctionNameTypeBase ||
       name_type_mask & eFunctionNameTypeMethod) {
     CacheFunctionNames();
 
     std::set<uint32_t> resolved_ids;
-    auto ResolveFn = [include_inlines, &name, &sc_list, &resolved_ids,
-                      this](UniqueCStringMap<uint32_t> &Names) {
+    auto ResolveFn = [this, &name, parent_decl_ctx, include_inlines, &sc_list,
+                      &resolved_ids](UniqueCStringMap<uint32_t> &Names) {
       std::vector<uint32_t> ids;
-      if (Names.GetValues(name, ids)) {
-        for (auto id : ids) {
-          if (resolved_ids.find(id) == resolved_ids.end()) {
-            if (ResolveFunction(id, include_inlines, sc_list))
-              resolved_ids.insert(id);
-          }
-        }
-      }
+      if (!Names.GetValues(name, ids))
+        return;
+
+      for (uint32_t id : ids) {
+        if (resolved_ids.find(id) != resolved_ids.end())
+          continue;
+
+        if (parent_decl_ctx &&
+            GetDeclContextContainingUID(id) != *parent_decl_ctx)
+          continue;
+
     };
     if (name_type_mask & eFunctionNameTypeFull) {
       ResolveFn(m_func_full_names);
+      ResolveFn(m_func_base_names);
+      ResolveFn(m_func_method_names);
     }
     if (name_type_mask & eFunctionNameTypeBase) {
       ResolveFn(m_func_base_names);
     }
     if (name_type_mask & eFunctionNameTypeMethod) {
       ResolveFn(m_func_method_names);
     }
   }
for (auto tag : tags_to_search) {
     }
   }
 }
 
 void SymbolFilePDB::FindTypesByName(
     llvm::StringRef name,
     const lldb_private::CompilerDeclContext *parent_decl_ctx,
     uint32_t max_matches, lldb_private::TypeMap &types) {
-  if (!parent_decl_ctx)
-    parent_decl_ctx = m_tu_decl_ctx_up.get();
   std::unique_ptr<IPDBEnumSymbols> results;
   if (name.empty())
     return;
   results = m_global_scope_up->findAllChildren(PDB_SymType::None);
   if (!results)
     return;
 
   uint32_t matches = 0;
default:
       continue;
     }
 
     // This should cause the type to get cached and stored in the `m_types`
     // lookup.
     if (!ResolveTypeUID(result->getSymIndexId()))
       continue;
 
-    auto actual_parent_decl_ctx =
-        GetDeclContextContainingUID(result->getSymIndexId());
-    if (actual_parent_decl_ctx != *parent_decl_ctx)
+    if (parent_decl_ctx && GetDeclContextContainingUID(
+                               result->getSymIndexId()) != *parent_decl_ctx)
       continue;
 
     auto iter = m_types.find(result->getSymIndexId());
     if (iter == m_types.end())
       continue;
     types.Insert(iter->second);
     ++matches;
   }