[PATCH] D107874: [DWARF] Find offset of attribute.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:155-184
   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
----------------
ayermolo wrote:
> dblaikie wrote:
> > 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? 
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107874/new/

https://reviews.llvm.org/D107874