Skip to content
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

[llvm/DWARF] Recursively resolve DW_AT_signature references #97423

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

labath
Copy link
Collaborator

@labath labath commented Jul 2, 2024

findRecursively follows DW_AT_specification and DW_AT_abstract_origin references, but not DW_AT_signature. As far as I can tell, there is no fundamental difference between these attributes that would make this behavior desirable, and this just seems like a consequence of the fact that this attribute is newer. This patch aims to change that.

The motivation is some code in lldb, which assumes that it can construct a qualified name of a type by just walking the parent chain and looking at the name attribute. This works for "regular" debug info, even when some of the DIEs are just forward declarations, but it breaks in the presence of type units, because of the need to explicitly resolve the signature reference.

While LLDB does not use the llvm's DWARFDie class (yet?), this seems like a very important change in the overall API, and any divergence here would complicate eventual reunification, which is why I am making the change in the llvm API first. However, putting lldb aside, I think this change is beneficial in llvm on its own, as it allows us to remove the explicit DW_AT_signature resolution in the DWARFTypePrinter.

findRecursively follows DW_AT_specification and DW_AT_abstract_origin
references, but not DW_AT_signature. As far as I can tell, there is no
fundamental difference between these attributes that would make this
behavior desirable, and this just seems like a consequence of the fact
that this attribute is newer. This patch aims to change that.

The motivation is some code in lldb, which assumes that it can construct
a qualified name of a type by just walking the parent chain and looking
at the name attribute. This works for "regular" debug info, even when
some of the DIEs are just forward declarations, but it breaks in the
presence of type units, because of the need to explicitly resolve the
signature reference.

While LLDB does not use the llvm's DWARFDie class (yet?), this seems
like a very important change in the overall API, and any divergance here
would complicate eventual reunification, which is why I am making the
change in the llvm API first. Nonetheless, I think this change is
beneficial in llvm as well, as it allows us to remove the explicit
DW_AT_signature resolution in the DWARFTypePrinter.
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 2, 2024

@llvm/pr-subscribers-debuginfo

Author: Pavel Labath (labath)

Changes

findRecursively follows DW_AT_specification and DW_AT_abstract_origin references, but not DW_AT_signature. As far as I can tell, there is no fundamental difference between these attributes that would make this behavior desirable, and this just seems like a consequence of the fact that this attribute is newer. This patch aims to change that.

The motivation is some code in lldb, which assumes that it can construct a qualified name of a type by just walking the parent chain and looking at the name attribute. This works for "regular" debug info, even when some of the DIEs are just forward declarations, but it breaks in the presence of type units, because of the need to explicitly resolve the signature reference.

While LLDB does not use the llvm's DWARFDie class (yet?), this seems like a very important change in the overall API, and any divergence here would complicate eventual reunification, which is why I am making the change in the llvm API first. However, putting lldb aside, I think this change is beneficial in llvm on its own, as it allows us to remove the explicit DW_AT_signature resolution in the DWARFTypePrinter.


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

3 Files Affected:

  • (modified) llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h (-2)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFDie.cpp (+15-28)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFTypePrinter.cpp (+25-27)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h
index 421b84d644db6..497d3bee048ab 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h
@@ -181,8 +181,6 @@ class DWARFDie {
   DWARFDie getAttributeValueAsReferencedDie(dwarf::Attribute Attr) const;
   DWARFDie getAttributeValueAsReferencedDie(const DWARFFormValue &V) const;
 
-  DWARFDie resolveTypeUnitReference() const;
-
   /// Extract the range base attribute from this DIE as absolute section offset.
   ///
   /// This is a utility function that checks for either the DW_AT_rnglists_base
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
index 410842a80b015..2cc5b416fe1a2 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;
@@ -312,27 +307,19 @@ DWARFDie::getAttributeValueAsReferencedDie(dwarf::Attribute Attr) const {
 
 DWARFDie
 DWARFDie::getAttributeValueAsReferencedDie(const DWARFFormValue &V) const {
-  DWARFDie Result;
   if (auto SpecRef = V.getAsRelativeReference()) {
     if (SpecRef->Unit)
-      Result = SpecRef->Unit->getDIEForOffset(SpecRef->Unit->getOffset() +
-                                              SpecRef->Offset);
-    else if (auto SpecUnit =
-                 U->getUnitVector().getUnitForOffset(SpecRef->Offset))
-      Result = SpecUnit->getDIEForOffset(SpecRef->Offset);
-  }
-  return Result;
-}
-
-DWARFDie DWARFDie::resolveTypeUnitReference() const {
-  if (auto Attr = find(DW_AT_signature)) {
-    if (std::optional<uint64_t> Sig = Attr->getAsReferenceUVal()) {
+      return SpecRef->Unit->getDIEForOffset(SpecRef->Unit->getOffset() +
+                                            SpecRef->Offset);
+    if (V.getForm() == dwarf::DW_FORM_ref_sig8) {
       if (DWARFTypeUnit *TU = U->getContext().getTypeUnitForHash(
-              U->getVersion(), *Sig, U->isDWOUnit()))
+              U->getVersion(), SpecRef->Offset, U->isDWOUnit()))
         return TU->getDIEForOffset(TU->getTypeOffset() + TU->getOffset());
     }
+    if (auto *SpecUnit = U->getUnitVector().getUnitForOffset(SpecRef->Offset))
+      return SpecUnit->getDIEForOffset(SpecRef->Offset);
   }
-  return *this;
+  return {};
 }
 
 std::optional<uint64_t> DWARFDie::getRangesBaseAttribute() const {
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFTypePrinter.cpp b/llvm/lib/DebugInfo/DWARF/DWARFTypePrinter.cpp
index a26431e8313f6..fc1aae77a9293 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: {
@@ -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);

@labath
Copy link
Collaborator Author

labath commented Jul 11, 2024

Ping? What do you think about this approach? Or do you know of someone who can review this?

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

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

Thanks. This seems a nice refactoring as well as a simplification.

Only few relevant tests are llvm/test/tools/llvm-dwarfdump/X86/prettyprint_type*.s

I followed the control flow in a debugger and this PR seems reasonable.

DW_TAG_GNU_template_parameter_pack
  DW_TAG_template_type_parameter
    DW_AT_type [DW_FORM_ref4]   (cu + 0x0058 => {0x00000058} "t1")

=>

DW_TAG_compile_unit
  DW_TAG_structure_type
    DW_AT_signature [DW_FORM_ref_sig8]      (0xc6694e51369161f2)

=>
Type Unit: type_signature = 0xc6694e51369161f2
DW_TAG_type_unit 
  DW_TAG_structure_type
    DW_AT_name

Best to wait for @dwblaikie who has authored 58b1b64 and
3cbc4c4

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

Love the change - thanks for this! (& putting it into LLVM with the intent to match future changes to LLDB is excellent - will help with the type printer generalization work I've got going that's trying to port the type printer to work with LLDB too, which I'd just commented out/punted on the type unit support for now - and with this patch it should come out in the wash (after resolving these changes against my own)

@@ -312,27 +307,19 @@ DWARFDie::getAttributeValueAsReferencedDie(dwarf::Attribute Attr) const {

DWARFDie
DWARFDie::getAttributeValueAsReferencedDie(const DWARFFormValue &V) const {
DWARFDie Result;
if (auto SpecRef = V.getAsRelativeReference()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems a bit questionable that getAsRelativeReference returns a valid value for DW_FORM_ref_sig8 - it's not a CU-relative reference form... (eg: if one had called getAttributeValueAsReferencedDie on a ref_sig8 before this patch, it would have walked off to some garbage offset, right? Interpreting the sig8 as an offset)

So might be worth changing it to not do that and having an explicit/separate case here?

(aside: this case is probably untested? How'd you come across changing this function? Clang/LLVM doesn't produce direct references via ref_sig8, only via the skeleton type DIEs... ah, I see, the findRecursively change would cause us to call getAttributeValueAsReferencedDie(DW_AT_signature)? OK.

Incidentally this has probably added support for some DWARF that GCC produces - where it references the type unit directly from DW_AT_type, maybe? Currently llvm-dwarfdump doesn't resolve such references ( https://godbolt.org/z/E5boxcj1r - note the DW_TAG_variable's DW_AT_type (0xc949e2ea8b91cfb0) only has the signature, and I guess with this patch maybe we now get DW_AT_type (0xc949e2ea8b91cfb0 "t1")?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So might be worth changing it to not do that and having an explicit/separate case here?

Yeah, I was wondering about that. It did seem somewhat suboptimal, but I wasn't particularly keen on taking it on.

Now I am, and here's my attempt to do something about this: #98905

Incidentally this has probably added support for some DWARF that GCC produces - where it references the type unit directly from DW_AT_type, maybe? Currently llvm-dwarfdump doesn't resolve such references ( https://godbolt.org/z/E5boxcj1r - note the DW_TAG_variable's DW_AT_type (0xc949e2ea8b91cfb0) only has the signature, and I guess with this patch maybe we now get DW_AT_type (0xc949e2ea8b91cfb0 "t1")?)

Interesting. I'll look into this tomorrow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Incidentally this has probably added support for some DWARF that GCC produces - where it references the type unit directly from DW_AT_type, maybe? Currently llvm-dwarfdump doesn't resolve such references ( https://godbolt.org/z/E5boxcj1r - note the DW_TAG_variable's DW_AT_type (0xc949e2ea8b91cfb0) only has the signature, and I guess with this patch maybe we now get DW_AT_type (0xc949e2ea8b91cfb0 "t1")?)

Interesting. I'll look into this tomorrow.

Yes, it really does fix it. I added a test (modifying an existing one) for that. PTAL.

labath added a commit to labath/llvm-project that referenced this pull request Jul 15, 2024
The result of the function cannot be correctly interpreted without
knowing the precise form type (a type signature needs to be looked up
very differently from a supplementary debug info reference). The
function sort of worked because the two reference types (unit-relative
and section-relative) that can be handled uniformly are also the most
common types of references, but this setup made it easy to write code
which does not support other kinds of reference (and if one tried to
support them, the result didn't look pretty --
https://github.com/llvm/llvm-project/pull/97423/files#r1676217081).

The split is based on the reference type classification from DWARFv5
(Section 7.5.5 Classes and Forms), and it should enable uniform (if
slightly more verbose) hadling. Note that this only affects users which
want more control of how (or if) the references are resolved. Users
which just want to access the referenced DIE can use the higher level
API (DWARFDie::GetAttributeValueAsReferencedDie) which returns (or will
return after llvm#97423 is merged) the correct die for all reference types
(except for supplementary references, which we don't support right now).
labath added a commit that referenced this pull request Jul 16, 2024
The result of the function cannot be correctly interpreted without
knowing the precise form type (a type signature needs to be looked up
very differently from a supplementary debug info reference). The
function sort of worked because the two reference types (unit-relative
and section-relative) that can be handled uniformly are also the most
common types of references, but this setup made it easy to write code
which does not support other kinds of reference (and if one tried to
support them, the result didn't look pretty --
https://github.com/llvm/llvm-project/pull/97423/files#r1676217081).

The split is based on the reference type classification from DWARFv5
(Section 7.5.5 Classes and Forms), and it should enable uniform (if
slightly more verbose) hadling. Note that this only affects users which
want more control of how (or if) the references are resolved. Users
which just want to access the referenced DIE can use the higher level
API (DWARFDie::GetAttributeValueAsReferencedDie) which returns (or will
return after #97423 is merged) the correct die for all reference types
(except for supplementary references, which we don't support right now).
sayhaan pushed a commit to sayhaan/llvm-project that referenced this pull request Jul 16, 2024
Summary:
The result of the function cannot be correctly interpreted without
knowing the precise form type (a type signature needs to be looked up
very differently from a supplementary debug info reference). The
function sort of worked because the two reference types (unit-relative
and section-relative) that can be handled uniformly are also the most
common types of references, but this setup made it easy to write code
which does not support other kinds of reference (and if one tried to
support them, the result didn't look pretty --
https://github.com/llvm/llvm-project/pull/97423/files#r1676217081).

The split is based on the reference type classification from DWARFv5
(Section 7.5.5 Classes and Forms), and it should enable uniform (if
slightly more verbose) hadling. Note that this only affects users which
want more control of how (or if) the references are resolved. Users
which just want to access the referenced DIE can use the higher level
API (DWARFDie::GetAttributeValueAsReferencedDie) which returns (or will
return after llvm#97423 is merged) the correct die for all reference types
(except for supplementary references, which we don't support right now).

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D59822392
@Endilll Endilll removed their request for review July 17, 2024 14:42
@labath
Copy link
Collaborator Author

labath commented Jul 17, 2024

Sorry about the messed up merge. TIL the order of parents is important.

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

Awesome!

@labath labath merged commit e93df78 into llvm:main Jul 18, 2024
7 checks passed
@labath labath deleted the signature-llvm branch July 18, 2024 07:44
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jul 18, 2024

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-gcc-ubuntu running on sie-linux-worker3 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/1857

Here is the relevant piece of the build log for the reference:

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'cross-project-tests :: debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
Verifying -:	file format elf64-x86-64
Verifying .debug_abbrev...
Verifying .debug_info Unit Header Chain...
Verifying .debug_types Unit Header Chain...
Verifying non-dwo Units...
Verifying unit: 1 / 1, "/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/cross-project-tests/debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp"
Verifying dwo Units...
Verifying .debug_line...
Verifying .debug_str_offsets...
No errors.
Verifying -:	file format elf64-x86-64
Verifying .debug_abbrev...
Verifying .debug_info Unit Header Chain...
Verifying .debug_types Unit Header Chain...
Verifying non-dwo Units...
Verifying unit: 1 / 1, "/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/cross-project-tests/debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp"
Verifying dwo Units...
Verifying .debug_line...
Verifying .debug_str_offsets...
No errors.
Verifying -:	file format elf64-x86-64
Verifying .debug_abbrev...
Verifying .debug_info Unit Header Chain...
Verifying .debug_types Unit Header Chain...
Verifying non-dwo Units...
Verifying unit: 1 / 83, "/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/cross-project-tests/debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp"
error: Simplified template DW_AT_name could not be reconstituted:
         original: t3<int, false>
    reconstituted: t3

0x000000d6: DW_TAG_structure_type [20]   (0x0000000b)
              DW_AT_declaration [DW_FORM_flag_present]	(true)
              DW_AT_signature [DW_FORM_ref_sig8]	(0x384ddc13a7a92863)


0x0000000b: DW_TAG_compile_unit [52] *
              DW_AT_producer [DW_FORM_strp]	( .debug_str[0x00000000] = "clang version 19.0.0git (https://github.com/llvm/llvm-project.git e93df78bd46b585c0bdabdbdc95410e4c08b9d38)")
              DW_AT_language [DW_FORM_data2]	(DW_LANG_C_plus_plus_14)
              DW_AT_name [DW_FORM_strp]	( .debug_str[0x0000006c] = "/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/cross-project-tests/debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp")
              DW_AT_stmt_list [DW_FORM_sec_offset]	(0x00000000)
              DW_AT_comp_dir [DW_FORM_strp]	( .debug_str[0x0000010a] = "/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/projects/cross-project-tests/debuginfo-tests/clang_llvm_roundtrip")
              DW_AT_low_pc [DW_FORM_addr]	(0x0000000000000000)
              DW_AT_ranges [DW_FORM_sec_offset]	(0x00000000
                 [0x0000000000000000, 0x000000000000038d) ".text"
                 [0x0000000000000390, 0x00000000000003a6) ".text"
...

labath added a commit that referenced this pull request Jul 18, 2024
labath added a commit that referenced this pull request Jul 18, 2024
Harini0924 pushed a commit to Harini0924/llvm-project that referenced this pull request Jul 22, 2024
findRecursively follows DW_AT_specification and DW_AT_abstract_origin
references, but not DW_AT_signature. As far as I can tell, there is no
fundamental difference between these attributes that would make this
behavior desirable, and this just seems like a consequence of the fact
that this attribute is newer. This patch aims to change that.

The motivation is some code in lldb, which assumes that it can construct
a qualified name of a type by just walking the parent chain and looking
at the name attribute. This works for "regular" debug info, even when
some of the DIEs are just forward declarations, but it breaks in the
presence of type units, because of the need to explicitly resolve the
signature reference.

While LLDB does not use the llvm's DWARFDie class (yet?), this seems
like a very important change in the overall API, and any divergence here
would complicate eventual reunification, which is why I am making the
change in the llvm API first. However, putting lldb aside, I think this
change is beneficial in llvm on its own, as it allows us to remove the
explicit DW_AT_signature resolution in the DWARFTypePrinter.
Harini0924 pushed a commit to Harini0924/llvm-project that referenced this pull request Jul 22, 2024
sgundapa pushed a commit to sgundapa/upstream_effort that referenced this pull request Jul 23, 2024
findRecursively follows DW_AT_specification and DW_AT_abstract_origin
references, but not DW_AT_signature. As far as I can tell, there is no
fundamental difference between these attributes that would make this
behavior desirable, and this just seems like a consequence of the fact
that this attribute is newer. This patch aims to change that.

The motivation is some code in lldb, which assumes that it can construct
a qualified name of a type by just walking the parent chain and looking
at the name attribute. This works for "regular" debug info, even when
some of the DIEs are just forward declarations, but it breaks in the
presence of type units, because of the need to explicitly resolve the
signature reference.

While LLDB does not use the llvm's DWARFDie class (yet?), this seems
like a very important change in the overall API, and any divergence here
would complicate eventual reunification, which is why I am making the
change in the llvm API first. However, putting lldb aside, I think this
change is beneficial in llvm on its own, as it allows us to remove the
explicit DW_AT_signature resolution in the DWARFTypePrinter.
sgundapa pushed a commit to sgundapa/upstream_effort that referenced this pull request Jul 23, 2024
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
The result of the function cannot be correctly interpreted without
knowing the precise form type (a type signature needs to be looked up
very differently from a supplementary debug info reference). The
function sort of worked because the two reference types (unit-relative
and section-relative) that can be handled uniformly are also the most
common types of references, but this setup made it easy to write code
which does not support other kinds of reference (and if one tried to
support them, the result didn't look pretty --
https://github.com/llvm/llvm-project/pull/97423/files#r1676217081).

The split is based on the reference type classification from DWARFv5
(Section 7.5.5 Classes and Forms), and it should enable uniform (if
slightly more verbose) hadling. Note that this only affects users which
want more control of how (or if) the references are resolved. Users
which just want to access the referenced DIE can use the higher level
API (DWARFDie::GetAttributeValueAsReferencedDie) which returns (or will
return after #97423 is merged) the correct die for all reference types
(except for supplementary references, which we don't support right now).

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251742
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
findRecursively follows DW_AT_specification and DW_AT_abstract_origin
references, but not DW_AT_signature. As far as I can tell, there is no
fundamental difference between these attributes that would make this
behavior desirable, and this just seems like a consequence of the fact
that this attribute is newer. This patch aims to change that.

The motivation is some code in lldb, which assumes that it can construct
a qualified name of a type by just walking the parent chain and looking
at the name attribute. This works for "regular" debug info, even when
some of the DIEs are just forward declarations, but it breaks in the
presence of type units, because of the need to explicitly resolve the
signature reference.

While LLDB does not use the llvm's DWARFDie class (yet?), this seems
like a very important change in the overall API, and any divergence here
would complicate eventual reunification, which is why I am making the
change in the llvm API first. However, putting lldb aside, I think this
change is beneficial in llvm on its own, as it allows us to remove the
explicit DW_AT_signature resolution in the DWARFTypePrinter.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251698
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…99444)

Summary:
Reverts #97423 due to a failure in the
cross-project-tests.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250990
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants