[lldb][DWARFASTParserClang] Resolve nested types when parsing structures

[Endill]

Summary

Fixes #64291

[Endill]

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

3046

I've used https://github.com/llvm/llvm-project/blob/fa2b038cadf17d08014e5fb75c47b5024860953e/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L2689 as a reference, and I guess I'm missing CompileUnit counterpart of this. Surprisingly to me, even with CompileUnit part missing, I was able to debug a Clang crash successfully.

[Endill]

This patch is intentionally aimed squarely at enums.
But I'd like lldb to address the issue of accessing compile-time constants holistically. Or at least agree on a path forward for that.

[Michael137]

Thanks for addressing this.

Can you add a test? Possibly in lldb/test/API/lang/cpp/enum_types/. Or lldb/test/API/lang/cpp/const_static_integral_member/

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

3043

At first glance this seems OK but I'll want to check why the type doesn't get resolved when clang::Sema asks LLDB for it by name

3043

Checking locally and will report back

[Michael137]

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

3043

Ok, so what happens for expr Outer::Enum::var is that LLDB will first resolve the Outer structure (and all its member fields, but not nested types). When clang looks for Enum, it wants to find it in a direct lookup into the Outer DeclContext. But that lookup will fail because we never created the decls for the nested type. There is no fallback to the external source in such a case unfortunately so LLDB never has the chance to correct this. See the LookupQualifiedName code path in Sema::BuildCXXNestedNameSpecifier.

So I think this proposed approach should be fine. But what I think could be better is if we just do the same for DW_TAG_enumeration_type and DW_TAG_structure_type as we do for DW_TAG_subprogram.

I.e., simply push back the type into member_function_dies (we should rename this) and then it will get resolved properly in CompleteRecordType.

[Michael137]

Could you update the commit message with a description of the failure and summary of the fix? And change the title to something like [lldb][DWARFASTParserClang] Resolve nested types when parsing structures

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

3043

Also DW_TAG_union_type while you're here :)

[Endill]

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

3043

I can look into doing things more lazily like DW_TAG_subprogram, if you can point me to code paths for this on LLDB side:

When clang looks for Enum, it wants to find it in a direct lookup into the Outer DeclContext. But that lookup will fail because we never created the decls for the nested type. There is no fallback to the external source in such a case unfortunately so LLDB never has the chance to correct this.

[Michael137]

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

3043

The DW_TAG_subprogram path is just above this one in the switch statement (line 3030). Search for member_function_dies in this file.

if you can point me to code paths for this on LLDB side

Clang requests type completion by calling FindExternalVisibleDeclsByName (which LLDB implements). So if you break in that function you should be able to see what LLDB finds when it looks for the outer structure and the enum. But you don't necessarily need to do that. If we just fill up the member_function_dies vector with the nested enum/union/structure types like we do for DW_TAG_subprogram that is sufficient.

[Michael137]

Are you still planning on moving this forward? Otherwise I could commandeer the revision to get this in. I do think it's a useful bug to address

[Endill]

In D156774#4572947, @Michael137 wrote:

Are you still planning on moving this forward? Otherwise I could commandeer the revision to get this in. I do think it's a useful bug to address

I do. Locally I've been preparing additional changes on top of this patch that expose enums in SBType, so that I can do end-to-end test to ensure this sufficiently addresses my use case.
On top of that, I'm planning to redo this patch after the example of DW_TAG_subprogram per your suggestion.

Unfortunately, I didn't have time last week for this, and now I'm knee deep into triaging old Clang bugs. I'm not finished with that until number of open issues is brought under 20k :)

[Michael137]

In D156774#4572967, @Endill wrote:

In D156774#4572947, @Michael137 wrote:

Are you still planning on moving this forward? Otherwise I could commandeer the revision to get this in. I do think it's a useful bug to address

I do. Locally I've been preparing additional changes on top of this patch that expose enums in SBType, so that I can do end-to-end test to ensure this sufficiently addresses my use case.
On top of that, I'm planning to redo this patch after the example of DW_TAG_subprogram per your suggestion.

Unfortunately, I didn't have time last week for this, and now I'm knee deep into triaging old Clang bugs. I'm not finished with that until number of open issues is brought under 20k :)

Sounds good, thanks!

[Endill]

I tested this patch together with the following new code:

uint32_t TypeSystemClang::GetNumMemberEnums(lldb::opaque_compiler_type_t type) {
  using EnumIt = clang::DeclContext::specific_decl_iterator<clang::EnumDecl>;
  if (!type)
    return 0;
  clang::QualType qual_type = RemoveWrappingTypes(GetCanonicalQualType(type));
  clang::DeclContext *ctx = qual_type->getAsRecordDecl()->getDeclContext();
  return std::distance(EnumIt(ctx->decls_begin()), EnumIt(ctx->decls_end()));
}

CompilerType
TypeSystemClang::GetMemberEnumAtIndex(lldb::opaque_compiler_type_t type,
                                      size_t index) {
  using EnumIt = clang::DeclContext::specific_decl_iterator<clang::EnumDecl>;
  if (!type)
    return CompilerType();

  clang::QualType qual_type = RemoveWrappingTypes(GetCanonicalQualType(type));
  clang::DeclContext *ctx = qual_type->getAsRecordDecl()->getDeclContext();
  size_t enum_index = 0;
  for (EnumIt enums_it(ctx->decls_begin()), enums_end(ctx->decls_end());
       enums_it != enums_end;
       ++enums_it, ++enum_index) {
      if (enum_index == index) {
        return CompilerType(weak_from_this(), *enums_it);
      }
  }
}

I created all the wrappers to make it available in Python. The result was unsatisfactory: this code doesn't even trigger DWARFASTParserClang::ParseChildMembers that this patch touches, and return 0 instead of 1. This doesn't change even if I trigger ParseChildMembers via other means before asking for a number of member enums in a type.

Code I tested this on:

using intptr_t = long;
using uintptr_t = unsigned long;

struct PointerIntPairInfo {
  enum MaskAndShiftConstants : uintptr_t {
    PointerBitMask =
        ~(uintptr_t)(((intptr_t)1 << 3) - 1),
  };

  int a{};
};

static uintptr_t dummy() {
  return PointerIntPairInfo::PointerBitMask;
}

int main()
{
    PointerIntPairInfo p;
    __builtin_debugtrap();
    return p.a + foo();
}

If you have any suggestions what I missed or did wrong, please let me know.

I'll continue with this patch nevertheless, but it's clear now that there's still a way to go until I can access that enum without going through slow expression evaluator.

[Michael137]

In D156774#4601705, @Endill wrote:

I tested this patch together with the following new code:

uint32_t TypeSystemClang::GetNumMemberEnums(lldb::opaque_compiler_type_t type) {
  using EnumIt = clang::DeclContext::specific_decl_iterator<clang::EnumDecl>;
  if (!type)
    return 0;
  clang::QualType qual_type = RemoveWrappingTypes(GetCanonicalQualType(type));
  clang::DeclContext *ctx = qual_type->getAsRecordDecl()->getDeclContext();
  return std::distance(EnumIt(ctx->decls_begin()), EnumIt(ctx->decls_end()));
}

CompilerType
TypeSystemClang::GetMemberEnumAtIndex(lldb::opaque_compiler_type_t type,
                                      size_t index) {
  using EnumIt = clang::DeclContext::specific_decl_iterator<clang::EnumDecl>;
  if (!type)
    return CompilerType();

  clang::QualType qual_type = RemoveWrappingTypes(GetCanonicalQualType(type));
  clang::DeclContext *ctx = qual_type->getAsRecordDecl()->getDeclContext();
  size_t enum_index = 0;
  for (EnumIt enums_it(ctx->decls_begin()), enums_end(ctx->decls_end());
       enums_it != enums_end;
       ++enums_it, ++enum_index) {
      if (enum_index == index) {
        return CompilerType(weak_from_this(), *enums_it);
      }
  }
}

I created all the wrappers to make it available in Python. The result was unsatisfactory: this code doesn't even trigger DWARFASTParserClang::ParseChildMembers that this patch touches, and return 0 instead of 1. This doesn't change even if I trigger ParseChildMembers via other means before asking for a number of member enums in a type.

Code I tested this on:

using intptr_t = long;
using uintptr_t = unsigned long;

struct PointerIntPairInfo {
  enum MaskAndShiftConstants : uintptr_t {
    PointerBitMask =
        ~(uintptr_t)(((intptr_t)1 << 3) - 1),
  };

  int a{};
};

static uintptr_t dummy() {
  return PointerIntPairInfo::PointerBitMask;
}

int main()
{
    PointerIntPairInfo p;
    __builtin_debugtrap();
    return p.a + foo();
}

If you have any suggestions what I missed or did wrong, please let me know.

I'll continue with this patch nevertheless, but it's clear now that there's still a way to go until I can access that enum without going through slow expression evaluator.

What were your lldb commands when you tested this?

LLDB currently completes types lazily when it thinks it can. Does your new API still fail if you run expr p prior? (the idea is that that would trigger completion of the type and parse dwarf). If we dont ever call GetFullCompilerType on your type LLDB will never try to pull in the definition

[Endill]

Follow the DW_TAG_subprogram approach for DW_TAG_enum_type, DW_TAG_structure_type, and DW_TAG_union_type: rely on ParseType() called afterwards instead of eagerly parsing them.

[Endill]

In D156774#4601736, @Michael137 wrote:

What were your lldb commands when you tested this?

script import lldb; frame = lldb.thread.GetFrameAtIndex(0); print(frame.variables[0].type.GetNumberOfMemberEnums())

LLDB currently completes types lazily when it thinks it can. Does your new API still fail if you run expr p prior? (the idea is that that would trigger completion of the type and parse dwarf). If we dont ever call GetFullCompilerType on your type LLDB will never try to pull in the definition

No amount of tinkering with expr makes script print(frame.variables[0].type.GetNumberOfMemberEnums()) output a non-zero value.
I tested this with the changes I uploaded here half an hour ago.

[Michael137]

In D156774#4601767, @Endill wrote:

In D156774#4601736, @Michael137 wrote:

What were your lldb commands when you tested this?

script import lldb; frame = lldb.thread.GetFrameAtIndex(0); print(frame.variables[0].type.GetNumberOfMemberEnums())

LLDB currently completes types lazily when it thinks it can. Does your new API still fail if you run expr p prior? (the idea is that that would trigger completion of the type and parse dwarf). If we dont ever call GetFullCompilerType on your type LLDB will never try to pull in the definition

No amount of tinkering with expr makes script print(frame.variables[0].type.GetNumberOfMemberEnums()) output a non-zero value.
I tested this with the changes I uploaded here half an hour ago.

In D156774#4601767, @Endill wrote:

In D156774#4601736, @Michael137 wrote:

What were your lldb commands when you tested this?

script import lldb; frame = lldb.thread.GetFrameAtIndex(0); print(frame.variables[0].type.GetNumberOfMemberEnums())

LLDB currently completes types lazily when it thinks it can. Does your new API still fail if you run expr p prior? (the idea is that that would trigger completion of the type and parse dwarf). If we dont ever call GetFullCompilerType on your type LLDB will never try to pull in the definition

No amount of tinkering with expr makes script print(frame.variables[0].type.GetNumberOfMemberEnums()) output a non-zero value.
I tested this with the changes I uploaded here half an hour ago.

I think you may want to use GetCompleteQualType before iterating the DeclContext; that's how some of the other TypeSystemClang APIs do it. That will make sure we complete the type by the time we look for the enums.

E.g.,:

clang::QualType qual_type = RemoveWrappingTypes(GetCanonicalQualType(type));                               
if (GetCompleteQualType(&getASTContext(), qual_type)) {                                                     
  const clang::RecordType *record_type =                                                                         
      llvm::cast<clang::RecordType>(qual_type.getTypePtr());                                                
  const clang::RecordDecl *record_decl = record_type->getDecl();                                                 
  assert(record_decl);                                                                                           
  return std::distance(EnumIt(record_decl->decls_begin()), EnumIt(record_decl->decls_end())); 
}

Will review it more thoroughly on tomorrow though

[Endill]

In D156774#4601830, @Michael137 wrote:

I think you may want to use GetCompleteQualType before iterating the DeclContext; that's how some of the other TypeSystemClang APIs do it. That will make sure we complete the type by the time we look for the enums.

I tried to incorporate this function into my version of GetNumberOfMemberEnums, but it didn't work out.

E.g.,:

clang::QualType qual_type = RemoveWrappingTypes(GetCanonicalQualType(type));                               
if (GetCompleteQualType(&getASTContext(), qual_type)) {                                                     
  const clang::RecordType *record_type =                                                                         
      llvm::cast<clang::RecordType>(qual_type.getTypePtr());                                                
  const clang::RecordDecl *record_decl = record_type->getDecl();                                                 
  assert(record_decl);                                                                                           
  return std::distance(EnumIt(record_decl->decls_begin()), EnumIt(record_decl->decls_end())); 
}

Will review it more thoroughly on tomorrow though

But your version actually works, returning 1! Thank you very much!

[Endill]

Ping @Michael137

[Michael137]

In D156774#4644503, @Endill wrote:

Ping @Michael137

Sorry for the delay, just came back from vacation

The change itself LGTM. Can we add a test though? We do have DWARFASTParserClang unittests: https://github.com/llvm/llvm-project/blob/main/lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp

If the yaml input file becomes tedious to write you could also just an API test. E.g., under https://github.com/llvm/llvm-project/tree/main/lldb/test/API/lang/cpp

Let me know if you need some pointers in writing the tests

[Michael137]

Also, I assume the extra changes to make the PointerIntPair formatter work will be in a follow-up patch?

[Endill]

In D156774#4644623, @Michael137 wrote:

Also, I assume the extra changes to make the PointerIntPair formatter work will be in a follow-up patch?

Yes. That work is not finished yet.

[Endill]

@Michael137 I moved this over to GitHub, and I need your help there writing the unit test.

[Endill]

Moved to GitHub: https://github.com/llvm/llvm-project/pull/66879

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

bool ParseTemplateParameterInfos(
       lldb_private::TypeSystemClang::TemplateParameterInfos
           &template_param_infos);
 
   std::string GetCPlusPlusQualifiedName(const DWARFDIE &die);
 
   bool ParseChildMembers(
       const DWARFDIE &die, lldb_private::CompilerType &class_compiler_type,
       std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
-      std::vector<DWARFDIE> &member_function_dies,
+      std::vector<DWARFDIE> &member_function_and_type_dies,
       DelayedPropertyList &delayed_properties,
       const lldb::AccessType default_accessibility,
       lldb_private::ClangASTImporter::LayoutInfo &layout_info);
 
   size_t
   ParseChildParameters(clang::DeclContext *containing_decl_ctx,
                        const DWARFDIE &parent_die, bool skip_artificial,
                        bool &is_static, bool &is_variadic,

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

if (die.HasChildren()) {
     } else if (tag == DW_TAG_union_type) {
       default_accessibility = eAccessPublic;
     } else if (tag == DW_TAG_class_type) {
       default_accessibility = eAccessPrivate;
     }
 
     std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> bases;
     // Parse members and base classes first
-    std::vector<DWARFDIE> member_function_dies;
+    std::vector<DWARFDIE> member_function_and_type_dies;
 
     DelayedPropertyList delayed_properties;
-    ParseChildMembers(die, clang_type, bases, member_function_dies,
+    ParseChildMembers(die, clang_type, bases, member_function_and_type_dies,
                       delayed_properties, default_accessibility, layout_info);
 
-    // Now parse any methods if there were any...
-    for (const DWARFDIE &die : member_function_dies)
+    // Now parse any methods or nested types if there were any...
+    for (const DWARFDIE &die : member_function_and_type_dies)
       dwarf->ResolveType(die);
 
     if (type_is_objc_object_or_interface) {
       ConstString class_name(clang_type.GetTypeName());
       if (class_name) {
         dwarf->GetObjCMethods(class_name, [&](DWARFDIE method_die) {
           method_die.ResolveType();
           return true;
void DWARFASTParserClang::ParseSingleMember(
 
   layout_info.field_offsets.insert(
       std::make_pair(field_decl, field_bit_offset));
 }
 
 bool DWARFASTParserClang::ParseChildMembers(
     const DWARFDIE &parent_die, CompilerType &class_clang_type,
     std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
-    std::vector<DWARFDIE> &member_function_dies,
+    std::vector<DWARFDIE> &member_function_and_type_dies,
     DelayedPropertyList &delayed_properties,
     const AccessType default_accessibility,
     ClangASTImporter::LayoutInfo &layout_info) {
   if (!parent_die)
     return false;
 
   FieldInfo last_field_info;
 
case DW_TAG_APPLE_property:
       break;
 
     case DW_TAG_member:
       ParseSingleMember(die, parent_die, class_clang_type,
                         default_accessibility, layout_info, last_field_info);
       break;
 
     case DW_TAG_subprogram:
+    case DW_TAG_enumeration_type:
+    case DW_TAG_structure_type:
+    case DW_TAG_union_type:
       // Let the type parsing code handle this one for us.
-      member_function_dies.push_back(die);
+      member_function_and_type_dies.push_back(die);
       break;
 
     case DW_TAG_inheritance:
       ParseInheritance(die, parent_die, class_clang_type, default_accessibility,
                        module_sp, base_classes, layout_info);
       break;
 
     default:
       break;
     }
   }
 
   return true;
 }
 
 size_t DWARFASTParserClang::ParseChildParameters(
     clang::DeclContext *containing_decl_ctx, const DWARFDIE &parent_die,
     bool skip_artificial, bool &is_static, bool &is_variadic,
     bool &has_template_params, std::vector<CompilerType> &function_param_types,