[LLDB][DWARF] Set MSInheritanceAttr for record decl when it's using Microsoft compatible ABI.

[zequanwu]

Summary

This is needed for object files with MS ABI and debug info in Dwarf.
MSInheritanceAttr is required for record decls under MS ABI.

Fixes #56458

[mstorsjo]

This looks sensible to me, although it might be good if someone else more familiar with this code has a look too.

[labath]

Makes sense to me. For my education, what is the effect of not providing a specific inheritance attribute, like it's being done in the pdb code (which, I presume, is not possible because that information is not encoded in dwarf)?

[rnk]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

1754

I'm concerned that this isn't really the right fix. Changing the inheritance model changes the size of the member pointer representation, so the consequences of getting the wrong one could affect expression evaluation results. Zequan suggested guessing the inheritance model from the class definition, but we really ought to write down the inheritance model in the DWARF when using the MS ABI. This probably needs its own DWARF attribute.

[mstorsjo]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

1754

FWIW, there’s two use cases at play here:

1. The executable actually is using the itanium ABI, but is being (mis)interpreted with the MSVC ABI. When this happens, we currently crash, which is a terrible user experience. In this case, there’s probably no good solution (we can’t really get it right at this point) - but pretty much anything is better than crashing. Before D127048, we always interpreted everything with the MSVC C++ ABI; now we probably are getting it right in a majority of cases at least.

2. People intentionally using dwarf debug into with MSVC ABI. Not very common, but something that some people do use, as far as I know. Here we at least have a chance of getting right.

[rnk]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

1754

I see, I thought we already knew we were in use case 2 not 1. Having this as a workaround to not crash seems fine, but we really ought to warn when LLDB encounters these conditions:

1. MS C++ ABI is in use
2. DWARF is in use
3. A C++ source file is in use

Using DWARF for C code in the MS ABI model is fine, no need to warn in that case.

[mstorsjo]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

1754

Some kind of warning would be great, yes. Is MS ABI C++ in dwarf essentially broken by definition (unless we’d extend our dwarf generation)?

[mstorsjo]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

1754

Ping @zequanwu - do you want to follow up on this one?

[zequanwu]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

1754

Hi, I don't think I have time to follow up on this recently.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (!clang_type_was_created) {
       clang_type_was_created = true;
       clang_type = m_ast.CreateRecordType(
           decl_ctx, GetOwningClangModule(die), attrs.accessibility,
           attrs.name.GetCString(), tag_decl_kind, attrs.class_language,
           &metadata, attrs.exports_symbols);
     }
   }
 
+  // Microsoft abi requires tag decl to have MSInheritanceAttr.
+  if (m_ast.getASTContext().getTargetInfo().getCXXABI().isMicrosoft() &&
+      clang_type_was_created) {
+    clang::QualType class_qt =
+        clang::QualType::getFromOpaquePtr(clang_type.GetOpaqueQualType());
+    if (clang::CXXRecordDecl *record_decl = class_qt->getAsCXXRecordDecl()) {
+      record_decl->addAttr(clang::MSInheritanceAttr::CreateImplicit(
+          m_ast.getASTContext(),
+          clang::MSInheritanceAttr::Spelling::Keyword_unspecified_inheritance));
+    }
+  }
+
   // Store a forward declaration to this class type in case any
   // parameters in any class methods need it for the clang types for
   // function prototypes.
   LinkDeclContextToDIE(m_ast.GetDeclContextForType(clang_type), die);
   type_sp = std::make_shared<Type>(
       die.GetID(), dwarf, attrs.name, attrs.byte_size, nullptr,
       LLDB_INVALID_UID, Type::eEncodingIsUID, &attrs.decl, clang_type,
       Type::ResolveState::Forward,

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()) {
+    if (clang::CXXRecordDecl *record_decl = class_type->getAsCXXRecordDecl()) {
       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;
if (clang::CXXRecordDecl *record_decl = class_type->getAsCXXRecordDecl()) {
       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(
+      record_decl->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)

lldb/test/Shell/SymbolFile/DWARF/x86/ms_abi.cpp

+// REQUIRES: lld
+
+// RUN: %clang -c --target=x86_64-windows-msvc -gdwarf %s -o %t.obj
+// RUN: lld-link /out:%t.exe %t.obj /nodefaultlib /entry:main /debug
+// RUN: %lldb -f %t.exe \
+// RUN:   -o "target variable mp1 mp2 mp3 mp4 mp5 mp6 mp7 mp8 mp9" -o "exit" \
+// RUN:   | FileCheck %s
+
+// 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;
+
+// CHECK: (SITYPE) mp1 = 00 00 00 00 00 00 00 00
+// CHECK: (MITYPE) mp2 = 00 00 00 00 00 00 00 00
+// CHECK: (MI2TYPE) mp3 = 00 00 00 00 00 00 00 00
+// CHECK: (VITYPE) mp4 = 00 00 00 00 00 00 00 00
+// CHECK: (VI2TYPE) mp5 = 00 00 00 00 00 00 00 00
+// CHECK: (UITYPE) mp6 = 00 00 00 00 00 00 00 00
+// CHECK: (MITYPE *) mp7 = 0x0000000000000000
+// CHECK: (VI2TYPE *) mp8 = 0x0000000000000000
+// CHECK: (int SI::*) mp9 = ff ff ff ff
+
+int main() {}