[LLDB][Clang] add AccessSpecDecl for methods and fields in RecordType

[zequanwu]

Summary

This allows access type be printed when running lldb-test -dump-ast and
lldb-test -dump-clang-ast.

[shafik]

I would like to see more extensive test cases, for example alternating public and private specifiers for both class and struct case.

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

1356

This feels a bit expensive, we have to go through all the fields every time we add a field.

[zequanwu]

Add tests for alternating access.

[zequanwu]

Use a map to keep CXXRecordDecl to last added AccessSpecifier.

[zequanwu]

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

1356

Use a map now.

[labath]

I like this feature, but I gotta say that a map feels a bit wasteful as well, given that the last access specifier is only needed for the short duration of time while the class definition is being parsed.

Maybe if you remove the relevant entry when CompleteTagDeclarationDefinition gets called, then we wouldn't at least have an unboundedly growing map on our hands?

[zequanwu]

Erase the cxx_record_decl entry from map after completing it.

[labath]

This dropped off my radar, but i think it generally looks good. Shafik, do you have anything to add?

[shafik]

LGTM

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

public:
   void SetMetadataAsUserID(const clang::Type *type, lldb::user_id_t user_id);
 
   void SetMetadata(const clang::Decl *object, ClangASTMetadata &meta_data);
 
   void SetMetadata(const clang::Type *object, ClangASTMetadata &meta_data);
   ClangASTMetadata *GetMetadata(const clang::Decl *object);
   ClangASTMetadata *GetMetadata(const clang::Type *object);
 
+  void SetCXXRecordDeclAccess(const clang::CXXRecordDecl *object,
+                              clang::AccessSpecifier access);
+  clang::AccessSpecifier
+  GetCXXRecordDeclAccess(const clang::CXXRecordDecl *object);
+
   // Basic Types
   CompilerType GetBuiltinTypeForEncodingAndBitSize(lldb::Encoding encoding,
                                                    size_t bit_size) override;
 
   CompilerType GetBasicType(lldb::BasicType type);
 
   static lldb::BasicType GetBasicTypeEnumeration(ConstString name);
 
private:
   typedef llvm::DenseMap<const clang::Decl *, ClangASTMetadata> DeclMetadataMap;
   /// Maps Decls to their associated ClangASTMetadata.
   DeclMetadataMap m_decl_metadata;
 
   typedef llvm::DenseMap<const clang::Type *, ClangASTMetadata> TypeMetadataMap;
   /// Maps Types to their associated ClangASTMetadata.
   TypeMetadataMap m_type_metadata;
 
+  typedef llvm::DenseMap<const clang::CXXRecordDecl *, clang::AccessSpecifier>
+      CXXRecordDeclAccessMap;
+  /// Maps CXXRecordDecl to their most recent added method/field's
+  /// AccessSpecifier.
+  CXXRecordDeclAccessMap m_cxx_record_decl_access;
+
   /// The sema associated that is currently used to build this ASTContext.
   /// May be null if we are already done parsing this ASTContext or the
   /// ASTContext wasn't created by parsing source code.
   clang::Sema *m_sema = nullptr;
 
   // For TypeSystemClang only
   TypeSystemClang(const TypeSystemClang &);
   const TypeSystemClang &operator=(const TypeSystemClang &);

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

 }
 
 namespace {
 /// Returns true iff the given TemplateArgument should be represented as an
 /// NonTypeTemplateParmDecl in the AST.
 bool IsValueParam(const clang::TemplateArgument &argument) {
   return argument.getKind() == TemplateArgument::Integral;
 }
+
+void AddAccessSpecifierDecl(clang::CXXRecordDecl *cxx_record_decl,
+                            ASTContext &ct,
+                            clang::AccessSpecifier previous_access,
+                            clang::AccessSpecifier access_specifier) {
+  if (!cxx_record_decl->isClass() && !cxx_record_decl->isStruct())
+    return;
+  if (previous_access != access_specifier) {
+    // For struct, don't add AS_public if it's the first AccessSpecDecl.
+    // For class, don't add AS_private if it's the first AccessSpecDecl.
+    if ((cxx_record_decl->isStruct() &&
+         previous_access == clang::AccessSpecifier::AS_none &&
+         access_specifier == clang::AccessSpecifier::AS_public) ||
+        (cxx_record_decl->isClass() &&
+         previous_access == clang::AccessSpecifier::AS_none &&
+         access_specifier == clang::AccessSpecifier::AS_private)) {
+      return;
+    }
+    cxx_record_decl->addDecl(
+        AccessSpecDecl::Create(ct, access_specifier, cxx_record_decl,
+                               SourceLocation(), SourceLocation()));
   }
+}
+} // namespace
 
 static TemplateParameterList *CreateTemplateParameterList(
     ASTContext &ast,
     const TypeSystemClang::TemplateParameterInfos &template_param_infos,
     llvm::SmallVector<NamedDecl *, 8> &template_param_decls) {
   const bool parameter_pack = false;
   const bool is_typename = false;
   const unsigned depth = 0;

 
 ClangASTMetadata *TypeSystemClang::GetMetadata(const clang::Type *object) {
   auto It = m_type_metadata.find(object);
   if (It != m_type_metadata.end())
     return &It->second;
   return nullptr;
 }
 
+void TypeSystemClang::SetCXXRecordDeclAccess(const clang::CXXRecordDecl *object,
+                                             clang::AccessSpecifier access) {
+  if (access == clang::AccessSpecifier::AS_none)
+    m_cxx_record_decl_access.erase(object);
+  else
+    m_cxx_record_decl_access[object] = access;
+}
+
+clang::AccessSpecifier
+TypeSystemClang::GetCXXRecordDeclAccess(const clang::CXXRecordDecl *object) {
+  auto It = m_cxx_record_decl_access.find(object);
+  if (It != m_cxx_record_decl_access.end())
+    return It->second;
+  return clang::AccessSpecifier::AS_none;
+}
+
 clang::DeclContext *
 TypeSystemClang::GetDeclContextForType(const CompilerType &type) {
   return GetDeclContextForType(ClangUtil::GetQualType(type));
 }
 
 /// Aggressively desugar the provided type, skipping past various kinds of
 /// syntactic sugar and other constructs one typically wants to ignore.
 /// The \p mask argument allows one to skip certain kinds of simplifications,
if (name.empty()) {
           if (!Rec->getDeclName()) {
             Rec->setAnonymousStructOrUnion(true);
             field->setImplicit();
           }
       }
     }
 
     if (field) {
-      field->setAccess(
-          TypeSystemClang::ConvertAccessTypeToAccessSpecifier(access));
+      clang::AccessSpecifier access_specifier =
+          TypeSystemClang::ConvertAccessTypeToAccessSpecifier(access);
+      field->setAccess(access_specifier);
 
+      if (clang::CXXRecordDecl *cxx_record_decl =
+              llvm::dyn_cast<CXXRecordDecl>(record_decl)) {
+        AddAccessSpecifierDecl(cxx_record_decl, ast->getASTContext(),
+                               ast->GetCXXRecordDeclAccess(cxx_record_decl),
+                               access_specifier);
+        ast->SetCXXRecordDeclAccess(cxx_record_decl, access_specifier);
+      }
       record_decl->addDecl(field);
 
       VerifyDecl(field);
     }
   } else {
     clang::ObjCInterfaceDecl *class_interface_decl =
         ast->GetAsObjCInterfaceDecl(type);
 
params.push_back(clang::ParmVarDecl::Create(
         clang::SourceLocation(),
         nullptr, // anonymous
         method_function_prototype->getParamType(param_index), nullptr,
         clang::SC_None, nullptr));
   }
 
   cxx_method_decl->setParams(llvm::ArrayRef<clang::ParmVarDecl *>(params));
 
+  AddAccessSpecifierDecl(cxx_record_decl, getASTContext(),
+                         GetCXXRecordDeclAccess(cxx_record_decl),
+                         access_specifier);
+  SetCXXRecordDeclAccess(cxx_record_decl, access_specifier);
+
   cxx_record_decl->addDecl(cxx_method_decl);
 
   // Sometimes the debug info will mention a constructor (default/copy/move),
   // destructor, or assignment operator (copy/move) but there won't be any
   // version of this in the code. So we check if the function was artificially
   // generated and if it is trivial and this lets the compiler/backend know
   // that it can inline the IR for these when it needs to and we can avoid a
   // "missing function" error when running expressions.

 }
 
 bool TypeSystemClang::CompleteTagDeclarationDefinition(
     const CompilerType &type) {
   clang::QualType qual_type(ClangUtil::GetQualType(type));
   if (qual_type.isNull())
     return false;
 
+  TypeSystemClang *lldb_ast =
+      llvm::dyn_cast<TypeSystemClang>(type.GetTypeSystem());
+  if (lldb_ast == nullptr)
+    return false;
+
   // Make sure we use the same methodology as
   // TypeSystemClang::StartTagDeclarationDefinition() as to how we start/end
   // the definition.
   const clang::TagType *tag_type = qual_type->getAs<clang::TagType>();
   if (tag_type) {
     clang::TagDecl *tag_decl = tag_type->getDecl();
 
     if (auto *cxx_record_decl = llvm::dyn_cast<CXXRecordDecl>(tag_decl)) {
if (auto *cxx_record_decl = llvm::dyn_cast<CXXRecordDecl>(tag_decl)) {
           cxx_record_decl->setImplicitCopyAssignmentIsDeleted();
       }
 
       if (!cxx_record_decl->isCompleteDefinition())
         cxx_record_decl->completeDefinition();
       cxx_record_decl->setHasLoadedFieldsFromExternalStorage(true);
       cxx_record_decl->setHasExternalLexicalStorage(false);
       cxx_record_decl->setHasExternalVisibleStorage(false);
+      lldb_ast->SetCXXRecordDeclAccess(cxx_record_decl,
+                                       clang::AccessSpecifier::AS_none);
       return true;
     }
   }
 
   const clang::EnumType *enutype = qual_type->getAs<clang::EnumType>();
 
   if (!enutype)
     return false;
   clang::EnumDecl *enum_decl = enutype->getDecl();
 
   if (enum_decl->isCompleteDefinition())
     return true;
 
-  TypeSystemClang *lldb_ast =
-      llvm::dyn_cast<TypeSystemClang>(type.GetTypeSystem());
-  if (lldb_ast == nullptr)
-    return false;
   clang::ASTContext &ast = lldb_ast->getASTContext();
 
   /// TODO This really needs to be fixed.
 
   QualType integer_type(enum_decl->getIntegerType());
   if (!integer_type.isNull()) {
     unsigned NumPositiveBits = 1;
     unsigned NumNegativeBits = 0;

lldb/test/Shell/SymbolFile/NativePDB/tag-types.cpp

 // clang-format off
 // REQUIRES: lld, x86
 
 // Test that we can display tag types.
 // RUN: %clang_cl --target=x86_64-windows-msvc -GS- -Od -Z7 -c /Fo%t.obj -- %s
 // RUN: lld-link -debug:full -nodefaultlib -entry:main %t.obj -out:%t.exe -pdb:%t.pdb
 // RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
 // RUN:     %p/Inputs/tag-types.lldbinit | FileCheck %s
 
 // Test struct
 struct Struct {
   // Test builtin types, which are represented by special CodeView type indices.
   bool                B;
+private:
   char                C;
+public:
   signed char         SC;
+protected:
   unsigned char       UC;
   char16_t            C16;
   char32_t            C32;
+protected:
   wchar_t             WC;
   short               S;
   unsigned short      US;
+public:
   int                 I;
+private:
   unsigned int        UI;
+public:
   long                L;
   unsigned long       UL;
   long long           LL;
   unsigned long long  ULL;
   float               F;
   double              D;
   long double         LD;
 };
 
 // Test class
 class Class {
-public:
   // Test pointers to builtin types, which are represented by different special
   // CodeView type indices.
   bool                *PB;
+public:
   char                *PC;
+private:
   signed char         *PSC;
+protected:
   unsigned char       *PUC;
+private:
   char16_t            *PC16;
+public:
   char32_t            *PC32;
+private:
   wchar_t             *PWC;
   short               *PS;
   unsigned short      *PUS;
   int                 *PI;
   unsigned int        *PUI;
   long                *PL;
   unsigned long       *PUL;
   long long           *PLL;

 
 // CHECK:      (lldb) target create "{{.*}}tag-types.cpp.tmp.exe"
 // CHECK-NEXT: Current executable set to '{{.*}}tag-types.cpp.tmp.exe'
 // CHECK-NEXT: (lldb) command source -s 0 '{{.*}}tag-types.lldbinit'
 // CHECK-NEXT: Executing commands in '{{.*}}tag-types.lldbinit'.
 // CHECK-NEXT: (lldb) type lookup -- Struct
 // CHECK-NEXT: struct Struct {
 // CHECK-NEXT:     bool B;
+// CHECK-NEXT: private:
 // CHECK-NEXT:     char C;
+// CHECK-NEXT: public:
 // CHECK-NEXT:     signed char SC;
+// CHECK-NEXT: protected:
 // CHECK-NEXT:     unsigned char UC;
 // CHECK-NEXT:     char16_t C16;
 // CHECK-NEXT:     char32_t C32;
 // CHECK-NEXT:     wchar_t WC;
 // CHECK-NEXT:     short S;
 // CHECK-NEXT:     unsigned short US;
+// CHECK-NEXT: public:
 // CHECK-NEXT:     int I;
+// CHECK-NEXT: private:
 // CHECK-NEXT:     unsigned int UI;
+// CHECK-NEXT: public:
 // CHECK-NEXT:     long L;
 // CHECK-NEXT:     unsigned long UL;
 // CHECK-NEXT:     long long LL;
 // CHECK-NEXT:     unsigned long long ULL;
 // CHECK-NEXT:     float F;
 // CHECK-NEXT:     double D;
 // CHECK-NEXT:     double LD;
 // CHECK-NEXT: }
 // CHECK-NEXT: (lldb) type lookup -- Class
 // CHECK-NEXT: class Class {
 // CHECK-NEXT:     bool *PB;
+// CHECK-NEXT: public:
 // CHECK-NEXT:     char *PC;
+// CHECK-NEXT: private:
 // CHECK-NEXT:     signed char *PSC;
+// CHECK-NEXT: protected:
 // CHECK-NEXT:     unsigned char *PUC;
+// CHECK-NEXT: private:
 // CHECK-NEXT:     char16_t *PC16;
+// CHECK-NEXT: public:
 // CHECK-NEXT:     char32_t *PC32;
+// CHECK-NEXT: private:
 // CHECK-NEXT:     wchar_t *PWC;
 // CHECK-NEXT:     short *PS;
 // CHECK-NEXT:     unsigned short *PUS;
 // CHECK-NEXT:     int *PI;
 // CHECK-NEXT:     unsigned int *PUI;
 // CHECK-NEXT:     long *PL;
 // CHECK-NEXT:     unsigned long *PUL;
 // CHECK-NEXT:     long long *PLL;

 // CHECK-NEXT:     const long long *PLL;
 // CHECK-NEXT:     const unsigned long long *PULL;
 // CHECK-NEXT:     const float *PF;
 // CHECK-NEXT:     const double *PD;
 // CHECK-NEXT:     const double *PLD;
 // CHECK-NEXT: }
 // CHECK-NEXT: (lldb) type lookup -- Derived
 // CHECK-NEXT: class Derived : public Class {
-// CHECK:          Derived &Reference;
+// CHECK-NEXT: public:
+// CHECK-NEXT:     Derived &Reference;
 // CHECK-NEXT:     OneMember Member;
 // CHECK-NEXT:     const OneMember ConstMember;
 // CHECK-NEXT:     volatile OneMember VolatileMember;
 // CHECK-NEXT:     const volatile OneMember CVMember;
 // CHECK-NEXT:     OneMember *PtrMember;
 // CHECK-NEXT:     OneMember &RefMember;
 // CHECK-NEXT:     OneMember &&RValueRefMember;
 // CHECK:      }