Skip to content

[lldb] Infer MSInheritanceAttr for CXXRecordDecl with DWARF on Windows #115177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

weliveindetail
Copy link
Member

Following up from #112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows.

@llvmbot
Copy link
Member

llvmbot commented Nov 6, 2024

@llvm/pr-subscribers-lldb

Author: Stefan Gränitz (weliveindetail)

Changes

Following up from #112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows.


Full diff: https://github.com/llvm/llvm-project/pull/115177.diff

2 Files Affected:

  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+5)
  • (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp (+7-1)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index a30d898a93cc4d..319540389c05ee 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2138,6 +2138,11 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
   if (record_decl)
     GetClangASTImporter().SetRecordLayout(record_decl, layout_info);
 
+  // TODO: Only necessary if we target the Microsoft C++ ABI
+  auto IM = record_decl->calculateInheritanceModel();
+  record_decl->addAttr(clang::MSInheritanceAttr::CreateImplicit(
+      m_ast.getASTContext(), true, {}, clang::MSInheritanceAttr::Spelling(IM)));
+
   // Now parse all contained types inside of the class. We make forward
   // declarations to all classes, but we need the CXXRecordDecl to have decls
   // for all contained types because we don't get asked for them via the
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index f5063175d6e070..a1e224a395e69f 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -2771,8 +2771,14 @@ static bool GetCompleteQualType(clang::ASTContext *ast,
         ast, llvm::cast<clang::AttributedType>(qual_type)->getModifiedType(),
         allow_completion);
 
-  case clang::Type::MemberPointer:
+  case clang::Type::MemberPointer: {
+    auto *MPT = qual_type.getTypePtr()->castAs<clang::MemberPointerType>();
+    if (MPT->getClass()->isRecordType())
+      GetCompleteRecordType(ast, clang::QualType(MPT->getClass(), 0),
+                            allow_completion);
+
     return !qual_type.getTypePtr()->isIncompleteType();
+  }
 
   default:
     break;

Copy link

github-actions bot commented Nov 6, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Virtual2 vi2;
Virtual3 vi3;
Virtual4 vi4;
int sum = sizeof(Single2) + sizeof(Multiple2);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to force emission of type info to DWARF right now. That's reasonable, because Clang doesn't know that we need it to infer member-pointer sizes. I guess we could teach it to do so, but in most real-world scenarios this might be a non-issue.

@weliveindetail
Copy link
Member Author

Pre-merge checks failed due to an unrelated test error. The one on Windows was fine actually, but let me squash and rebase to try this again.

@weliveindetail weliveindetail force-pushed the lldb-msabi-member-pointers branch from 2a30cd9 to b12300b Compare November 8, 2024 16:18
Copy link
Contributor

@ZequanWu ZequanWu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Michael137
Copy link
Member

Btw @weliveindetail , the TestDAP_evaluate.py test failure happens on other PRs too. So I think this is safe to merge.

@weliveindetail weliveindetail force-pushed the lldb-msabi-member-pointers branch from b12300b to 2c8f557 Compare November 18, 2024 09:50
@weliveindetail
Copy link
Member Author

Yes, the pre-merge test failure wasn't related to my change, but still I prefer landing patches with a green light :) Was out of office last week. Let's give it one last try.

@weliveindetail weliveindetail merged commit e370946 into llvm:main Nov 18, 2024
7 checks passed
weliveindetail added a commit to weliveindetail/llvm-project that referenced this pull request Nov 27, 2024
llvm#115177)

Following up from llvm#112928, we
can reuse the approach from Clang Sema to infer the MSInheritanceModel
and add the necessary attribute manually. This allows the inspection of
member function pointers with DWARF on Windows.
adrian-prantl pushed a commit to adrian-prantl/llvm-project that referenced this pull request Dec 5, 2024
llvm#115177)

Following up from llvm#112928, we
can reuse the approach from Clang Sema to infer the MSInheritanceModel
and add the necessary attribute manually. This allows the inspection of
member function pointers with DWARF on Windows.

(cherry picked from commit e9dd419)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants