[NativePDB] Add anonymous namespaces support

[aleksandr.urakov]

Summary

This patch adds anonymous namespaces support to the native PDB plugin.

I had to reference from the main function variables of the types that are inside of the anonymous namespace to include them in debug info. Without the references they are not included. I think it's because they are static, then are visible only in the current translation unit, so they are not needed without any references to them.

There is also the problem case with variables of types that are nested in template structs. For now I've left FIXME in the test because this case is not related to the change.

[labath]

Don't know much about PDBs, but the change seems pretty straight-forward to me. Zach would be the best person to review this, but he's probably busy with other things now. Maybe Adrian could take a look?

[amccarth]

Sorry for the slow response; I'm still learning about this code.

I like where this is going.

lit/SymbolFile/NativePDB/ast-types.cpp

77 ↗

(On Diff #195540)

Is this a new problem because of your change? Or is this an already existing problem you discovered in the process? Is the cause understood? Should you add a commented-out CHECK showing what the test would be when this problem is fixed?

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

575 ↗

(On Diff #195540)

Again, I think you should drop the .c_str().

812 ↗

(On Diff #195540)

Normally, I would agree that StringRef is the right type for name, but looking at the three call sites, it seems that this is going to cause a lot of unnecessary conversions.

The call sites are all passing std::string::c_str(), which is a const char *. So we're constructing a StringRef name (which involves a strlen). The name is then passed on as name.str().c_str() which constructs a new std::string only to pass a const char * again.

I'd argue that, in this case, we're better off declaring name as a const char *.

813 ↗

(On Diff #195540)

I'd also recommend making GetOrCreateNamespaceDecl private, since the only callers are here in PdbAstBuilder.cpp.

[amccarth]

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

575 ↗

(On Diff #195540)

Disregard this comment. I was going to make a different suggestion and this was a leftover part of that.

[aleksandr.urakov]

lit/SymbolFile/NativePDB/ast-types.cpp

77 ↗

(On Diff #195540)

It's an existing problem, but I didn't go deep into it yet. I've added the test lines describing how it should be, thanks.

[aleksandr.urakov]

Thank you for the comments, I agree with you. I've fixed the patch.

[amccarth]

Thanks for the changes. If the tests pass, then this LGTM. Just check the one last question I added about the AC1D test.

(The diff in Phabricator looks very confused, as though you deleted several hundred lines. Perhaps because the original diff had context and this one didn't. If I'm interpretting the diff correctly, this looks good now.)

lit/SymbolFile/NativePDB/ast-types.cpp

22 ↗

(On Diff #195863)

Should this one start with FIXME rather than CHECK? Isn't this the case that's currently broken?

[aleksandr.urakov]

Thanks for the review! Sorry, I've forgot to include context in the updated patch. Yes, it's exactly the case that is broken, I've left FIXME there until it will be fixed.

lldb/trunk/lit/SymbolFile/NativePDB/Inputs/ast-types.lldbinit

 target variable TC
 target variable TS
 target variable TU
 target variable TE
 
 target variable ABCInt
 target variable ABCFloat
 target variable ABCVoid
 
 target variable AC0
 target variable ACNeg1
 
+target variable AC1D
 target variable AC0D
 target variable ACNeg1D
 target variable AD
 target variable ADE
 
+target variable AnonInt
+target variable AnonABCVoid
+target variable AnonABCVoidD
+
 target modules dump ast
 
 quit

lldb/trunk/lit/SymbolFile/NativePDB/ast-types.cpp

 
 A::B::C<int> ABCInt;
 A::B::C<float> ABCFloat;
 A::B::C<void> ABCVoid;
 
 A::C<0> AC0;
 A::C<-1> ACNeg1;
 
+// FIXME: The type `D` is located now at the level of the translation unit.
+// FIXME: Should be located in the namespace `A`, in the struct `C<1>`.
+A::C<1>::D AC1D;
+
 A::C<0>::D AC0D;
 A::C<-1>::D ACNeg1D;
 A::D AD;
 A::D::E ADE;
 
-// FIXME: Anonymous namespaces aren't working correctly.
 Anonymous<int> AnonInt;
 Anonymous<A::B::C<void>> AnonABCVoid;
-Anonymous<A::B::C<int>>::D AnonABCVoidD;
+Anonymous<A::B::C<void>>::D AnonABCVoidD;
 
 // FIXME: Enum size isn't being correctly determined.
 // FIXME: Can't read memory for variable values.
 
 // CHECK: (TrivialC) TC = {}
 // CHECK: (TrivialS) TS = {}
 // CHECK: (TrivialU) TU = {}
 // CHECK: (TrivialE) TE = TE_A
 // CHECK: (A::B::C<int>) ABCInt = (ABCMember = 0)
 // CHECK: (A::B::C<float>) ABCFloat = (ABCMember = 0)
 // CHECK: (A::B::C<void>) ABCVoid = (ABCSpecializationMember = 0x{{0+}})
 // CHECK: (A::C<0>) AC0 = {}
 // CHECK: (A::C<-1>) ACNeg1 = {}
+// CHECK: (A::C<1>::D) AC1D = (ACDMember = 0, CPtr = 0x{{0+}})
 // CHECK: (A::C<0>::D) AC0D = (ACDMember = 0, CPtr = 0x{{0+}})
 // CHECK: (A::C<-1>::D) ACNeg1D = (ACDMember = 0, CPtr = 0x{{0+}})
 // CHECK: (A::D) AD = {}
 // CHECK: (A::D::E) ADE = (ADDMember = 0)
+// CHECK: ((anonymous namespace)::Anonymous<int>) AnonInt = (AnonymousMember = 0)
+// CHECK: ((anonymous namespace)::Anonymous<A::B::C<void>>) AnonABCVoid = (AnonymousMember = 0)
+// CHECK: ((anonymous namespace)::Anonymous<A::B::C<void>>::D) AnonABCVoidD = (AnonymousDMember = 0)
 // CHECK: Dumping clang ast for 1 modules.
 // CHECK: TranslationUnitDecl {{.*}}
 // CHECK: |-CXXRecordDecl {{.*}} class TrivialC definition
 // CHECK: |-CXXRecordDecl {{.*}} struct TrivialS definition
 // CHECK: |-CXXRecordDecl {{.*}} union TrivialU definition
 // CHECK: |-EnumDecl {{.*}} TrivialE
 // CHECK: | `-EnumConstantDecl {{.*}} TE_A 'TrivialE'
 // CHECK: |-NamespaceDecl {{.*}} A
 // CHECK: | |-NamespaceDecl {{.*}} B
 // CHECK: | | |-CXXRecordDecl {{.*}} struct C<int> definition
 // CHECK: | | | `-FieldDecl {{.*}} ABCMember 'int'
 // CHECK: | | |-CXXRecordDecl {{.*}} struct C<float> definition
 // CHECK: | | | `-FieldDecl {{.*}} ABCMember 'float'
 // CHECK: | | `-CXXRecordDecl {{.*}} struct C<void> definition
 // CHECK: | |   `-FieldDecl {{.*}} ABCSpecializationMember 'void *'
+// FIXME: | |-CXXRecordDecl {{.*}} struct C<1> definition
+// FIXME: | | `-CXXRecordDecl {{.*}} class D definition
+// FIXME: | |   |-FieldDecl {{.*}} ACDMember 'int'
+// FIXME: | |   `-FieldDecl {{.*}} CPtr 'A::C<1> *'
 // CHECK: | |-CXXRecordDecl {{.*}} struct C<0> definition
 // CHECK: | | `-CXXRecordDecl {{.*}} class D definition
 // CHECK: | |   |-FieldDecl {{.*}} ACDMember 'int'
 // CHECK: | |   `-FieldDecl {{.*}} CPtr 'A::C<-1> *'
 // CHECK: | |-CXXRecordDecl {{.*}} struct C<-1> definition
 // CHECK: | | `-CXXRecordDecl {{.*}} class D definition
 // CHECK: | |   |-FieldDecl {{.*}} ACDMember 'int'
 // CHECK: | |   `-FieldDecl {{.*}} CPtr 'A::C<-2> *'
 // CHECK: | |-CXXRecordDecl {{.*}} struct C<-2>
 // CHECK: | `-CXXRecordDecl {{.*}} struct D definition
 // CHECK: |   `-CXXRecordDecl {{.*}} struct E definition
 // CHECK: |     `-FieldDecl {{.*}} ADDMember 'int'
+// CHECK: |-NamespaceDecl
+// CHECK: | |-CXXRecordDecl {{.*}} struct Anonymous<int> definition
+// CHECK: | | `-FieldDecl {{.*}} AnonymousMember 'int'
+// CHECK: | `-CXXRecordDecl {{.*}} struct Anonymous<A::B::C<void>> definition
+// CHECK: |   |-FieldDecl {{.*}} AnonymousMember 'int'
+// CHECK: |   `-CXXRecordDecl {{.*}} struct D definition
+// CHECK: |     `-FieldDecl {{.*}} AnonymousDMember 'int'
 
 int main(int argc, char **argv) {
+  AnonInt.AnonymousMember = 1;
+  AnonABCVoid.AnonymousMember = 2;
+  AnonABCVoidD.AnonymousDMember = 3;
+
   return 0;
 }

lldb/trunk/lit/SymbolFile/NativePDB/typedefs.cpp

int main(int argc, char **argv) {
   FuncPtrTypedef X7;
   VarArgsFuncTypedef X8;
   VarArgsFuncTypedefA X9;
   A::AliasTemplate<float> X10;
   return 0;
 }
 
 
-// CHECK:      namespace `anonymous namespace' {
+// CHECK:      namespace  {
 // CHECK-NEXT:     typedef bool AnonNamespaceTypedef;
 // CHECK-NEXT: }
 // CHECK-NEXT: typedef unsigned long ULongArrayTypedef[10];
 // CHECK-NEXT: typedef double *&RefTypedef;
 // CHECK-NEXT: namespace A {
 // CHECK-NEXT:     namespace B {
 // CHECK-NEXT:         typedef double NamespaceTypedef;
 // CHECK-NEXT:     }
 // CHECK-NEXT:     typedef float AliasTemplate<float>;
 // CHECK-NEXT: }
 // CHECK-NEXT: typedef long long (*FuncPtrTypedef)(int &, unsigned char **, short *, const double, volatile bool);
 // CHECK-NEXT: typedef char (*VarArgsFuncTypedef)(void *, long, unsigned short, unsigned int, ...);
 // CHECK-NEXT: typedef float (*VarArgsFuncTypedefA)(...);
 // CHECK-NEXT: typedef int IntTypedef;

lldb/trunk/lit/SymbolFile/PDB/ast-restore.test

 REQUIRES: system-windows, msvc
 RUN: %build --compiler=msvc --nodefaultlib --output=%t.exe %S/Inputs/AstRestoreTest.cpp
-RUN: lldb-test symbols -dump-ast %t.exe | FileCheck --check-prefix=ENUM %s
+RUN: env LLDB_USE_NATIVE_PDB_READER=0 lldb-test symbols -dump-ast %t.exe | FileCheck --check-prefix=ENUM %s
+RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb-test symbols -dump-ast %t.exe | FileCheck --check-prefix=ENUM %s
 RUN: lldb-test symbols -dump-ast %t.exe | FileCheck --check-prefix=GLOBAL %s
 RUN: lldb-test symbols -dump-ast %t.exe | FileCheck --check-prefix=BASE %s
 RUN: lldb-test symbols -dump-ast %t.exe | FileCheck --check-prefix=CLASS %s
 RUN: lldb-test symbols -dump-ast %t.exe | FileCheck --check-prefix=INNER %s
 RUN: lldb-test symbols -dump-ast %t.exe | FileCheck --check-prefix=TEMPLATE %s
 RUN: lldb-test symbols -dump-ast %t.exe | FileCheck --check-prefix=FOO %s
 RUN: lldb-test symbols -dump-ast %t.exe | FileCheck --check-prefix=MAIN %s
 

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

public:
   PdbAstBuilder(ObjectFile &obj, PdbIndex &index);
 
   clang::DeclContext &GetTranslationUnitDecl();
 
   clang::Decl *GetOrCreateDeclForUid(PdbSymUid uid);
   clang::DeclContext *GetOrCreateDeclContextForUid(PdbSymUid uid);
   clang::DeclContext *GetParentDeclContext(PdbSymUid uid);
 
-  clang::NamespaceDecl *GetOrCreateNamespaceDecl(llvm::StringRef name,
-                                                 clang::DeclContext &context);
   clang::FunctionDecl *GetOrCreateFunctionDecl(PdbCompilandSymId func_id);
   clang::BlockDecl *GetOrCreateBlockDecl(PdbCompilandSymId block_id);
   clang::VarDecl *GetOrCreateVariableDecl(PdbCompilandSymId scope_id,
                                           PdbCompilandSymId var_id);
   clang::VarDecl *GetOrCreateVariableDecl(PdbGlobalSymId var_id);
   clang::TypedefNameDecl *GetOrCreateTypedefDecl(PdbGlobalSymId id);
   void ParseDeclsForContext(clang::DeclContext &context);
 
void CreateFunctionParameters(PdbCompilandSymId func_id,
                                 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);
+
   void ParseAllNamespacesPlusChildrenOf(llvm::Optional<llvm::StringRef> parent);
   void ParseDeclsForSimpleContext(clang::DeclContext &context);
   void ParseBlockChildren(PdbCompilandSymId block_id);
 
   void BuildParentMap();
   std::pair<clang::DeclContext *, std::string>
   CreateDeclInfoForType(const llvm::codeview::TagRecord &record, TypeIndex ti);
   std::pair<clang::DeclContext *, std::string>

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

GetNestedTagDefinition(const NestedTypeRecord &Record,
   piece.push_back('@');
   qname.insert(4, std::move(piece));
   if (qname != child.asTag().UniqueName)
     return llvm::None;
 
   return std::move(child);
 }
 
+static bool IsAnonymousNamespaceName(llvm::StringRef name) {
+  return name == "`anonymous namespace'" || name == "`anonymous-namespace'";
+}
+
 PdbAstBuilder::PdbAstBuilder(ObjectFile &obj, PdbIndex &index)
     : m_index(index), m_clang(GetClangASTContext(obj)) {
   BuildParentMap();
 }
 
 clang::DeclContext &PdbAstBuilder::GetTranslationUnitDecl() {
   return *m_clang.GetTranslationUnitDecl();
 }
if (parent_iter == m_parent_types.end()) {
     // a NamespaceDecl and a CXXRecordDecl, so instead we create a class at
     // global scope with the fully qualified name.
     if (AnyScopesHaveTemplateParams(scopes))
       return {context, record.Name};
 
     for (llvm::ms_demangle::Node *scope : scopes) {
       auto *nii = static_cast<llvm::ms_demangle::NamedIdentifierNode *>(scope);
       std::string str = nii->toString();
-      context = m_clang.GetUniqueNamespaceDeclaration(str.c_str(), context);
+      context = GetOrCreateNamespaceDecl(str.c_str(), *context);
     }
     return {context, uname};
   }
 
   // Otherwise, all we need to do is get the parent type of this type and
   // recurse into our lazy type creation / AST reconstruction logic to get an
   // LLDB TypeSP for the parent.  This will cause the AST to automatically get
   // the right DeclContext created for any parent.
while (!types.empty()) {
     if (tag)
       return {clang::TagDecl::castToDeclContext(tag), uname};
     types.pop_back();
   }
 
   // If that fails, treat it as a series of namespaces.
   for (const MSVCUndecoratedNameSpecifier &spec : specs) {
     std::string ns_name = spec.GetBaseName().str();
-    context = m_clang.GetUniqueNamespaceDeclaration(ns_name.c_str(), context);
+    context = GetOrCreateNamespaceDecl(ns_name.c_str(), *context);
   }
   return {context, uname};
 }
 
 clang::DeclContext *
 PdbAstBuilder::GetParentDeclContextForSymbol(const CVSymbol &sym) {
   if (!SymbolHasAddress(sym))
     return CreateDeclInfoForUndecoratedName(getSymbolName(sym)).first;
while (!matches.empty()) {
       matches.pop_back();
     }
   }
 
   // It's not a type.  It must be a series of namespaces.
   clang::DeclContext *context = &GetTranslationUnitDecl();
   while (!name_components.empty()) {
     std::string ns = name_components.front()->toString();
-    context = m_clang.GetUniqueNamespaceDeclaration(ns.c_str(), context);
+    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.

 clang::Decl *PdbAstBuilder::TryGetDecl(PdbSymUid uid) const {
   auto iter = m_uid_to_decl.find(toOpaqueUid(uid));
   if (iter != m_uid_to_decl.end())
     return iter->second;
   return nullptr;
 }
 
 clang::NamespaceDecl *
-PdbAstBuilder::GetOrCreateNamespaceDecl(llvm::StringRef name,
+PdbAstBuilder::GetOrCreateNamespaceDecl(const char *name,
                                         clang::DeclContext &context) {
-  return m_clang.GetUniqueNamespaceDeclaration(name.str().c_str(), &context);
+  return m_clang.GetUniqueNamespaceDeclaration(
+      IsAnonymousNamespaceName(name) ? nullptr : name, &context);
 }
 
 clang::BlockDecl *
 PdbAstBuilder::GetOrCreateBlockDecl(PdbCompilandSymId block_id) {
   if (clang::Decl *decl = TryGetDecl(block_id))
     return llvm::dyn_cast<clang::BlockDecl>(decl);
 
   clang::DeclContext *scope = GetParentDeclContext(block_id);