Reapply "[llvm/DWARF] Recursively resolve DW_AT_signature references"… #99495
Conversation
|
@llvm/pr-subscribers-debuginfo Author: Pavel Labath (labath) Changes… (#99444) The previous version introduced a bug (caught by cross-project tests). Explicit signature resolution is still necessary when one wants to access the children (not attributes) of a given DIE. In the new version of the patch, I keep the I've also extended the prettyprint_type_units test case to cover this scenario (and also reduced it to remove irrelevant details). Patch is 43.89 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/99495.diff 3 Files Affected:
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
index 72e7464b68971..7ae6af5dfe496 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
@@ -103,10 +103,6 @@ static void dumpLocationExpr(raw_ostream &OS, const DWARFFormValue &FormValue,
.print(OS, DumpOpts, U);
}
-static DWARFDie resolveReferencedType(DWARFDie D, DWARFFormValue F) {
- return D.getAttributeValueAsReferencedDie(F).resolveTypeUnitReference();
-}
-
static void dumpAttribute(raw_ostream &OS, const DWARFDie &Die,
const DWARFAttribute &AttrValue, unsigned Indent,
DIDumpOptions DumpOpts) {
@@ -198,8 +194,8 @@ static void dumpAttribute(raw_ostream &OS, const DWARFDie &Die,
DINameKind::LinkageName))
OS << Space << "\"" << Name << '\"';
} else if (Attr == DW_AT_type || Attr == DW_AT_containing_type) {
- DWARFDie D = resolveReferencedType(Die, FormValue);
- if (D && !D.isNULL()) {
+ if (DWARFDie D = Die.getAttributeValueAsReferencedDie(FormValue);
+ D && !D.isNULL()) {
OS << Space << "\"";
dumpTypeQualifiedName(D, OS);
OS << '"';
@@ -291,13 +287,12 @@ DWARFDie::findRecursively(ArrayRef<dwarf::Attribute> Attrs) const {
if (auto Value = Die.find(Attrs))
return Value;
- if (auto D = Die.getAttributeValueAsReferencedDie(DW_AT_abstract_origin))
- if (Seen.insert(D).second)
- Worklist.push_back(D);
-
- if (auto D = Die.getAttributeValueAsReferencedDie(DW_AT_specification))
- if (Seen.insert(D).second)
- Worklist.push_back(D);
+ for (dwarf::Attribute Attr :
+ {DW_AT_abstract_origin, DW_AT_specification, DW_AT_signature}) {
+ if (auto D = Die.getAttributeValueAsReferencedDie(Attr))
+ if (Seen.insert(D).second)
+ Worklist.push_back(D);
+ }
}
return std::nullopt;
@@ -319,6 +314,10 @@ DWARFDie::getAttributeValueAsReferencedDie(const DWARFFormValue &V) const {
} else if (Offset = V.getAsDebugInfoReference(); Offset) {
if (DWARFUnit *SpecUnit = U->getUnitVector().getUnitForOffset(*Offset))
Result = SpecUnit->getDIEForOffset(*Offset);
+ } else if (std::optional<uint64_t> Sig = V.getAsSignatureReference()) {
+ if (DWARFTypeUnit *TU = U->getContext().getTypeUnitForHash(
+ U->getVersion(), *Sig, U->isDWOUnit()))
+ Result = TU->getDIEForOffset(TU->getTypeOffset() + TU->getOffset());
}
return Result;
}
@@ -333,7 +332,6 @@ DWARFDie DWARFDie::resolveTypeUnitReference() const {
}
return *this;
}
-
std::optional<uint64_t> DWARFDie::getRangesBaseAttribute() const {
return toSectionOffset(find({DW_AT_rnglists_base, DW_AT_GNU_ranges_base}));
}
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFTypePrinter.cpp b/llvm/lib/DebugInfo/DWARF/DWARFTypePrinter.cpp
index a26431e8313f6..cc38bc6197eb8 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFTypePrinter.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFTypePrinter.cpp
@@ -62,17 +62,10 @@ void DWARFTypePrinter::appendArrayType(const DWARFDie &D) {
EndedWithTemplate = false;
}
-static DWARFDie resolveReferencedType(DWARFDie D,
- dwarf::Attribute Attr = DW_AT_type) {
- return D.getAttributeValueAsReferencedDie(Attr).resolveTypeUnitReference();
-}
-static DWARFDie resolveReferencedType(DWARFDie D, DWARFFormValue F) {
- return D.getAttributeValueAsReferencedDie(F).resolveTypeUnitReference();
-}
DWARFDie DWARFTypePrinter::skipQualifiers(DWARFDie D) {
while (D && (D.getTag() == DW_TAG_const_type ||
D.getTag() == DW_TAG_volatile_type))
- D = resolveReferencedType(D);
+ D = D.getAttributeValueAsReferencedDie(DW_AT_type);
return D;
}
@@ -103,7 +96,9 @@ DWARFTypePrinter::appendUnqualifiedNameBefore(DWARFDie D,
return DWARFDie();
}
DWARFDie InnerDIE;
- auto Inner = [&] { return InnerDIE = resolveReferencedType(D); };
+ auto Inner = [&] {
+ return InnerDIE = D.getAttributeValueAsReferencedDie(DW_AT_type);
+ };
const dwarf::Tag T = D.getTag();
switch (T) {
case DW_TAG_pointer_type: {
@@ -134,7 +129,8 @@ DWARFTypePrinter::appendUnqualifiedNameBefore(DWARFDie D,
OS << '(';
else if (Word)
OS << ' ';
- if (DWARFDie Cont = resolveReferencedType(D, DW_AT_containing_type)) {
+ if (DWARFDie Cont =
+ D.getAttributeValueAsReferencedDie(DW_AT_containing_type)) {
appendQualifiedName(Cont);
EndedWithTemplate = false;
OS << "::";
@@ -173,7 +169,8 @@ DWARFTypePrinter::appendUnqualifiedNameBefore(DWARFDie D,
case DW_TAG_base_type:
*/
default: {
- const char *NamePtr = dwarf::toString(D.find(DW_AT_name), nullptr);
+ const char *NamePtr =
+ dwarf::toString(D.findRecursively(DW_AT_name), nullptr);
if (!NamePtr) {
appendTypeTagName(D.getTag());
return DWARFDie();
@@ -235,9 +232,9 @@ void DWARFTypePrinter::appendUnqualifiedNameAfter(
case DW_TAG_pointer_type: {
if (needsParens(Inner))
OS << ')';
- appendUnqualifiedNameAfter(Inner, resolveReferencedType(Inner),
- /*SkipFirstParamIfArtificial=*/D.getTag() ==
- DW_TAG_ptr_to_member_type);
+ appendUnqualifiedNameAfter(
+ Inner, Inner.getAttributeValueAsReferencedDie(DW_AT_type),
+ /*SkipFirstParamIfArtificial=*/D.getTag() == DW_TAG_ptr_to_member_type);
break;
}
case DW_TAG_LLVM_ptrauth_type: {
@@ -326,7 +323,7 @@ bool DWARFTypePrinter::appendTemplateParameters(DWARFDie D,
bool IsTemplate = false;
if (!FirstParameter)
FirstParameter = &FirstParameterValue;
- for (const DWARFDie &C : D) {
+ for (const DWARFDie &C : D.resolveTypeUnitReference()) {
auto Sep = [&] {
if (*FirstParameter)
OS << '<';
@@ -341,7 +338,7 @@ bool DWARFTypePrinter::appendTemplateParameters(DWARFDie D,
appendTemplateParameters(C, FirstParameter);
}
if (C.getTag() == dwarf::DW_TAG_template_value_parameter) {
- DWARFDie T = resolveReferencedType(C);
+ DWARFDie T = C.getAttributeValueAsReferencedDie(DW_AT_type);
Sep();
if (T.getTag() == DW_TAG_enumeration_type) {
OS << '(';
@@ -461,7 +458,7 @@ bool DWARFTypePrinter::appendTemplateParameters(DWARFDie D,
continue;
auto TypeAttr = C.find(DW_AT_type);
Sep();
- appendQualifiedName(TypeAttr ? resolveReferencedType(C, *TypeAttr)
+ appendQualifiedName(TypeAttr ? C.getAttributeValueAsReferencedDie(*TypeAttr)
: DWARFDie());
}
if (IsTemplate && *FirstParameter && FirstParameter == &FirstParameterValue) {
@@ -473,15 +470,15 @@ bool DWARFTypePrinter::appendTemplateParameters(DWARFDie D,
void DWARFTypePrinter::decomposeConstVolatile(DWARFDie &N, DWARFDie &T,
DWARFDie &C, DWARFDie &V) {
(N.getTag() == DW_TAG_const_type ? C : V) = N;
- T = resolveReferencedType(N);
+ T = N.getAttributeValueAsReferencedDie(DW_AT_type);
if (T) {
auto Tag = T.getTag();
if (Tag == DW_TAG_const_type) {
C = T;
- T = resolveReferencedType(T);
+ T = T.getAttributeValueAsReferencedDie(DW_AT_type);
} else if (Tag == DW_TAG_volatile_type) {
V = T;
- T = resolveReferencedType(T);
+ T = T.getAttributeValueAsReferencedDie(DW_AT_type);
}
}
}
@@ -491,10 +488,11 @@ void DWARFTypePrinter::appendConstVolatileQualifierAfter(DWARFDie N) {
DWARFDie T;
decomposeConstVolatile(N, T, C, V);
if (T && T.getTag() == DW_TAG_subroutine_type)
- appendSubroutineNameAfter(T, resolveReferencedType(T), false, C.isValid(),
- V.isValid());
+ appendSubroutineNameAfter(T, T.getAttributeValueAsReferencedDie(DW_AT_type),
+ false, C.isValid(), V.isValid());
else
- appendUnqualifiedNameAfter(T, resolveReferencedType(T));
+ appendUnqualifiedNameAfter(T,
+ T.getAttributeValueAsReferencedDie(DW_AT_type));
}
void DWARFTypePrinter::appendConstVolatileQualifierBefore(DWARFDie N) {
DWARFDie C;
@@ -504,7 +502,7 @@ void DWARFTypePrinter::appendConstVolatileQualifierBefore(DWARFDie N) {
bool Subroutine = T && T.getTag() == DW_TAG_subroutine_type;
DWARFDie A = T;
while (A && A.getTag() == DW_TAG_array_type)
- A = resolveReferencedType(A);
+ A = A.getAttributeValueAsReferencedDie(DW_AT_type);
bool Leading =
(!A || (A.getTag() != DW_TAG_pointer_type &&
A.getTag() != llvm::dwarf::DW_TAG_ptr_to_member_type)) &&
@@ -546,7 +544,7 @@ void DWARFTypePrinter::appendSubroutineNameAfter(
if (P.getTag() != DW_TAG_formal_parameter &&
P.getTag() != DW_TAG_unspecified_parameters)
return;
- DWARFDie T = resolveReferencedType(P);
+ DWARFDie T = P.getAttributeValueAsReferencedDie(DW_AT_type);
if (SkipFirstParamIfArtificial && RealFirst && P.find(DW_AT_artificial)) {
FirstParamIfArtificial = T;
RealFirst = false;
@@ -567,7 +565,7 @@ void DWARFTypePrinter::appendSubroutineNameAfter(
if (DWARFDie P = FirstParamIfArtificial) {
if (P.getTag() == DW_TAG_pointer_type) {
auto CVStep = [&](DWARFDie CV) {
- if (DWARFDie U = resolveReferencedType(CV)) {
+ if (DWARFDie U = CV.getAttributeValueAsReferencedDie(DW_AT_type)) {
Const |= U.getTag() == DW_TAG_const_type;
Volatile |= U.getTag() == DW_TAG_volatile_type;
return U;
@@ -653,7 +651,8 @@ void DWARFTypePrinter::appendSubroutineNameAfter(
if (D.find(DW_AT_rvalue_reference))
OS << " &&";
- appendUnqualifiedNameAfter(Inner, resolveReferencedType(Inner));
+ appendUnqualifiedNameAfter(
+ Inner, Inner.getAttributeValueAsReferencedDie(DW_AT_type));
}
void DWARFTypePrinter::appendScopes(DWARFDie D) {
if (D.getTag() == DW_TAG_compile_unit)
@@ -666,7 +665,6 @@ void DWARFTypePrinter::appendScopes(DWARFDie D) {
return;
if (D.getTag() == DW_TAG_lexical_block)
return;
- D = D.resolveTypeUnitReference();
if (DWARFDie P = D.getParent())
appendScopes(P);
appendUnqualifiedName(D);
diff --git a/llvm/test/tools/llvm-dwarfdump/X86/prettyprint_type_units.s b/llvm/test/tools/llvm-dwarfdump/X86/prettyprint_type_units.s
index aad748a301e6b..0893889dbc259 100644
--- a/llvm/test/tools/llvm-dwarfdump/X86/prettyprint_type_units.s
+++ b/llvm/test/tools/llvm-dwarfdump/X86/prettyprint_type_units.s
@@ -1,403 +1,269 @@
# RUN: llvm-mc < %s -filetype obj -triple x86_64 -o - \
# RUN: | llvm-dwarfdump - | FileCheck %s
-# Significantly modified assembly generated from this source:
+# Hand-written assembly roughly equivalent to this source code:
#
# struct t1 { };
# struct t2 { };
# template<typename ...T>
-# void f1() { }
-# int main() {
-# f1<t1, t2>();
-# }
+# struct S {};
+# S<t1, t2, t1> s;
#
-# $ clang++-tot test.cpp -g -S -fdebug-types-section -gdwarf-5 -o test.5.s -fstandalone-debug
-#
-# I inserted a DWARFv4 copy of the first type unit ("t1") to replace the
-# DWARFv5 type unit - to test both v4 and v5 type unit support. This test
-# doesn't really need templates - two local variables would've sufficed
-# (anything that references the type units) but I was working on something else
-# and this seemed minimal enough.
+# To various scenarios, the test uses a mixture of DWARF v4 and v5 type units,
+# and of llvm and gcc styles of referring to them.
+
+# CHECK: DW_TAG_variable
+# CHECK-NEXT: DW_AT_name ("s")
+# CHECK-NEXT: DW_AT_type ({{.*}} "S<t1, t2, t1>")
+# CHECK: DW_TAG_template_type_parameter
+# CHECK-NEXT: DW_AT_type ({{.*}} "t1")
+# CHECK: DW_TAG_template_type_parameter
+# CHECK-NEXT: DW_AT_type ({{.*}} "t2")
+# CHECK: DW_TAG_template_type_parameter
+# CHECK-NEXT: DW_AT_type (0xdeadbeef00000001 "t1")
-# CHECK: DW_TAG_template_type_parameter
-# CHECK: DW_AT_type ({{.*}} "t1")
-# CHECK: DW_TAG_template_type_parameter
-# CHECK: DW_AT_type ({{.*}} "t2")
+.set S_sig, 0xdeadbeef00000000
+.set t1_sig, 0xdeadbeef00000001
+.set t2_sig, 0xdeadbeef00000002
- .text
- .file "test.cpp"
- .globl main # -- Begin function main
- .p2align 4, 0x90
- .type main,@function
-main: # @main
-.Lfunc_begin0:
- .file 0 "/usr/local/google/home/blaikie/dev/scratch" "test.cpp" md5 0xafee1e55f64a0b86063fa85c8c456dba
- .loc 0 5 0 # test.cpp:5:0
- .cfi_startproc
-# %bb.0: # %entry
- pushq %rbp
- .cfi_def_cfa_offset 16
- .cfi_offset %rbp, -16
- movq %rsp, %rbp
- .cfi_def_cfa_register %rbp
-.Ltmp0:
- .loc 0 6 3 prologue_end # test.cpp:6:3
- callq _Z2f1IJ2t12t2EEvv
- .loc 0 7 1 # test.cpp:7:1
- xorl %eax, %eax
- popq %rbp
- .cfi_def_cfa %rsp, 8
- retq
-.Ltmp1:
-.Lfunc_end0:
- .size main, .Lfunc_end0-main
- .cfi_endproc
- # -- End function
- .section .text._Z2f1IJ2t12t2EEvv,"axG",@progbits,_Z2f1IJ2t12t2EEvv,comdat
- .weak _Z2f1IJ2t12t2EEvv # -- Begin function _Z2f1IJ2t12t2EEvv
- .p2align 4, 0x90
- .type _Z2f1IJ2t12t2EEvv,@function
-_Z2f1IJ2t12t2EEvv: # @_Z2f1IJ2t12t2EEvv
-.Lfunc_begin1:
- .loc 0 4 0 # test.cpp:4:0
- .cfi_startproc
-# %bb.0: # %entry
- pushq %rbp
- .cfi_def_cfa_offset 16
- .cfi_offset %rbp, -16
- movq %rsp, %rbp
- .cfi_def_cfa_register %rbp
-.Ltmp2:
- .loc 0 4 13 prologue_end # test.cpp:4:13
- popq %rbp
- .cfi_def_cfa %rsp, 8
- retq
-.Ltmp3:
-.Lfunc_end1:
- .size _Z2f1IJ2t12t2EEvv, .Lfunc_end1-_Z2f1IJ2t12t2EEvv
- .cfi_endproc
- # -- End function
- .section .debug_types,"G",@progbits,14297044602779165170,comdat
- .long .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+ .section .debug_types,"G",@progbits,t1_sig,comdat
+.Ltu_begin0:
+ .long .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
.Ldebug_info_start0:
- .short 4 # DWARF version number
- .long .debug_abbrev # Offset Into Abbrev. Section
- .byte 8 # Address Size (in bytes)
- .quad -4149699470930386446 # Type Signature
- .long 30 # Type DIE Offset
- .byte 10 # Abbrev [1] 0x17:0x11 DW_TAG_type_unit
- .short 33 # DW_AT_language
- .long .Lline_table_start0 # DW_AT_stmt_list
- .byte 11 # Abbrev [2] 0x1e:0x9 DW_TAG_structure_type
- .byte 5 # DW_AT_calling_convention
- .long .Linfo_string6 # DW_AT_name
- .byte 1 # DW_AT_byte_size
- .byte 1 # DW_AT_decl_file
- .byte 1 # DW_AT_decl_line
- .byte 0 # End Of Children Mark
+ .short 4 # DWARF version number
+ .long .debug_abbrev # Offset Into Abbrev. Section
+ .byte 8 # Address Size (in bytes)
+ .quad t1_sig # Type Signature
+ .long .Lt1_def-.Ltu_begin0 # Type DIE Offset
+ .byte 10 # Abbrev [10] DW_TAG_type_unit
+ .short 33 # DW_AT_language
+.Lt1_def:
+ .byte 11 # Abbrev [11] DW_TAG_structure_type
+ .long .Linfo_string6 # DW_AT_name
+ .byte 1 # DW_AT_byte_size
+ .byte 0 # End Of Children Mark
.Ldebug_info_end0:
- .section .debug_info,"G",@progbits,5649318945901130368,comdat
- .long .Ldebug_info_end1-.Ldebug_info_start1 # Length of Unit
+ .section .debug_info,"G",@progbits,t2_sig,comdat
+.Ltu_begin1:
+ .long .Ldebug_info_end1-.Ldebug_info_start1 # Length of Unit
.Ldebug_info_start1:
- .short 5 # DWARF version number
- .byte 2 # DWARF Unit Type
- .byte 8 # Address Size (in bytes)
- .long .debug_abbrev # Offset Into Abbrev. Section
- .quad 5649318945901130368 # Type Signature
- .long 35 # Type DIE Offset
- .byte 1 # Abbrev [1] 0x18:0x12 DW_TAG_type_unit
- .short 33 # DW_AT_language
- .long .Lline_table_start0 # DW_AT_stmt_list
- .long .Lstr_offsets_base0 # DW_AT_str_offsets_base
- .byte 2 # Abbrev [2] 0x23:0x6 DW_TAG_structure_type
- .byte 5 # DW_AT_calling_convention
- .byte 7 # DW_AT_name
- .byte 1 # DW_AT_byte_size
- .byte 0 # DW_AT_decl_file
- .byte 2 # DW_AT_decl_line
- .byte 0 # End Of Children Mark
+ .short 5 # DWARF version number
+ .byte 2 # DWARF Unit Type
+ .byte 8 # Address Size (in bytes)
+ .long .debug_abbrev # Offset Into Abbrev. Section
+ .quad t2_sig # Type Signature
+ .long .Lt2_def-.Ltu_begin1 # Type DIE Offset
+ .byte 1 # Abbrev [1] 0x18:0x12 DW_TAG_type_unit
+ .short 33 # DW_AT_language
+ .long .Lstr_offsets_base0 # DW_AT_str_offsets_base
+.Lt2_def:
+ .byte 2 # Abbrev [2] 0x23:0x6 DW_TAG_structure_type
+ .byte 7 # DW_AT_name
+ .byte 1 # DW_AT_byte_size
+ .byte 0 # End Of Children Mark
.Ldebug_info_end1:
- .section .debug_abbrev,"",@progbits
- .byte 1 # Abbreviation Code
- .byte 65 # DW_TAG_type_unit
- .byte 1 # DW_CHILDREN_yes
- .byte 19 # DW_AT_language
- .byte 5 # DW_FORM_data2
- .byte 16 # DW_AT_stmt_list
- .byte 23 # DW_FORM_sec_offset
- .byte 114 # DW_AT_str_offsets_base
- .byte 23 # DW_FORM_sec_offset
- .byte 0 # EOM(1)
- .byte 0 # EOM(2)
- .byte 2 # Abbreviation Code
- .byte 19 # DW_TAG_structure_type
- .byte 0 # DW_CHILDREN_no
- .byte 54 # DW_AT_calling_convention
- .byte 11 # DW_FORM_data1
- .byte 3 # DW_AT_name
- .byte 37 # DW_FORM_strx1
- .byte 11 # DW_AT_byte_size
- .byte 11 # DW_FORM_data1
- .byte 58 # DW_AT_decl_file
- .byte 11 # DW_FORM_data1
- .byte 59 # DW_AT_decl_line
- .byte 11 # DW_FORM_data1
- .byte 0 # EOM(1)
- .byte 0 # EOM(2)
- .byte 3 # Abbreviation Code
- .byte 17 # DW_TAG_compile_unit
- .byte 1 # DW_CHILDREN_yes
- .byte 37 # DW_AT_producer
- .byte 37 # DW_FORM_strx1
- .byte 19 # DW_AT_language
- .byte 5 # DW_FORM_data2
- .byte 3 # DW_AT_name
- .byte 37 # DW_FORM_strx1
- .byte 114 # DW_AT_str_offsets_base
- .byte 23 # DW_FORM_sec_offset
- .byte 16 # DW_AT_stmt_list
- .byte 23 ...
[truncated]
|
| @@ -173,7 +169,8 @@ DWARFTypePrinter::appendUnqualifiedNameBefore(DWARFDie D, | |||
| case DW_TAG_base_type: | |||
| */ | |||
| default: { | |||
| const char *NamePtr = dwarf::toString(D.find(DW_AT_name), nullptr); | |||
| const char *NamePtr = | |||
| dwarf::toString(D.findRecursively(DW_AT_name), nullptr); | |||
There was a problem hiding this comment.
possibly if D is resolved through type unit references before this line, then this findRecursively won't be needed and the resolve through type units won't be needed in appendTemplateParameters either?
There was a problem hiding this comment.
That would work, but I think that basically amounts to reverting all the changes in this file (as that's what the original code was doing). Is that what you're suggesting?
(The reason I changed this file was to demonstrate how one can write code to access the type mostly without caring about type units. It's not really required to make the main change (have recursive resolution look through type unit references), but it makes me wonder whether it is a good idea.)
There was a problem hiding this comment.
Ah, I guess maybe it'd make sense to split this patch up. (haven't thought about whether these two changes are ordered or order independent)
The getAttributeValueAsReferencedDie accounting for type units is one thing, sounds good.
The "recurse through DW_AT_signature" to resolve some lookups is a separate thing - I think it still does make sense... maybe? It is a bit less efficient to have to lookup type units multiple times, compared to the abstract_origin/specification indirection which is at least just jumping to an offset (though I guess it's still a binary search through a list of DIEs, ultimately, to find the DIE with a given offset?).
I guess alternatively, we could make the children iteration do the same indirection too? Though doing that implicitly might be too much (like even the dumper would get confused, dumping the children of the skeleton type DIE, etc) so then it'd have to be a separate function...
I guess maybe my thoughts are that findRecursively, finding recursively explicitly is OK - but that maybe it doesn't suit the use case here in the type printer (because it's comprehensively visiting lots of stuff - whereas just "give me the name of this thing" or "give me the file/line this thing was defined on" is a relatively more common/mild query that's nice to be able to do more conveniently)
Hmm - when/where/how does /this/ function (appendUnqualifiedNameBefore) get called on an unresolved type? Perhaps this should be pushed further up... like every call to "get a DIE from this DW_AT_type" is already looking through to the type unit, what's left/how else does this code end up dealing with a skeleton type unit? (I guess maybe in top level callers - perhaps that needs an explicit entry point that resolves before going into the generic walking code)
There was a problem hiding this comment.
Ah, I guess maybe it'd make sense to split this patch up. (haven't thought about whether these two changes are ordered or order independent)
The
getAttributeValueAsReferencedDieaccounting for type units is one thing, sounds good.
Sounds good. I'll create a patch for that. In fact, I think lldb already has this part.
The "recurse through DW_AT_signature" to resolve some lookups is a separate thing - I think it still does make sense... maybe? It is a bit less efficient to have to lookup type units multiple times, compared to the abstract_origin/specification indirection which is at least just jumping to an offset (though I guess it's still a binary search through a list of DIEs, ultimately, to find the DIE with a given offset?).
It is, and I guess that the direct lookup would always be faster, though maybe not it a way that we care.
I guess alternatively, we could make the children iteration do the same indirection too? Though doing that implicitly might be too much (like even the dumper would get confused, dumping the children of the skeleton type DIE, etc) so then it'd have to be a separate function...
Yeah, that sound like too much. We don't do that for DW_AT_specification either.
I guess maybe my thoughts are that findRecursively, finding recursively explicitly is OK - but that maybe it doesn't suit the use case here in the type printer (because it's comprehensively visiting lots of stuff - whereas just "give me the name of this thing" or "give me the file/line this thing was defined on" is a relatively more common/mild query that's nice to be able to do more conveniently)
I can definitely understand that position.
Hmm - when/where/how does /this/ function (
appendUnqualifiedNameBefore) get called on an unresolved type? Perhaps this should be pushed further up... like every call to "get a DIE from this DW_AT_type" is already looking through to the type unit, what's left/how else does this code end up dealing with a skeleton type unit? (I guess maybe in top level callers - perhaps that needs an explicit entry point that resolves before going into the generic walking code)
I think this can happen anywhere where we have a double reference (so when a DW_AT_type points to a declaration, which points (via DW_AT_signature) to a type unit) (*). getAttributeValueAsReferencedDie will only resolve the first reference, and not the second one. Unless we want to teach it to resolve references recursively (which sounds like a bad idea), we're left with explicitly calling some type-unit resolving function at every call site, which sounds a lot like what resolveReferencedType already is.
So maybe the verdict is:
- getAttributeValueAsReferencedDie should resolve type unit references
- findRecursively should do that as well
- we should leave type printer alone?
(*) This also means that this problem doesn't occur with gcc-style type unit references, which I'm really starting to like. :)
There was a problem hiding this comment.
Aside: GCC doesn't use this style for all type unit references, it actually uses 3 different styles depending on context
https://godbolt.org/z/Pozc4TYM1
t1, type unit referenced once DW_AT_type [DW_FORM_ref_sig8] (0xf8473fd3bfd1e564)
t2, type unit referenced twice (save space by sharing the sig8): DW_AT_type [DW_FORM_ref4] (cu + 0x00cc => {0x000000cc} "t2")
0x000000cc: DW_TAG_structure_type [14] (0x0000000c)
DW_AT_signature [DW_FORM_ref_sig8] (0x3fea40f8df132361)
t3: referenced a declaration of a member function:
0x000000b3: DW_TAG_subprogram [13] (0x0000000c)
DW_AT_specification [DW_FORM_ref4] (cu + 0x0049 => {0x00000049} "_ZN2ns2t33memEv")
DW_AT_decl_line [DW_FORM_data1] (9)
DW_AT_decl_column [DW_FORM_data1] (6)
DW_AT_low_pc [DW_FORM_addr] (0x0000000000000000 ".text")
DW_AT_high_pc [DW_FORM_data8] (0x0000000000000007)
DW_AT_frame_base [DW_FORM_exprloc] (DW_OP_call_frame_cfa)
DW_AT_call_all_calls [DW_FORM_flag_present] (true)
0x0000002e: DW_TAG_namespace [10] * (0x0000000c)
DW_AT_name [DW_FORM_string] ("ns")
DW_AT_decl_file [DW_FORM_data1] ("/app/example.cpp")
DW_AT_decl_line [DW_FORM_data1] (1)
DW_AT_decl_column [DW_FORM_data1] (11)
DW_AT_sibling [DW_FORM_ref4] (cu + 0x0086 => {0x00000086})
0x00000039: DW_TAG_structure_type [11] * (0x0000002e)
DW_AT_name [DW_FORM_string] ("t3")
DW_AT_declaration [DW_FORM_flag_present] (true)
DW_AT_signature [DW_FORM_ref_sig8] (0x55d7e46295b56ec8)
DW_AT_sibling [DW_FORM_ref4] (cu + 0x0056 => {0x00000056})
0x00000049: DW_TAG_subprogram [5] (0x00000039)
DW_AT_external [DW_FORM_flag_present] (true)
DW_AT_name [DW_FORM_string] ("mem")
DW_AT_decl_file [DW_FORM_data1] ("/app/example.cpp")
DW_AT_decl_line [DW_FORM_data1] (4)
DW_AT_decl_column [DW_FORM_data1] (25)
DW_AT_linkage_name [DW_FORM_strp] ( .debug_str[0x00000091] = "_ZN2ns2t33memEv")
DW_AT_declaration [DW_FORM_flag_present] (true)
In any case...
It seems to me that perhaps we could/should introduce a getAttributeValueAsReferencedDieRecursively? That recurses the referenced type, but doesn't recurse the attribute (since this is called on an already found attribute - with find or findRecursively)?
getAttributeValueAsReferencedDie will only resolve the first reference, and not the second one. Unless we want to teach it to resolve references recursively (which sounds like a bad idea)
Hmm, you mentioned that this sounds like a bad idea, but then mention that we should do it a few lines later? Not sure which you're thinking of, or why it might be a bad idea? (I mean, it might be, not suggesting it isn't, but trying to understand better)
Or perhaps you were presenting these as alternatives...
There was a problem hiding this comment.
Aside: GCC doesn't use this style for all type unit references, it actually uses 3 different styles depending on context
Okay, not so nice then, I guess :)
It seems to me that perhaps we could/should introduce a
getAttributeValueAsReferencedDieRecursively? That recurses the referenced type, but doesn't recurse the attribute (since this is called on an already found attribute - with find or findRecursively)?
This seems a bit too magical to me. getAttributeValueAsReferencedDieRecursively() is not that much shorter than getAttributeValueAsReferencedDie().resolveTypeUnitReference(), but the latter is clearer in what's happening (and I think this will need to be evaluated on a case-by-case basis, as depending on what exactly you want to do (e.g. which children to look at) you might or might not want to do the "recursive" part).
I think it would make sense if it were named somewhat differently to so that it's clear this is used for types and that it will point you to some kind of a canonical type definition... oops, I think i've just reinvented the existing
static DWARFDie resolveReferencedType(DWARFDie D, DWARFFormValue F) {
return D.getAttributeValueAsReferencedDie(F).resolveTypeUnitReference();
}
getAttributeValueAsReferencedDie will only resolve the first reference, and not the second one. Unless we want to teach it to resolve references recursively (which sounds like a bad idea)
Hmm, you mentioned that this sounds like a bad idea, but then mention that we should do it a few lines later? Not sure which you're thinking of, or why it might be a bad idea? (I mean, it might be, not suggesting it isn't, but trying to understand better)
Or perhaps you were presenting these as alternatives...
I'm sorry, I was being imprecise here. I think that the recursive reference resolution is a bad idea. The rest of the text omits the word "recursive", which was meant to mean that it should handle a single DW_FORM_ref_sig8 reference (and nothing more)
- getAttributeValueAsReferencedDie should resolve type unit references
It should resolve a single DW_FORM_ref_sig8 like it does in this patch
- findRecursively should do that as well
It should recurse on DW_AT_signature (like it does in this patch), by calling getAttributeValueAsReferencedDie
- we should leave type printer alone?
just that
There was a problem hiding this comment.
It seems to me that perhaps we could/should introduce a
getAttributeValueAsReferencedDieRecursively? That recurses the referenced type, but doesn't recurse the attribute (since this is called on an already found attribute - with find or findRecursively)?This seems a bit too magical to me.
getAttributeValueAsReferencedDieRecursively()is not that much shorter thangetAttributeValueAsReferencedDie().resolveTypeUnitReference(), but the latter is clearer in what's happening (and I think this will need to be evaluated on a case-by-case basis, as depending on what exactly you want to do (e.g. which children to look at) you might or might not want to do the "recursive" part).I think it would make sense if it were named somewhat differently to so that it's clear this is used for types and that it will point you to some kind of a canonical type definition... oops, I think i've just reinvented the existing
static DWARFDie resolveReferencedType(DWARFDie D, DWARFFormValue F) { return D.getAttributeValueAsReferencedDie(F).resolveTypeUnitReference(); }
Fair enough - though if there's more places that want this functionality, I'd be fine with elevating it to some more general purpose helper on DWARFDie rather than hidden as a static helper in DWARFTypePrinter.
getAttributeValueAsReferencedDie will only resolve the first reference, and not the second one. Unless we want to teach it to resolve references recursively (which sounds like a bad idea)
Hmm, you mentioned that this sounds like a bad idea, but then mention that we should do it a few lines later? Not sure which you're thinking of, or why it might be a bad idea? (I mean, it might be, not suggesting it isn't, but trying to understand better)
Or perhaps you were presenting these as alternatives...I'm sorry, I was being imprecise here. I think that the recursive reference resolution is a bad idea. The rest of the text omits the word "recursive", which was meant to mean that it should handle a single DW_FORM_ref_sig8 reference (and nothing more)
Ah, OK.
- getAttributeValueAsReferencedDie should resolve type unit references
It should resolve a single DW_FORM_ref_sig8 like it does in this patch
- findRecursively should do that as well
It should recurse on DW_AT_signature (like it does in this patch), by calling getAttributeValueAsReferencedDie
- we should leave type printer alone?
just that
Yeah, that sounds OK to me. Thanks for your patience/giving this a go! (& if you are touching the lldb side of this sort of change too - it'd be handy to have similar type resolution helpers (ideally /really/ similar, because Real Soon Now, we'd like the DWARFTypePrinter to be generalized across both DWARFDie and DWARFDIE), so thanks for keeping this in mind (: )
There was a problem hiding this comment.
This is ready for review now. I'm sorry it took me so long to get back to this.
…_sig8 Splitting from llvm#99495. I've extended the type unit test case to feature more kinds of references, including the gcc-style DW_AT_type[DW_FORM_ref_sig8] reference, which this patch fixes.
…llvm#99444) The previous version introduced a bug (caught by cross-project tests). Explicit signature resolution is still necessary when one wants to access the children (not attributes) of a given DIE. The new version keeps just the findRecursively extension, and reverts all the DWARFTypePrinter modifications.
labath
force-pushed
the
signature-llvm
branch
from
ad42e60 to
4147f18
Compare
This allows e.g. DWARFDIE::GetName() to return the name of the type when looking at its declaration (which contains only DW_AT_declaration+DW_AT_signature). This is similar to how we recurse through DW_AT_specification when looking for a function name. Llvm dwarf parser has obtained the same functionality through llvm#99495. This fixes a bug where we would confuse a type like NS::Outer::Struct with NS::Struct (because NS::Outer (and its name) was in a type unit).
…107241) This allows e.g. DWARFDIE::GetName() to return the name of the type when looking at its declaration (which contains only DW_AT_declaration+DW_AT_signature). This is similar to how we recurse through DW_AT_specification when looking for a function name. Llvm dwarf parser has obtained the same functionality through #99495. This fixes a bug where we would confuse a type like NS::Outer::Struct with NS::Struct (because NS::Outer (and its name) was in a type unit).
… (#99444)
The previous version introduced a bug (caught by cross-project tests). Explicit signature resolution is still necessary when one wants to access the children (not attributes) of a given DIE.
The new version keeps just the findRecursively extension, and reverts all the DWARFTypePrinter modifications.