Skip to content

[DebugInfo] Fix bound generic types debug info generation #71939

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.

Sign up for GitHub

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
Mar 2, 2024
Merged

[DebugInfo] Fix bound generic types debug info generation #71939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions lib/IRGen/IRGenDebugInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1184,11 +1184,10 @@ class IRGenDebugInfoImpl : public IRGenDebugInfo {
return OpaqueType;
}

auto *opaqueType = createOpaqueStructWithSizedContainer(
Scope, Decl ? Decl->getNameStr() : "", File, Line, SizeInBits,
AlignInBits, Flags, MangledName, collectGenericParams(Type),
UnsubstitutedType);
return opaqueType;
auto *OpaqueType = createOpaqueStruct(
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this code doesn't look into the parent type of the nominal type at all, so for example if you have

struct Outer<T> {
  struct InnerGeneric<U> {}
  struct Inner {}
}

The type Outer<Int>.Inner is a StructType and not a BoundGenericStructType, but it is specialized (isSpecialized() returns true) and getParent() returns the BoundGenericStructType Outer<Int>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case "opaque type" is in regards to the debug info representation of the type. An opaque entry is one that doesn't contain any information about the type's fields, only some basic information about the type (like name, size, alignment, etc). The compiler emits complete debug info for the unsubstituted generic type, and then an opaque entry for each substituted type.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case why does the substituted type need to record generic arguments at all? Either you need the complete set and you should walk into the parent type, or you don't need them at all. Just collecting the innermost arguments doesn't seem to make much sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Today we collect them to ensure they survive in the debug info when producing a dSYM. dsymutil may delete any debug info that is unreachable when stitching together debug info from multiple .o files, so if a type happens to only be used as a generic argument, it would be deleted from the debug info.

In the future we might also stop emitting the substituted type's mangled name, and produce it by combining the unsubstituted generic type with the generic arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Today we collect them to ensure they survive in the debug info when producing a dSYM.

It seems that by the same reasoning you should collect the parent type’s generic arguments too then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see what you mean now. You're right, this doesn't address an inner specialized non-generic type at all. I'll address that in a follow up patch.

Scope, "", File, Line, SizeInBits, AlignInBits, Flags, MangledName,
collectGenericParams(Type), UnsubstitutedType);
return OpaqueType;
}

/// Create debug information for an enum with a raw type (enum E : Int {}).
Expand Down Expand Up @@ -1624,12 +1623,19 @@ class IRGenDebugInfoImpl : public IRGenDebugInfo {
llvm::DICompositeType *
createOpaqueStruct(llvm::DIScope *Scope, StringRef Name, llvm::DIFile *File,
unsigned Line, unsigned SizeInBits, unsigned AlignInBits,
llvm::DINode::DIFlags Flags, StringRef MangledName) {
return DBuilder.createStructType(
llvm::DINode::DIFlags Flags, StringRef MangledName,
llvm::DINodeArray BoundParams = {},
llvm::DIType *SpecificationOf = nullptr) {

auto StructType = DBuilder.createStructType(
Scope, Name, File, Line, SizeInBits, AlignInBits, Flags,
/* DerivedFrom */ nullptr,
DBuilder.getOrCreateArray(ArrayRef<llvm::Metadata *>()),
llvm::dwarf::DW_LANG_Swift, nullptr, MangledName);
llvm::dwarf::DW_LANG_Swift, nullptr, MangledName, SpecificationOf);

if (BoundParams)
DBuilder.replaceArrays(StructType, nullptr, BoundParams);
return StructType;
}

bool shouldCacheDIType(llvm::DIType *DITy, DebugTypeInfo &DbgTy) {
Expand Down Expand Up @@ -2012,11 +2018,9 @@ class IRGenDebugInfoImpl : public IRGenDebugInfo {
// Force the creation of the unsubstituted type, don't create it
// directly so it goes through all the caching/verification logic.
auto unsubstitutedDbgTy = getOrCreateType(DbgTy);
DBuilder.retainType(unsubstitutedDbgTy);
return createOpaqueStructWithSizedContainer(
Scope, Decl->getName().str(), L.File, FwdDeclLine, SizeInBits,
AlignInBits, Flags, MangledName, collectGenericParams(EnumTy),
unsubstitutedDbgTy);
return createOpaqueStruct(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark here about generic arguments in the parent type

Scope, "", L.File, FwdDeclLine, SizeInBits, AlignInBits, Flags,
MangledName, collectGenericParams(EnumTy), unsubstitutedDbgTy);
}
return createOpaqueStructWithSizedContainer(
Scope, Decl->getName().str(), L.File, FwdDeclLine, SizeInBits,
Expand Down
5 changes: 4 additions & 1 deletion test/DebugInfo/BoundGenericStruct.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@ public let s = S<Int>(t: 0)
// CHECK: ![[INT]] = !DICompositeType(tag: DW_TAG_structure_type, name: "$sSiD",


// DWARF: !DICompositeType(tag: DW_TAG_structure_type, name: "$s18BoundGenericStruct1SVySiGD",
// DWARF: !DICompositeType(tag: DW_TAG_structure_type,
// DWARF-SAME: templateParams: ![[PARAMS:[0-9]+]]
// DWARF-SAME: identifier: "$s18BoundGenericStruct1SVySiGD"
// DWARF-SAME: specification_of:
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for just noticing this now, but this field should really be called specification. Otherwise it sounds like "this is the specification of:", which is the exact opposite.


// DWARF: ![[PARAMS]] = !{![[INTPARAM:[0-9]+]]}
// DWARF: ![[INTPARAM]] = !DITemplateTypeParameter(type: ![[INT:[0-9]+]])
// DWARF: ![[INT]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Int", {{.*}}identifier: "$sSiD"
Expand Down