This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Find offset of attribute.
ClosedPublic

Authored by ayermolo on Aug 10 2021, 5:34 PM.

Details

Summary

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

Diff Detail

Event Timeline

ayermolo created this revision.Aug 10 2021, 5:34 PM
ayermolo requested review of this revision.Aug 10 2021, 5:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2021, 5:34 PM
ayermolo updated this revision to Diff 365633.Aug 10 2021, 5:40 PM

updated API description

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?

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.

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 added inline comments.Aug 25 2021, 6:12 PM
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 updated this revision to Diff 369495.Aug 30 2021, 11:04 AM

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 accepted this revision.Aug 30 2021, 9:51 PM
dblaikie added inline comments.
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.

This revision is now accepted and ready to land.Aug 30 2021, 9:51 PM
ayermolo updated this revision to Diff 369812.Aug 31 2021, 3:54 PM

Changed if condition to assert

ayermolo marked 3 inline comments as done.Aug 31 2021, 3:59 PM
ayermolo added inline comments.
llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1788 ↗(On Diff #369495)

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

dblaikie added inline comments.Aug 31 2021, 4:13 PM
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 updated this revision to Diff 369816.Aug 31 2021, 4:21 PM

Added check for AttrIndex value.

ayermolo marked 2 inline comments as done.Aug 31 2021, 4:22 PM
ayermolo added inline comments.
llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1788 ↗(On Diff #369495)

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

dblaikie accepted this revision.Aug 31 2021, 4:54 PM

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.

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

I got commit access few weeks ago.

This revision was automatically updated to reflect the committed changes.