[PATCH] D145499: [DebugInfo][DWARF] Add DwarfVersion parameter to DWARFFormValue::isFormClass.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D145499#4175662 <https://reviews.llvm.org/D145499#4175662>, @avl wrote:

> In D145499#4175487 <https://reviews.llvm.org/D145499#4175487>, @dblaikie wrote:
>
>> Sounds OK, though might be nice to avoid having these different ways of using (with/without DWARFUnit) and find some way to unify them (have some more general abstraction over DWARFUnit that can be provided by all users of this API or something)
>
> something like this?
>
>   diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
>   index fa47621265f7..b13f5ffdf7f4 100644
>   --- a/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
>   +++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
>   @@ -77,10 +77,7 @@ public:
>      dwarf::Form getForm() const { return Form; }
>      uint64_t getRawUValue() const { return Value.uval; }
>    
>   -  /// Returns true if current value is of \p FC form class.
>   -  /// If current value does not have reference to original DWARFUnit
>   -  /// then \p DwarfVersion might be used to check form class of value.
>   -  bool isFormClass(FormClass FC, uint16_t DwarfVersion = 3) const;
>   +  bool isFormClass(FormClass FC) const;
>      const DWARFUnit *getUnit() const { return U; }
>      void dump(raw_ostream &OS, DIDumpOptions DumpOpts = DIDumpOptions()) const;
>      void dumpSectionedAddress(raw_ostream &OS, DIDumpOptions DumpOpts,
>   @@ -352,6 +349,9 @@ toBlock(const std::optional<DWARFFormValue> &V) {
>      return std::nullopt;
>    }
>    
>   +bool doesFormBelongToClass(dwarf::Form Form, DWARFFormValue::FormClass FC,
>   +                                 uint16_t DwarfVersion);
>   +
>    } // end namespace dwarf
>    
>    } // end namespace llvm
>   diff --git a/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp b/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
>   index fd94c8a7ffbb..5ea0aa63c218 100644
>   --- a/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
>   +++ b/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
>   @@ -213,41 +213,8 @@ bool DWARFFormValue::skipValue(dwarf::Form Form, DataExtractor DebugInfoData,
>      return true;
>    }
>    
>   -bool DWARFFormValue::isFormClass(DWARFFormValue::FormClass FC,
>   -                                 uint16_t DwarfVersion) const {
>   -  // First, check DWARF5 form classes.
>   -  if (Form < ArrayRef(DWARF5FormClasses).size() &&
>   -      DWARF5FormClasses[Form] == FC)
>   -    return true;
>   -  // Check more forms from extensions and proposals.
>   -  switch (Form) {
>   -  case DW_FORM_GNU_ref_alt:
>   -    return (FC == FC_Reference);
>   -  case DW_FORM_GNU_addr_index:
>   -    return (FC == FC_Address);
>   -  case DW_FORM_GNU_str_index:
>   -  case DW_FORM_GNU_strp_alt:
>   -    return (FC == FC_String);
>   -  case DW_FORM_LLVM_addrx_offset:
>   -    return (FC == FC_Address);
>   -  default:
>   -    break;
>   -  }
>   -
>   -  if (FC == FC_SectionOffset) {
>   -    if (Form == DW_FORM_strp || Form == DW_FORM_line_strp)
>   -      return true;
>   -    // In DWARF3 DW_FORM_data4 and DW_FORM_data8 served also as a section
>   -    // offset. If we don't have a DWARFUnit, default to the old behavior.
>   -    if (Form == DW_FORM_data4 || Form == DW_FORM_data8) {
>   -      if (U)
>   -        return U->getVersion() <= 3;
>   -
>   -      return DwarfVersion <= 3;
>   -    }
>   -  }
>   -
>   -  return false;
>   +bool DWARFFormValue::isFormClass(DWARFFormValue::FormClass FC) const {
>   +  return doesFormBelongToClass(Form, FC, U ? U->getVersion() : 3);
>    }
>    
>    bool DWARFFormValue::extractValue(const DWARFDataExtractor &Data,
>   @@ -783,3 +750,36 @@ DWARFFormValue::getAsFile(DILineInfoSpecifier::FileLineInfoKind Kind) const {
>      }
>      return std::nullopt;
>    }
>   +
>   +bool doesFormBelongToClass(dwarf::Form Form, DWARFFormValue::FormClass FC,
>   +                                 uint16_t DwarfVersion) {
>   +    // First, check DWARF5 form classes.
>   +    if (Form < ArrayRef(DWARF5FormClasses).size() &&
>   +        DWARF5FormClasses[Form] == FC)
>   +      return true;
>   +    // Check more forms from extensions and proposals.
>   +    switch (Form) {
>   +    case DW_FORM_GNU_ref_alt:
>   +      return (FC == DWARFFormValue::FC_Reference);
>   +    case DW_FORM_GNU_addr_index:
>   +      return (FC == DWARFFormValue::FC_Address);
>   +    case DW_FORM_GNU_str_index:
>   +    case DW_FORM_GNU_strp_alt:
>   +      return (FC == DWARFFormValue::FC_String);
>   +    case DW_FORM_LLVM_addrx_offset:
>   +      return (FC == DWARFFormValue::FC_Address);
>   +    default:
>   +      break;
>   +    }
>   +
>   +    if (FC == DWARFFormValue::FC_SectionOffset) {
>   +      if (Form == DW_FORM_strp || Form == DW_FORM_line_strp)
>   +        return true;
>   +      // In DWARF3 DW_FORM_data4 and DW_FORM_data8 served also as a section
>   +      // offset.
>   +      if (Form == DW_FORM_data4 || Form == DW_FORM_data8)
>   +        return DwarfVersion <= 3;
>   +    }
>   +
>   +    return false;
>   +}

Sure, yeah, that seems OK!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145499