[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

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

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

812

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

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

[amccarth]

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

575

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

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

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.

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

 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

lit/SymbolFile/NativePDB/ast-types.cpp

 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: (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: | | | `-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: | `-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;
 }

lit/SymbolFile/NativePDB/typedefs.cpp

 }
 
 
-// CHECK:      namespace `anonymous namespace' {
+// CHECK:      namespace  {
 // CHECK-NEXT:     typedef bool AnonNamespaceTypedef;
 // CHECK-NEXT: }
 // CHECK-NEXT: typedef unsigned long ULongArrayTypedef[10];

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

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h

   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,

   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);

source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

   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();

     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};
   }

   // 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 *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::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 *