[LLDB][NativePDB] Fix PdbAstBuilder::GetParentDeclContext when ICF happens.

[zequanwu]

Summary

Removed GetParentDeclContextForSymbol as this is not necesssary. We can get
the demangled names from CVSymbol and then using it to create tag decl or
namespace decl. This also fixed a bug when icf applied.

[zequanwu]

Fix comment in test case.

[labath]

lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

568–569

I know you didn't write this, but I still have to express my surprise at the existence of this code. My don't have anything like that -- using demangled names to look up things -- in the DWARF plugin. There, you can find the containing context by just going "up" in the DIE tree. I don't know if that is possible in PDB, but the current implementation seems fairly fragile, and I suspect it could be broken if the user overrides a mangled name of a symbol (void f() asm("my_linkage_name");).
Are you sure it's not possible to do this in some other way?

581

What if the two symbols have the same address (same code) and the same base name (but are e.g. in different namespaces)?

[zequanwu]

lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

568–569

For namespace, I don't see any symbol record for it. All members of namespace, functions or variables, just have namespace string as a prefix for their names, like N1::N2::func.
@rnk Do you know any other ways to get the namespace string of a symbol rather than using mangled names?

[zequanwu]

• Removed GetParentDeclContextForSymbol as this is not necesssary. We can get the demangled names from CVSymbol and then using it to create tag decl or namespace decl.
• Add test cases with same base names, but with different namespace/class.

[labath]

Well, I still don't claim to understand this code, but I am happy to stamp anything that fixes bugs by deleting code.

Just as an idea for future work, you may want to check that we're correctly handling functions with unusual linkage names (e.g. due to asm labels). In DWARF, we're adding explicit asm labels with the debug-info-provided linkage names to make this work.

lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h

private:
 
   void CreateFunctionParameters(PdbCompilandSymId func_id,
                                 clang::FunctionDecl &function_decl,
                                 uint32_t param_count);
   clang::Decl *GetOrCreateSymbolForId(PdbCompilandSymId id);
   clang::VarDecl *CreateVariableDecl(PdbSymUid uid,
                                      llvm::codeview::CVSymbol sym,
                                      clang::DeclContext &scope);
-  clang::DeclContext *
-  GetParentDeclContextForSymbol(const llvm::codeview::CVSymbol &sym);
-
   clang::NamespaceDecl *GetOrCreateNamespaceDecl(const char *name,
                                                  clang::DeclContext &context);
   clang::FunctionDecl *CreateFunctionDeclFromId(PdbTypeSymId func_tid,
                                                 PdbCompilandSymId func_sid);
   clang::FunctionDecl *
   CreateFunctionDecl(PdbCompilandSymId func_id, llvm::StringRef func_name,
                      TypeIndex func_ti, CompilerType func_ct,
                      uint32_t param_count, clang::StorageClass func_storage,

lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

static bool isLocalVariableType(SymbolKind K) {
   case S_LOCAL:
     return true;
   default:
     break;
   }
   return false;
 }
 
-static std::string
-RenderScopeList(llvm::ArrayRef<llvm::ms_demangle::Node *> nodes) {
-  lldbassert(!nodes.empty());
-
-  std::string result = nodes.front()->toString();
-  nodes = nodes.drop_front();
-  while (!nodes.empty()) {
-    result += "::";
-    result += nodes.front()->toString(llvm::ms_demangle::OF_NoTagSpecifier);
-    nodes = nodes.drop_front();
-  }
-  return result;
-}
-
-static llvm::Optional<PublicSym32> FindPublicSym(const SegmentOffset &addr,
-                                                 SymbolStream &syms,
-                                                 PublicsStream &publics) {
-  llvm::FixedStreamArray<ulittle32_t> addr_map = publics.getAddressMap();
-  auto iter = llvm::lower_bound(
-      addr_map, addr, [&](const ulittle32_t &x, const SegmentOffset &y) {
-        CVSymbol s1 = syms.readRecord(x);
-        lldbassert(s1.kind() == S_PUB32);
-        PublicSym32 p1;
-        llvm::cantFail(SymbolDeserializer::deserializeAs<PublicSym32>(s1, p1));
-        if (p1.Segment < y.segment)
-          return true;
-        return p1.Offset < y.offset;
-      });
-  if (iter == addr_map.end())
-    return llvm::None;
-  CVSymbol sym = syms.readRecord(*iter);
-  lldbassert(sym.kind() == S_PUB32);
-  PublicSym32 p;
-  llvm::cantFail(SymbolDeserializer::deserializeAs<PublicSym32>(sym, p));
-  if (p.Segment == addr.segment && p.Offset == addr.offset)
-    return p;
-  return llvm::None;
-}
-
 clang::Decl *PdbAstBuilder::GetOrCreateSymbolForId(PdbCompilandSymId id) {
   CVSymbol cvs = m_index.ReadSymbolRecord(id);
 
   if (isLocalVariableType(cvs.kind())) {
     clang::DeclContext *scope = GetParentDeclContext(id);
     if (!scope)
       return nullptr;
     clang::Decl *scope_decl = clang::Decl::castFromDeclContext(scope);
PdbAstBuilder::CreateDeclInfoForUndecoratedName(llvm::StringRef name) {
 
   // If that fails, treat it as a series of namespaces.
   for (const MSVCUndecoratedNameSpecifier &spec : specs) {
     std::string ns_name = spec.GetBaseName().str();
     context = GetOrCreateNamespaceDecl(ns_name.c_str(), *context);
   }
   return {context, std::string(uname)};
 }
 
-clang::DeclContext *
-PdbAstBuilder::GetParentDeclContextForSymbol(const CVSymbol &sym) {
-  if (!SymbolHasAddress(sym))
-    return CreateDeclInfoForUndecoratedName(getSymbolName(sym)).first;
-  SegmentOffset addr = GetSegmentAndOffset(sym);
-  llvm::Optional<PublicSym32> pub =
-      FindPublicSym(addr, m_index.symrecords(), m_index.publics());
-  if (!pub)
-    return CreateDeclInfoForUndecoratedName(getSymbolName(sym)).first;
-
-  llvm::ms_demangle::Demangler demangler;
-  StringView name{pub->Name.begin(), pub->Name.size()};
-  llvm::ms_demangle::SymbolNode *node = demangler.parse(name);
-  if (!node)
-    return FromCompilerDeclContext(GetTranslationUnitDecl());
-  llvm::ArrayRef<llvm::ms_demangle::Node *> name_components{
-      node->Name->Components->Nodes, node->Name->Components->Count - 1};
-
-  if (!name_components.empty()) {
-    // Render the current list of scope nodes as a fully qualified name, and
-    // look it up in the debug info as a type name.  If we find something,
-    // this is a type (which may itself be prefixed by a namespace).  If we
-    // don't, this is a list of namespaces.
-    std::string qname = RenderScopeList(name_components);
-    std::vector<TypeIndex> matches = m_index.tpi().findRecordsByName(qname);
-    while (!matches.empty()) {
-      clang::QualType qt = GetOrCreateType(matches.back());
-      if (qt.isNull())
-        continue;
-      clang::TagDecl *tag = qt->getAsTagDecl();
-      if (tag)
-        return clang::TagDecl::castToDeclContext(tag);
-      matches.pop_back();
-    }
-  }
-
-  // It's not a type.  It must be a series of namespaces.
-  auto context = FromCompilerDeclContext(GetTranslationUnitDecl());
-  while (!name_components.empty()) {
-    std::string ns = name_components.front()->toString();
-    context = GetOrCreateNamespaceDecl(ns.c_str(), *context);
-    name_components = name_components.drop_front();
-  }
-  return context;
-}
-
 clang::DeclContext *PdbAstBuilder::GetParentDeclContext(PdbSymUid uid) {
   // We must do this *without* calling GetOrCreate on the current uid, as
   // that would be an infinite recursion.
   switch (uid.kind()) {
   case PdbSymUidKind::CompilandSym: {
     llvm::Optional<PdbCompilandSymId> scope =
         FindSymbolScope(m_index, uid.asCompilandSym());
     if (scope)
       return GetOrCreateDeclContextForUid(*scope);
 
     CVSymbol sym = m_index.ReadSymbolRecord(uid.asCompilandSym());
-    return GetParentDeclContextForSymbol(sym);
+    return CreateDeclInfoForUndecoratedName(getSymbolName(sym)).first;
   }
   case PdbSymUidKind::Type: {
     // It could be a namespace, class, or global.  We don't support nested
     // functions yet.  Anyway, we just need to consult the parent type map.
     PdbTypeSymId type_id = uid.asTypeSym();
     auto iter = m_parent_types.find(type_id.index);
     if (iter == m_parent_types.end())
       return FromCompilerDeclContext(GetTranslationUnitDecl());
     return GetOrCreateDeclContextForUid(PdbTypeSymId(iter->second));
   }
   case PdbSymUidKind::FieldListMember:
     // In this case the parent DeclContext is the one for the class that this
     // member is inside of.
     break;
   case PdbSymUidKind::GlobalSym: {
     // If this refers to a compiland symbol, just recurse in with that symbol.
     // The only other possibilities are S_CONSTANT and S_UDT, in which case we
     // need to parse the undecorated name to figure out the scope, then look
     // that up in the TPI stream.  If it's found, it's a type, othewrise it's
     // a series of namespaces.
     // FIXME: do this.
     CVSymbol global = m_index.ReadSymbolRecord(uid.asGlobalSym());
     switch (global.kind()) {
     case SymbolKind::S_GDATA32:
     case SymbolKind::S_LDATA32:
-      return GetParentDeclContextForSymbol(global);
+      return CreateDeclInfoForUndecoratedName(getSymbolName(global)).first;;
     case SymbolKind::S_PROCREF:
     case SymbolKind::S_LPROCREF: {
       ProcRefSym ref{global.kind()};
       llvm::cantFail(
           SymbolDeserializer::deserializeAs<ProcRefSym>(global, ref));
       PdbCompilandSymId cu_sym_id{ref.modi(), ref.SymOffset};
       return GetParentDeclContext(cu_sym_id);
     }

lldb/test/Shell/SymbolFile/NativePDB/icf.cpp

+// clang-format off
+// REQUIRES: lld, x86
+
+// Test lldb finds the correct parent context decl for functions and class methods when icf happens.
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -GS- -fno-addrsig -c /Fo%t.obj -- %s
+// RUN: lld-link -opt:icf -debug:full -nodefaultlib -entry:main %t.obj -out:%t.exe -pdb:%t.pdb
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb-test symbols --dump-ast %t.exe | FileCheck %s
+
+struct A {
+  int f1(int x) {
+    return x * 2;
+  }
+};
+struct B {
+  int f2(int x) {
+    return x * 2;
+  }
+};
+namespace N1 {
+int f3(void*, int x) {
+  return  x * 2;
+}
+} // namespace N1
+
+namespace N2 {
+namespace N3 {
+  int f4(void*, int x) {
+    return  x * 2;
+  }
+} // namespace N3
+} // namespace N2
+
+namespace N4 {
+  // Same base name as N1::f3 but different namespaces.
+  int f3(void*, int x) {
+    return x * 2;
+  }
+  // Same base name as B::f2 but this is in namespace.
+  int f2(void*, int x) {
+    return x * 2;
+  }
+} // namespace N4
+
+
+int main() {
+  A a;
+  B b;
+  return a.f1(1) + b.f2(1) + N1::f3(nullptr, 1) + N2::N3::f4(nullptr, 1) +
+         N4::f3(nullptr, 1);
+}
+
+
+// CHECK:      namespace N1 {
+// CHECK-NEXT:     int f3(void *, int x);
+// CHECK-NEXT: }
+// CHECK-NEXT: namespace N2 {
+// CHECK-NEXT:     namespace N3 {
+// CHECK-NEXT:         int f4(void *, int x);
+// CHECK-NEXT:     }
+// CHECK-NEXT: }
+// CHECK-NEXT: namespace N4 {
+// CHECK-NEXT:     int f3(void *, int x);
+// CHECK-NEXT:     int f2(void *, int x);
+// CHECK-NEXT: }
+// CHECK-NEXT: int main();
+// CHECK-NEXT: struct A {
+// CHECK-NEXT:     int f1(int);
+// CHECK-NEXT: };
+// CHECK-NEXT: struct B {
+// CHECK-NEXT:     int f2(int);
+// CHECK-NEXT: };