[DWARF] Find offset of attribute.

[ayermolo]

Summary

This is used by BOLT to do patching of DebugInfo section, and Line Table. Directly by using find, and through getAttrFieldOffsetForUnit.

[ayermolo]

updated API description

[dblaikie]

Broadly I'm a bit hesitant about the whole direction of adding various features to libDebugInfoDWARF for writing DWARF - since it hasn't been used for that so far, it's a fairly significant change in direction/API, even if the particular features needed so far are small.

How does llvm-dsymutil handle this? Does it use libDebugInfoDWARF for modifying DWARF, or does it do something else? (& broadly: Why are BOLT's needs categorically different from llvm-dsymutil's needs that motivate different API design here?)

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

155–184

Might it make sense to decompose this function into two (the original getAttributeValue could remain - but it'd call two other functions to do the work - one that computes the offset+AttributeSpec, and another that uses that to extract the value.

Then the BOLT use case could use the first function to get the offset if it wants it, without modifying the existing API to have an extra/optional out parameter that's a bit strange for its use case?

[ayermolo]

In D107874#2942339, @dblaikie wrote:

Broadly I'm a bit hesitant about the whole direction of adding various features to libDebugInfoDWARF for writing DWARF - since it hasn't been used for that so far, it's a fairly significant change in direction/API, even if the particular features needed so far are small.

How does llvm-dsymutil handle this? Does it use libDebugInfoDWARF for modifying DWARF, or does it do something else? (& broadly: Why are BOLT's needs categorically different from llvm-dsymutil's needs that motivate different API design here?)

I looked in to dsymutil in llvm tools. Looks like it relies on DWARFLinker. Looking through that closest I found is

1. patchRangesForUnit This uses DWARFLInker data structures and DWARFDataExtractor to iterate over all the ranges, patches them and writes them out.
2. cloneAllCompileUnits --> cloneDIE This goes through DIEs in systematic way keeping track of offset as it clones a die.

I believe BOLT access pattern is more generic where we access context specific values in DIE in various parts of code. Both using getAttrFieldOffsetForUnit, and DWARDie::find explicilty.
Some of the examples.
getAttrFieldOffsetForUnit
https://github.com/facebookincubator/BOLT/blob/37e51a3d450f371b811bf4ee5c0575d5e7c97e9c/bolt/src/DWARFRewriter.cpp#L704
DWARFDie::find
https://github.com/facebookincubator/BOLT/blob/37e51a3d450f371b811bf4ee5c0575d5e7c97e9c/bolt/src/DWARFRewriter.cpp#L170
https://github.com/facebookincubator/BOLT/blob/37e51a3d450f371b811bf4ee5c0575d5e7c97e9c/bolt/src/DWARFRewriter.cpp#L197

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

155–184

Maybe break it up in to two APIs. one not taking offset (under the hood invoking) one that invokes one which takes it as none optional parameter?
There are cases in BOLT where we want both the value and offset.
https://github.com/facebookincubator/BOLT/blob/37e51a3d450f371b811bf4ee5c0575d5e7c97e9c/bolt/src/DWARFRewriter.cpp#L446
Somewhat similar to extractValue in DWARFFormValue.

[dblaikie]

This'll need some test coverage - probably a unit test.

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

155–184

My hope was to avoid adding more optional out parameters, to keep APIs simpler.

If you need both values, you'd be able to use the two components directly, rather than using the wrapper function (this function that already exists and would become a wrapper for the two parts).

Imagine some code like this:

uint64_t getAttributeOffsetFromIndex(uint32_t AttrIndex, const DWARFUnit &U) {
  auto DebugInfoData = U.getDebugInfoExtractor();

  // Add the byte size of ULEB that for the abbrev Code so we can start
  // skipping the attribute data.
  uint64_t Offset = CodeByteSize;
  for (uint32_t CurAttrIdx = 0; CurAttrIdx != AttrIndex; ++CurAttrIdx)
    // Match Offset along until we get to the attribute we want.
    if (auto FixedSize = AttributeSpecs[CurAttrIdx].getByteSize(U))
      Offset += *FixedSize;
    else
      DWARFFormValue::skipValue(AttributeSpecs[CurAttrIdx].Form, DebugInfoData,
                                &Offset, U.getFormParams());

  return Offset;
}
Optional<DWARFFormValue> getAttributeValueFromOffset(uint32_t Index, uint64_t Offset, DWARFUnit &U) {
      // We have arrived at the attribute to extract, extract if from Offset.
    const AttributeSpec &Spec = AttributeSpecs[Index];
    if (Spec.isImplicitConst())
      return DWARFFormValue::createFromSValue(Spec.Form,
                                              Spec.getImplicitConstValue());
  
    DWARFFormValue FormValue(Spec.Form);
    if (FormValue.extractValue(DebugInfoData, &Offset, U.getFormParams(), &U))
      return FormValue;
  
    return None;
}
Optional<DWARFFormValue> DWARFAbbreviationDeclaration::getAttributeValue(
      const uint64_t DIEOffset, const dwarf::Attribute Attr,
      const DWARFUnit &U) const {
  Optional<uint32_t> Index = findAttributeIndex(Attr);
  if (!Index)
      return None;
  uint64_t Offset = getAttributeOffsetFromIndex(*Index, U);
  return getAttributeValueFromOffset(*Index, Offset, U); 
}

And in your use-case (and/or in the new getAttrFieldOffsetForUnit you're proposing in this patch (not sure if that's the only entrypoint to this data you want, or if you've got other uses in mind))you can use these 3 functions directly, keeping the Offset for other purposes as well.

[ayermolo]

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

155–184

Sorry for late reply. Circling back to this. Let me refactor our code with these APIs in mind.
There are two entry points. getAttrFieldOffsetForUnit, and DIE.find. For later there are two use cases. One is we just need an offset with a check if attribute exists. For second we need both offset AND value. Let me see what can be done on that end.
Also add tests. Was holding off on that until path forward is clear.

[ayermolo]

Refactored to create two new APIs.
Also removed getAttrFieldOffsetForUnit, and changes to DWARFDie.
Looking over our usage model in light of the two new APIs, that functionality can be done with a helper function locally.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

177–178

I guess this should probably be an assertion.

llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1788 ↗

(On Diff #369495)

Could probably also test the AttrIndex value before using it to lookup the attribute value.

[ayermolo]

Changed if condition to assert

[ayermolo]

llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1788 ↗

(On Diff #369495)

Not sure I follow. I am testing it on line 1786.

[dblaikie]

llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1788 ↗

(On Diff #369495)

The 1786 checks that AttrIndex has a value (any value, not None), but doesn't check what value that is.

Specifically, something like:

EXPECT_TRUE(*AttrIndex, 0);

I guess (since it seems this is only looking at the zeroth attribute, judging by line 1782?). In addition to the existing expects (I'd put this between 1786 and 1787 - after checking that AttrIndex is not None, but before using the value - so if there was a buggy value (if it returned 10 instead of 0, for instance) this check would fail before invoking UB/asserting inside getAttributeOffsetFromIndex, for instance))

[ayermolo]

Added check for AttrIndex value.

[ayermolo]

llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1788 ↗

(On Diff #369495)

Ah I see. thanks. Added. Will commit tomorrow if no other comments.

[dblaikie]

Do you need me to commit this for you, or can you commit it yourself?

llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1787 ↗

(On Diff #369816)

I'd probably write that as 0u rather than with the cast, but whatever suits.

[ayermolo]

In D107874#2975873, @dblaikie wrote:

Do you need me to commit this for you, or can you commit it yourself?

I got commit access few weeks ago.

llvm/include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h

public:
   ///
   /// \param DIEOffset the DIE offset that points to the ULEB128 abbreviation
   /// code in the .debug_info data.
   /// \param Attr DWARF attribute to search for.
   /// \param U the DWARFUnit the contains the DIE.
   /// \returns Optional DWARF form value if the attribute was extracted.
   Optional<DWARFFormValue> getAttributeValue(const uint64_t DIEOffset,
                                              const dwarf::Attribute Attr,
-                                             const DWARFUnit &U) const;
+                                             const DWARFUnit &U,
+                                             uint64_t *OffsetPtr = 0) const;
 
   bool extract(DataExtractor Data, uint64_t* OffsetPtr);
   void dump(raw_ostream &OS) const;
 
   // Return an optional byte size of all attribute data in this abbreviation
   // if a constant byte size can be calculated given a DWARFUnit. This allows
   // DWARF parsing to be faster as many DWARF DIEs have a fixed byte size.
   Optional<size_t> getFixedAttributesByteSize(const DWARFUnit &U) const;

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

public:
   };
 
   /// Get the compilation unit, the function DIE and lexical block DIE for the
   /// given address where applicable.
   /// TODO: change input parameter from "uint64_t Address"
   ///       into "SectionedAddress Address"
   DIEsForAddress getDIEsForAddress(uint64_t Address);
 
+  /// Get offset to an attribute value within a compile unit
+  /// or 0 if the attribute was not found.
+  Optional<uint64_t> getAttrFieldOffsetForUnit(DWARFUnit &U,
+                                               dwarf::Attribute Attr) const;
+
   DILineInfo getLineInfoForAddress(
       object::SectionedAddress Address,
       DILineInfoSpecifier Specifier = DILineInfoSpecifier()) override;
   DILineInfoTable getLineInfoForAddressRange(
       object::SectionedAddress Address, uint64_t Size,
       DILineInfoSpecifier Specifier = DILineInfoSpecifier()) override;
   DIInliningInfo getInliningInfoForAddress(
       object::SectionedAddress Address,

llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h

public:
   ///
   /// Extract an attribute value from this DIE only. This call doesn't look
   /// for the attribute value in any DW_AT_specification or
   /// DW_AT_abstract_origin referenced DIEs.
   ///
   /// \param Attr the attribute to extract.
   /// \returns an optional DWARFFormValue that will have the form value if the
   /// attribute was successfully extracted.
-  Optional<DWARFFormValue> find(dwarf::Attribute Attr) const;
+  Optional<DWARFFormValue> find(dwarf::Attribute Attr,
+                                uint64_t *OffsetPtr = 0) const;
 
   /// Extract the first value of any attribute in Attrs from this DIE.
   ///
   /// Extract the first attribute that matches from this DIE only. This call
   /// doesn't look for the attribute value in any DW_AT_specification or
   /// DW_AT_abstract_origin referenced DIEs. The attributes will be searched
   /// linearly in the order they are specified within Attrs.
   ///

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

DWARFAbbreviationDeclaration::findAttributeIndex(dwarf::Attribute Attr) const {
   for (uint32_t i = 0, e = AttributeSpecs.size(); i != e; ++i) {
     if (AttributeSpecs[i].Attr == Attr)
       return i;
   }
   return None;
 }
 
 Optional<DWARFFormValue> DWARFAbbreviationDeclaration::getAttributeValue(
-    const uint64_t DIEOffset, const dwarf::Attribute Attr,
-    const DWARFUnit &U) const {
+    const uint64_t DIEOffset, const dwarf::Attribute Attr, const DWARFUnit &U,
+    uint64_t *OffsetPtr) const {
   // Check if this abbreviation has this attribute without needing to skip
   // any data so we can return quickly if it doesn't.
   Optional<uint32_t> MatchAttrIndex = findAttributeIndex(Attr);
   if (!MatchAttrIndex)
     return None;
 
   auto DebugInfoData = U.getDebugInfoExtractor();
 
   // Add the byte size of ULEB that for the abbrev Code so we can start
   // skipping the attribute data.
   uint64_t Offset = DIEOffset + CodeByteSize;
   for (uint32_t CurAttrIdx = 0; CurAttrIdx != *MatchAttrIndex; ++CurAttrIdx)
     // Match Offset along until we get to the attribute we want.
     if (auto FixedSize = AttributeSpecs[CurAttrIdx].getByteSize(U))
       Offset += *FixedSize;
     else
       DWARFFormValue::skipValue(AttributeSpecs[CurAttrIdx].Form, DebugInfoData,
                                 &Offset, U.getFormParams());
 
   // We have arrived at the attribute to extract, extract if from Offset.
+  if (OffsetPtr)
+    *OffsetPtr = Offset;
   const AttributeSpec &Spec = AttributeSpecs[*MatchAttrIndex];
   if (Spec.isImplicitConst())
     return DWARFFormValue::createFromSValue(Spec.Form,
                                             Spec.getImplicitConstValue());
 
   DWARFFormValue FormValue(Spec.Form);
   if (FormValue.extractValue(DebugInfoData, &Offset, U.getFormParams(), &U))
     return FormValue;
 
   return None;
 }
 
 size_t DWARFAbbreviationDeclaration::FixedSizeInfo::getByteSize(
     const DWARFUnit &U) const {
   size_t ByteSize = NumBytes;
   if (NumAddrs)
     ByteSize += NumAddrs * U.getAddressByteSize();
   if (NumRefAddrs)

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

if (Spec.FLIKind != FileLineInfoKind::None) {
                                    CallDiscriminator);
       }
     }
     InliningInfo.addFrame(Frame);
   }
   return InliningInfo;
 }
 
+Optional<uint64_t>
+DWARFContext::getAttrFieldOffsetForUnit(DWARFUnit &U,
+                                        dwarf::Attribute Attr) const {
+  const DWARFDie UnitDIE = U.getUnitDIE();
+  if (!UnitDIE)
+    return None;
+
+  uint64_t Offset = 0;
+  if (!UnitDIE.find(Attr, &Offset))
+    return None;
+
+  return Offset;
+}
+
 std::shared_ptr<DWARFContext>
 DWARFContext::getDWOContext(StringRef AbsolutePath) {
   if (auto S = DWP.lock()) {
     DWARFContext *Ctxt = S->Context.get();
     return std::shared_ptr<DWARFContext>(std::move(S), Ctxt);
   }
 
   std::weak_ptr<DWOFile> *Entry = &DWOFiles[AbsolutePath];

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp

 
 bool DWARFDie::isSubprogramDIE() const { return getTag() == DW_TAG_subprogram; }
 
 bool DWARFDie::isSubroutineDIE() const {
   auto Tag = getTag();
   return Tag == DW_TAG_subprogram || Tag == DW_TAG_inlined_subroutine;
 }
 
-Optional<DWARFFormValue> DWARFDie::find(dwarf::Attribute Attr) const {
+Optional<DWARFFormValue> DWARFDie::find(dwarf::Attribute Attr,
+                                        uint64_t *OffsetPtr) const {
   if (!isValid())
     return None;
   auto AbbrevDecl = getAbbreviationDeclarationPtr();
   if (AbbrevDecl)
-    return AbbrevDecl->getAttributeValue(getOffset(), Attr, *U);
+    return AbbrevDecl->getAttributeValue(getOffset(), Attr, *U, OffsetPtr);
   return None;
 }
 
 Optional<DWARFFormValue>
 DWARFDie::find(ArrayRef<dwarf::Attribute> Attrs) const {
   if (!isValid())
     return None;
   auto AbbrevDecl = getAbbreviationDeclarationPtr();