[DebugInfo] No longer assign non-fixed-size types the size of a pointer#87569
[DebugInfo] No longer assign non-fixed-size types the size of a pointer#87569Teemperor merged 1 commit intoswiftlang:mainfrom
Conversation
|
@swift-ci please test |
|
|
||
| extension Slab: Copyable where Element: Copyable {} | ||
|
|
||
| // CHECK-DAG: !DICompositeType({{.*}}name: "$sxq_BVD" |
There was a problem hiding this comment.
I'm not sure if this change is an improvement or not, but the fact that we emit the new name is actually because we his the code path for the specific type here:
case TypeKind::BuiltinFixedArray: {
if (Opts.DebugInfoLevel > IRGenDebugInfoLevel::ASTTypes) {
// DISABLED IN THE TEST
}
unsigned FwdDeclLine = 0;
return createOpaqueStruct(Scope, "Builtin.FixedArray", MainFile, // << HERE
FwdDeclLine, SizeInBits, AlignInBits, Flags,
MangledName);
}
| // If we do not know the size of this type, pass a size of 0. | ||
| // FIXME: createEnumType should not require a sized debug info type. | ||
| if (!CompletedDbgTy) | ||
| CompletedDbgTy = CompletedDebugTypeInfo::get(DbgTy, 0); |
There was a problem hiding this comment.
I guess your FIXME makes that clear already, but this defeats the purpose of CompletedDebugTypeInfo. Why can't we call createOpaqueStructWithSizedContainer() here any more?
There was a problem hiding this comment.
Tbh, I just want to keep this change as uninvasive as possible just so we can unblock main. Maybe we could call this, but then even more stuff breaks.
lib/IRGen/IRGenDebugInfo.cpp
Outdated
| const bool RequiresSize = | ||
| !isa<PrimaryArchetypeType>(DbgTy.getType()) && | ||
| !DbgTy.getType()->hasArchetype() && | ||
| !isa<TypeAliasType>(DbgTy.getType()); |
test/DebugInfo/value-generics.swift
Outdated
| extension Slab: Copyable where Element: Copyable {} | ||
|
|
||
| // CHECK-DAG: !DICompositeType({{.*}}name: "$sxq_BVD" | ||
| // CHECK-DAG: !DICompositeType({{.*}}name: "Builtin.FixedArray" |
There was a problem hiding this comment.
I think that is fine, but can you CHECK for the mangled name instead (or in addition)? The mangled names are more important for LLDB's functionality than the human-readable ones.
…entation Swift commit fcbebc5 added support for emitting Builtin.FixedArray as a DW_TAG_array_type. With swiftlang/swift#87569 this code path is now also taken for `Builtin.FixedArray<A, B>` which is the non-substituted `FixedArray` class. Before PR llvm#87569 the DWARF emitted for `FixedArray<A, B>` looked like this: DW_TAG_structure_type DW_AT_name ("$exq_BVD") DW_AT_linkage_name ("$exq_BVD") DW_AT_declaration (true) DW_AT_APPLE_runtime_class (DW_LANG_Swift) and the new DWARF that is emitted now takes the FixedArray code path which produces this DWARF: DW_TAG_array_type DW_AT_type (0x00006c70 "$eq_D") DW_TAG_structure_type DW_AT_name ("$eq_D") DW_AT_byte_size (0x00) DW_AT_APPLE_runtime_class (DW_LANG_Swift) The patch adds support for parsing this specific pattern. We curiously don't need to ever parse the substituted array types for embedded Swift, so this patch just adds enough support for the unsubstituted type.
| // then only emit a forward declaration. | ||
| // TODO: This check can probably be simplified and generalized. | ||
| const bool RequiresSize = !isa<PrimaryArchetypeType>(DbgTy.getType()) && | ||
| !DbgTy.getType()->hasArchetype() && |
There was a problem hiding this comment.
If the archetype is constrained to AnyObject (a class) the size is known.
struct W<T: AnyObject> {
let t: T
}
I'm not sure if there are other cases, I think not.
There was a problem hiding this comment.
I guess constraining T to a class directly, a protocol that is itself constrained to any object. and maybe @objc protocols too.
There was a problem hiding this comment.
I feel like this check (i.e., does this type have a fixed size) should already exist somewhere in the compiler and we should reuse it, but I don't know what is the right function here.
There was a problem hiding this comment.
TypeInfo::IsFixedSize should do it, unless you don't have access to a TypeInfo here?
augusto2112
left a comment
There was a problem hiding this comment.
This is a nice change.
Like I mentioned on swiftlang/llvm-project#12571 though I think that you should figure out why FixedArrays are being emitted as $eq_D first.
augusto2112
left a comment
There was a problem hiding this comment.
Actually I think this change is correct. I commented on swiftlang/llvm-project#12571 what I think needs to be done to fix the FixedArray change.
…entation Swift commit fcbebc5 added support for emitting Builtin.FixedArray as a DW_TAG_array_type. With swiftlang/swift#87569 this code path is now also taken for `Builtin.FixedArray<A, B>` which is the non-substituted `FixedArray` class. Before PR llvm#87569 the DWARF emitted for `FixedArray<A, B>` looked like this: DW_TAG_structure_type DW_AT_name ("$exq_BVD") DW_AT_linkage_name ("$exq_BVD") DW_AT_declaration (true) DW_AT_APPLE_runtime_class (DW_LANG_Swift) and the new DWARF that is emitted now takes the FixedArray code path which produces this DWARF: DW_TAG_array_type DW_AT_type (0x00006c70 "$eq_D") DW_TAG_structure_type DW_AT_name ("$eq_D") DW_AT_byte_size (0x00) DW_AT_APPLE_runtime_class (DW_LANG_Swift) The patch adds support for parsing this specific pattern. We curiously don't need to ever parse the substituted array types for embedded Swift, so this patch just adds enough support for the unsubstituted type.
`CompletedDebugTypeInfo::getFromTypeInfo` currently uses the storage type as
the preferred source of size information.
For address-only types (and any other type that has no fixed size at compile
time), the storage type is always changed to a pointer.
This currently causes that some non-fixed-size types (e.g., generics)
are assigned the size of a pointer as their actual size in the generated
debug information.
For example:
```
protocol P {
associatedtype S
}
func foo<B: P>(_ b: B) {
let x: (aaa: Int, bbb: B.S?) = (aaa: 0, bbb: nil)
_ = x
let y: (Fx: Int, Fy: Int) = (Fx: 0, Fy: 0)
_ = y
}
```
results in the following DWARF output where the first struct
is given size 8 (= the size of a pointer).
```
0x000000d4: DW_TAG_structure_type
DW_AT_name ("$sSi3aaa_1S4main1PPQzSg3bbbtD")
DW_AT_linkage_name ("$sSi3aaa_1S4main1PPQzSg3bbbtD")
DW_AT_byte_size (0x08)
DW_AT_APPLE_runtime_class (DW_LANG_Swift)
0x000000f3: DW_TAG_structure_type
DW_AT_name ("$sSi2Fx_Si2FytD")
DW_AT_linkage_name ("$sSi2Fx_Si2FytD")
DW_AT_byte_size (0x10)
DW_AT_APPLE_runtime_class (DW_LANG_Swift)
```
This change modifies `CompletedDebugTypeInfo::getFromTypeInfo` to ignore the
storage size for types that do not have fixed size. Generics now have no
specified size or 0 in DWARF. FixedArray<A, B> without substituted type/size
is now also emitted using the dedicated FixedArray code.
|
@swift-ci please test and merge |
|
@swift-ci please test |
…entation Swift commit fcbebc5 added support for emitting Builtin.FixedArray as a DW_TAG_array_type. With swiftlang/swift#87569 this code path is now also taken for `Builtin.FixedArray<A, B>` which is the non-substituted `FixedArray` class. Before PR llvm#87569 the DWARF emitted for `FixedArray<A, B>` looked like this: DW_TAG_structure_type DW_AT_name ("$exq_BVD") DW_AT_linkage_name ("$exq_BVD") DW_AT_declaration (true) DW_AT_APPLE_runtime_class (DW_LANG_Swift) and the new DWARF that is emitted now takes the FixedArray code path which produces this DWARF: DW_TAG_array_type DW_AT_type (0x00006c70 "$eq_D") DW_TAG_structure_type DW_AT_name ("$eq_D") DW_AT_byte_size (0x00) DW_AT_APPLE_runtime_class (DW_LANG_Swift) The patch adds support for parsing this specific pattern. We curiously don't need to ever parse the substituted array types for embedded Swift, so this patch just adds enough support for the unsubstituted type. (cherry picked from commit 1e1af53)
|
@swift-ci please test |
|
@swift-ci smoke test macOS |
CompletedDebugTypeInfo::getFromTypeInfocurrently uses the storage type asthe preferred source of size information.
For address-only types (and any other type that has no fixed size at compile
time), the storage type is always changed to a pointer.
This currently causes that some non-fixed-size types (e.g., generics)
are assigned the size of a pointer as their actual size in the generated
debug information.
For example:
results in the following DWARF output where the first struct
is given size 8 (= the size of a pointer).
This change modifies
CompletedDebugTypeInfo::getFromTypeInfoto ignore thestorage size for types that do not have fixed size. Generics now have no
specified size or 0 in DWARF. FixedArray<A, B> without substituted type/size
is now also emitted using the dedicated FixedArray code.