[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

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 ↗

(On Diff #445942)

This should be virtual, not multiple, right?

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

111 ↗

(On Diff #445942)

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/UdtRecordCompleter.h

class UdtRecordCompleter : public llvm::codeview::TypeVisitorCallbacks {
   clang::TagDecl &m_tag_decl;
   PdbAstBuilder &m_ast_builder;
   PdbIndex &m_index;
   std::vector<IndexedBase> m_bases;
   ClangASTImporter::LayoutInfo m_layout;
   llvm::DenseMap<lldb::opaque_compiler_type_t,
                  llvm::SmallSet<std::pair<llvm::StringRef, CompilerType>, 8>>
       &m_cxx_record_map;
+  bool has_virtual_base = false;
 
 public:
   UdtRecordCompleter(
       PdbTypeSymId id, CompilerType &derived_ct, clang::TagDecl &tag_decl,
       PdbAstBuilder &ast_builder, PdbIndex &index,
       llvm::DenseMap<lldb::opaque_compiler_type_t,
                      llvm::SmallSet<std::pair<llvm::StringRef, CompilerType>,
                                     8>> &cxx_record_map);

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

Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr,
   auto offset = clang::CharUnits::fromQuantity(base.getBaseOffset());
   m_layout.base_offsets.insert(std::make_pair(decl, offset));
 
   return llvm::Error::success();
 }
 
 Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr,
                                            VirtualBaseClassRecord &base) {
+  has_virtual_base = true;
   AddBaseClassForTypeIndex(base.BaseType, base.getAccess(), base.VTableIndex);
 
   return Error::success();
 }
 
 Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr,
                                            ListContinuationRecord &cont) {
   return Error::success();
void UdtRecordCompleter::complete() {
   TypeSystemClang &clang = m_ast_builder.clang();
   clang.TransferBaseClasses(m_derived_ct.GetOpaqueQualType(), std::move(bases));
 
   clang.AddMethodOverridesForCXXRecordType(m_derived_ct.GetOpaqueQualType());
   TypeSystemClang::BuildIndirectFields(m_derived_ct);
   TypeSystemClang::CompleteTagDeclarationDefinition(m_derived_ct);
 
   if (auto *record_decl = llvm::dyn_cast<clang::CXXRecordDecl>(&m_tag_decl)) {
+    clang::MSInheritanceAttr::Spelling spelling;
+    if (has_virtual_base)
+      spelling = clang::MSInheritanceAttr::Keyword_virtual_inheritance;
+    else if (bases.size() > 1)
+      spelling = clang::MSInheritanceAttr::Keyword_multiple_inheritance;
+    else
+      spelling = clang::MSInheritanceAttr::Keyword_single_inheritance;
+    clang::MSInheritanceAttr *inheritance_attr =
+        clang::MSInheritanceAttr::CreateImplicit(
+            m_ast_builder.clang().getASTContext(), spelling);
+    record_decl->addAttr(inheritance_attr);
     m_ast_builder.importer().SetRecordLayout(record_decl, m_layout);
   }
 }