[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

149–183

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

149–183

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

149–183

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

149–183

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

170–171

I guess this should probably be an assertion.

llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1788

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

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

[dblaikie]

llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

1788

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

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

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:
   /// 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;
 
+  /// Compute an offset from a DIE specified by DIE offset and attribute index.
+  ///
+  /// \param AttrIndex an index of DWARF attribute.
+  /// \param DIEOffset the DIE offset that points to the ULEB128 abbreviation
+  /// code in the .debug_info data.
+  /// \param U the DWARFUnit the contains the DIE.
+  /// \returns an offset of the attribute.
+  uint64_t getAttributeOffsetFromIndex(uint32_t AttrIndex, uint64_t DIEOffset,
+                                       const DWARFUnit &U) const;
+
+  /// Extract a DWARF form value from a DIE speccified by attribute index and
+  /// its offset.
+  ///
+  /// \param AttrIndex an index of DWARF attribute.
+  /// \param Offset offset of the attribute.
+  /// \param U the DWARFUnit the contains the DIE.
+  /// \returns Optional DWARF form value if the attribute was extracted.
+  Optional<DWARFFormValue>
+  getAttributeValueFromOffset(uint32_t AttrIndex, uint64_t Offset,
+                              const DWARFUnit &U) 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/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

 Optional<uint32_t>
 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 {
-  // 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();
+uint64_t DWARFAbbreviationDeclaration::getAttributeOffsetFromIndex(
+    uint32_t AttrIndex, uint64_t DIEOffset, const DWARFUnit &U) const {
+  DWARFDataExtractor 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)
+  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>
+DWARFAbbreviationDeclaration::getAttributeValueFromOffset(
+    uint32_t AttrIndex, uint64_t Offset, const DWARFUnit &U) const {
+  if (AttributeSpecs.size() <= AttrIndex)
+    return None;
 
   // We have arrived at the attribute to extract, extract if from Offset.
-  const AttributeSpec &Spec = AttributeSpecs[*MatchAttrIndex];
+  const AttributeSpec &Spec = AttributeSpecs[AttrIndex];
   if (Spec.isImplicitConst())
     return DWARFFormValue::createFromSValue(Spec.Form,
                                             Spec.getImplicitConstValue());
 
   DWARFFormValue FormValue(Spec.Form);
+  DWARFDataExtractor DebugInfoData = U.getDebugInfoExtractor();
   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 {
+  // 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;
+
+  uint64_t Offset = getAttributeOffsetFromIndex(*MatchAttrIndex, DIEOffset, U);
+
+  return getAttributeValueFromOffset(*MatchAttrIndex, Offset, U);
 }
 
 size_t DWARFAbbreviationDeclaration::FixedSizeInfo::getByteSize(
     const DWARFUnit &U) const {
   size_t ByteSize = NumBytes;
   if (NumAddrs)
     ByteSize += NumAddrs * U.getAddressByteSize();
   if (NumRefAddrs)

llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp

TEST(DWARFDebugInfo, TestImplicitConstAbbrevs) {
   AbbrevIt Val2Abbrev = Abbrevs->end();
   for(auto it = Abbrevs->begin(); it != Abbrevs->end(); ++it) {
     if (it->getNumAttributes() == 0)
       continue; // root abbrev for DW_TAG_compile_unit
 
     auto A = it->getAttrByIndex(0);
     EXPECT_EQ(A, Attr);
 
+    Optional<uint32_t> AttrIndex = it->findAttributeIndex(A);
+    EXPECT_TRUE((bool)AttrIndex);
+    uint64_t OffsetVal =
+        it->getAttributeOffsetFromIndex(*AttrIndex, /* offset */ 0, *U);
+    EXPECT_TRUE(
+        it->getAttributeValueFromOffset(*AttrIndex, OffsetVal, *U).hasValue());
+
     auto FormValue = it->getAttributeValue(/* offset */ 0, A, *U);
     EXPECT_TRUE((bool)FormValue);
     EXPECT_EQ(FormValue->getForm(), dwarf::DW_FORM_implicit_const);
 
     const auto V = FormValue->getAsSignedConstant();
     EXPECT_TRUE((bool)V);
 
     auto VerifyAbbrevDump = [&V](AbbrevIt it) {