[LLDB][NativePDB] Add MSInheritanceAttr when creating pointer type that is a pointer to member.

[DavidSpickett]

Is this fixing a specific situation and if so, can a test be written for it? I'm sure we have existing tests you can adapt.

[zequanwu]

add testscase.

[zequanwu]

In D129807#3654447, @DavidSpickett wrote:

Is this fixing a specific situation and if so, can a test be written for it? I'm sure we have existing tests you can adapt.

It fixed the issue. Testcase added.

[rnk]

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

492 ↗

(On Diff #445081)

I think you want to make code changes here to look at mpi.Representation.

762 ↗

(On Diff #445081)

This codepath seems specific to global variable creation. Are there other ways to hit this issue through other codepaths, such as local variables or non-type template parameters?

I would consider moving this logic closer to the logic which creates the AST member pointer type. Any time LLDB loads a member pointer type, it should check the inheritance kind, and then check if the class already has an MSInheritanceAttr, and add one if it is missing.

If the existing attribute doesn't match the attribute you want to add, that represents an ODR violation in the user program. I'm not sure what LLDB should do in that case.

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

319 ↗

(On Diff #445081)

We shouldn't try to reimplement the frontend calculation logic, we should just trust the debug info. Users can use the pointers_to_members pragma to force the compiler to use different representations, so this calculation won't always be right.

Also, I don't think this case handles multiple inheritance via indirect bases. Consider:

struct A { int a; };
struct B { int b; };
struct C : A, B { int c; };
struct D : C { int d; };

If we need this logic in LLDB, try calling CXXRecordDecl::calculateInheritanceModel.

[zequanwu]

Use PointerToMemberRepresentation in pdb to get inheritance attribute.

[rnk]

Thanks!

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

840

This should be virtual, not multiple, right?

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

111

It's worth adding tests for data member pointers as well as function member pointers, in particular ones that use -1 as the null representation. Try this:

int SI::*mp9 = nullptr;

Because offset zero is a valid offset, -1 is used to represent null in this case.

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

clang::QualType PdbAstBuilder::CreatePointerType(const PointerRecord &pointer) {
   if (pointee_type.isNull())
     return {};
 
   if (pointer.isPointerToMember()) {
     MemberPointerInfo mpi = pointer.getMemberInfo();
     clang::QualType class_type = GetOrCreateType(mpi.ContainingType);
     if (class_type.isNull())
       return {};
+    if (clang::TagDecl *tag = class_type->getAsTagDecl()) {
+      clang::MSInheritanceAttr::Spelling spelling;
+      switch (mpi.Representation) {
+      case llvm::codeview::PointerToMemberRepresentation::SingleInheritanceData:
+      case llvm::codeview::PointerToMemberRepresentation::
+          SingleInheritanceFunction:
+        spelling =
+            clang::MSInheritanceAttr::Spelling::Keyword_single_inheritance;
+        break;
+      case llvm::codeview::PointerToMemberRepresentation::
+          MultipleInheritanceData:
+      case llvm::codeview::PointerToMemberRepresentation::
+          MultipleInheritanceFunction:
+        spelling =
+            clang::MSInheritanceAttr::Spelling::Keyword_multiple_inheritance;
+        break;
+      case llvm::codeview::PointerToMemberRepresentation::
+          VirtualInheritanceData:
+      case llvm::codeview::PointerToMemberRepresentation::
+          VirtualInheritanceFunction:
+        spelling =
+            clang::MSInheritanceAttr::Spelling::Keyword_virtual_inheritance;
+        break;
+      case llvm::codeview::PointerToMemberRepresentation::Unknown:
+        spelling =
+            clang::MSInheritanceAttr::Spelling::Keyword_unspecified_inheritance;
+        break;
+      default:
+        spelling = clang::MSInheritanceAttr::Spelling::SpellingNotCalculated;
+        break;
+      }
+      tag->addAttr(clang::MSInheritanceAttr::CreateImplicit(
+          m_clang.getASTContext(), spelling));
+    }
     return m_clang.getASTContext().getMemberPointerType(
         pointee_type, class_type.getTypePtr());
   }
 
   clang::QualType pointer_type;
   if (pointer.getMode() == PointerMode::LValueReference)
     pointer_type = m_clang.getASTContext().getLValueReferenceType(pointee_type);
   else if (pointer.getMode() == PointerMode::RValueReference)

lldb/test/Shell/SymbolFile/NativePDB/Inputs/ast-types.lldbinit

 target variable ACNeg1D
 target variable AD
 target variable ADE
 
 target variable AnonInt
 target variable AnonABCVoid
 target variable AnonABCVoidD
 
+target variable mp1 mp2 mp3 mp4 mp5 mp6 mp7 mp8 mp9
+
 target modules dump ast
 
 quit

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

 A::C<-1>::D ACNeg1D;
 A::D AD;
 A::D::E ADE;
 
 Anonymous<int> AnonInt;
 Anonymous<A::B::C<void>> AnonABCVoid;
 Anonymous<A::B::C<void>>::D AnonABCVoidD;
 
+// The following tests that MSInheritanceAttr are set for record decls.
+class  SI { int si; };
+struct SI2 { int si2; };
+class  MI : SI, SI2 { int mi; };
+class MI2 : MI { int mi2; };
+class VI : virtual MI { int vi; };
+class VI2 : virtual SI, virtual SI2 { int vi; };
+class/* __unspecified_inheritance*/ UI;
+
+typedef void (SI::*SITYPE)();
+typedef void (MI::*MITYPE)();
+typedef void (MI2::*MI2TYPE)();
+typedef void (VI::*VITYPE)();
+typedef void (VI2::*VI2TYPE)();
+typedef void (UI::*UITYPE)();
+SITYPE mp1 = nullptr;
+MITYPE mp2 = nullptr;
+MI2TYPE mp3 = nullptr;
+VITYPE mp4 = nullptr;
+VI2TYPE mp5 = nullptr;
+UITYPE mp6 = nullptr;
+MITYPE *mp7 = nullptr;
+VI2TYPE *mp8 = nullptr;
+int SI::*mp9 = nullptr;
+
 // 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<int>) AnonInt = (AnonymousMember = 0)
 // CHECK: (Anonymous<A::B::C<void>>) AnonABCVoid = (AnonymousMember = 0)
 // CHECK: (Anonymous<A::B::C<void>>::D) AnonABCVoidD = (AnonymousDMember = 0)
+// CHECK: (void (SI::*)()) mp1 = 00 00 00 00 00 00 00 00
+// CHECK: (void (MI::*)()) mp2 = 00 00 00 00 00 00 00 00
+// CHECK: (void (MI2::*)()) mp3 = 00 00 00 00 00 00 00 00
+// CHECK: (void (VI::*)()) mp4 = 00 00 00 00 00 00 00 00
+// CHECK: (void (VI2::*)()) mp5 = 00 00 00 00 00 00 00 00
+// CHECK: (void (UI::*)()) mp6 = 00 00 00 00 00 00 00 00
+// CHECK: (void (MI::**)()) mp7 = 0x0000000000000000
+// CHECK: (void (VI2::**)()) mp8 = 0x0000000000000000
+// CHECK: (int SI::*) mp9 = ff ff ff ff
 // 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