[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

631

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

634–661

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?

[zequanwu]

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

634–661

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.cpp

RenderScopeList(llvm::ArrayRef<llvm::ms_demangle::Node *> nodes) {
   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,
+static llvm::SmallVector<PublicSym32> FindPublicSyms(const SegmentOffset &addr,
                                                      SymbolStream &syms,
                                                      PublicsStream &publics) {
   llvm::FixedStreamArray<ulittle32_t> addr_map = publics.getAddressMap();
   auto iter = std::lower_bound(
       addr_map.begin(), addr_map.end(), 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;
+  llvm::SmallVector<PublicSym32, 1> public_syms;
+  while (iter != addr_map.end()) {
     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;
+      public_syms.push_back(p);
+    ++iter;
+  }
+  return public_syms;
 }
 
 clang::Decl *PdbAstBuilder::GetOrCreateSymbolForId(PdbCompilandSymId id) {
   CVSymbol cvs = m_index.ReadSymbolRecord(id);
 
   if (isLocalVariableType(cvs.kind())) {
     clang::DeclContext *scope = GetParentDeclContext(id);
     if (!scope)
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;
+  llvm::StringRef full_name = getSymbolName(sym);
+  llvm::StringRef base_name = full_name.rsplit("::").second;
+  if (!SymbolHasAddress(sym) || base_name.empty())
+    return CreateDeclInfoForUndecoratedName(full_name).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::SmallVector<PublicSym32> public_syms =
+      FindPublicSyms(addr, m_index.symrecords(), m_index.publics());
 
+  for (const PublicSym32 &pub : public_syms) {
     llvm::ms_demangle::Demangler demangler;
-  StringView name{pub->Name.begin(), pub->Name.size()};
+    StringView name{pub.Name.begin(), pub.Name.size()};
     llvm::ms_demangle::SymbolNode *node = demangler.parse(name);
     if (!node)
-    return FromCompilerDeclContext(GetTranslationUnitDecl());
+      continue;
     llvm::ArrayRef<llvm::ms_demangle::Node *> name_components{
-      node->Name->Components->Nodes, node->Name->Components->Count - 1};
+        node->Name->Components->Nodes, node->Name->Components->Count};
+    // Because ICF, we need to filter out the public symbols by base name.
+    if (name_components.empty() ||
+        base_name != name_components.back()->toString())
+      continue;
 
+    name_components = name_components.drop_back();
     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());
+    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;
   }
+  return CreateDeclInfoForUndecoratedName(full_name).first;
+}
 
 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());

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

+// clang-format off
+// REQUIRES: lld, x86
+
+// Test lldb is not treating function as class method or other way around when icf applies to a
+// function and a class method.
+// 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
+
+int main() {
+  A a;
+  B b;
+  return a.f1(1) + b.f2(1) + N1::f3(nullptr, 1) + N2::N3::f4(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: int main();
+// CHECK-NEXT: struct A {
+// CHECK-NEXT:     int f1(int);
+// CHECK-NEXT: };
+// CHECK-NEXT: struct B {
+// CHECK-NEXT:     int f2(int);
+// CHECK-NEXT: };