[lldb][NFCI] Replace dw_form_t with llvm::dwarf::Form

[bulbazord]

Summary

LLDB currently defines dw_form_t as a uint16_t which makes sense.
However, LLVM also defines a similar type llvm::dwarf::Form which is
an enum backed by a uint16_t. Switching to the llvm implementation
means that we can more easily interoperate with the LLVM DWARF code.

Additionally, we get some type checking out of this: I found that
DWARFAttribute had a method called FormAtIndex that returned a
dw_attr_t. Although dw_attr_t is also a uint16_t under the hood,
the type checking benefits here are undeniable: If this had returned a
something of different signedness/width, we could have had some bad
bugs.

[rastogishubham]

Great refactoring!

[jingham]

Apparently a similar change was made with dw_tag_t, in the line below your first deletion I see:

typedef llvm::dwarf::Tag dw_tag_t;

It seems weird to have dw_tag_t but lvm::dwarf::Form. If there's a good reason to use the more verbose form, we should probably do the same with the Tag for consistency. Otherwise, you can just play the same re-typedef-ing as was done for tag, right?

[bulbazord]

In D150228#4330798, @jingham wrote:

Apparently a similar change was made with dw_tag_t, in the line below your first deletion I see:

typedef llvm::dwarf::Tag dw_tag_t;

It seems weird to have dw_tag_t but lvm::dwarf::Form. If there's a good reason to use the more verbose form, we should probably do the same with the Tag for consistency. Otherwise, you can just play the same re-typedef-ing as was done for tag, right?

Yes, Jonas did that work in 7fa72881d4cbf. I intentionally chose to not typedef here as a matter of personal preference, I prefer the explicitness of the full type. I do agree that we should be consistent here though. I don't think it quite matters which solution we go with, but do you (or anybody else) have strong opinions about this?

[bulbazord]

Do a typedef in dwarf.h (like llvm::dwarf::Tag) instead of explicitly writing the type out everywhere

[bulbazord]

Remove unused include in header

[fdeazeve]

LGTM! I like how the static_cast now is explicit about the fact that some truncation is going on from the ULEB reading.

Do you have plans to do something similar for the attribute typedef?

[bulbazord]

In D150228#4331943, @fdeazeve wrote:

Do you have plans to do something similar for the attribute typedef?

Yes I would like to do that too! :)

[bulbazord]

Fix a warning in HashedNameToDIE

lldb/include/lldb/Core/dwarf.h

 
 namespace lldb_private {
 namespace dwarf {
   using namespace llvm::dwarf;
 }
 }
 
 typedef uint16_t dw_attr_t;
-typedef uint16_t dw_form_t;
+typedef llvm::dwarf::Form dw_form_t;
 typedef llvm::dwarf::Tag dw_tag_t;
 typedef uint64_t dw_addr_t; // Dwarf address define that must be big enough for
                             // any addresses in the compile units that get
                             // parsed
 
 typedef uint64_t dw_offset_t; // Dwarf Debug Information Entry offset for any
                               // offset into the file
 

lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h

public:
   DWARFAbbreviationDeclaration(dw_tag_t tag, uint8_t has_children);
 
   uint32_t Code() const { return m_code; }
   void SetCode(uint32_t code) { m_code = code; }
   dw_tag_t Tag() const { return m_tag; }
   bool HasChildren() const { return m_has_children; }
   size_t NumAttributes() const { return m_attributes.size(); }
   dw_form_t GetFormByIndex(uint32_t idx) const {
-    return m_attributes.size() > idx ? m_attributes[idx].get_form() : 0;
+    return m_attributes.size() > idx ? m_attributes[idx].get_form()
+                                     : dw_form_t(0);
   }
 
   // idx is assumed to be valid when calling GetAttrAndFormByIndex()
   void GetAttrAndFormValueByIndex(uint32_t idx, dw_attr_t &attr,
                                   DWARFFormValue &form_value) const {
     m_attributes[idx].get(attr, form_value.FormRef(), form_value.ValueRef());
   }
   dw_form_t GetFormByIndexUnchecked(uint32_t idx) const {

lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp

DWARFAbbreviationDeclaration::extract(const DWARFDataExtractor &data,
   if (m_tag == DW_TAG_null)
     return llvm::make_error<llvm::object::GenericBinaryError>(
         "abbrev decl requires non-null tag.");
 
   m_has_children = data.GetU8(offset_ptr);
 
   while (data.ValidOffset(*offset_ptr)) {
     dw_attr_t attr = data.GetULEB128(offset_ptr);
-    dw_form_t form = data.GetULEB128(offset_ptr);
+    auto form = static_cast<dw_form_t>(data.GetULEB128(offset_ptr));
 
     // This is the last attribute for this abbrev decl, but there may still be
     // more abbrev decls, so return MoreItems to indicate to the caller that
     // they should call this function again.
     if (!attr && !form)
       return DWARFEnumState::MoreItems;
 
     if (!attr || !form)

lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h

void Append(const DWARFFormValue &form_value, dw_offset_t attr_die_offset,
               dw_attr_t attr);
   DWARFUnit *CompileUnitAtIndex(uint32_t i) const { return m_infos[i].cu; }
   dw_offset_t DIEOffsetAtIndex(uint32_t i) const {
     return m_infos[i].die_offset;
   }
   dw_attr_t AttributeAtIndex(uint32_t i) const {
     return m_infos[i].attr.get_attr();
   }
-  dw_attr_t FormAtIndex(uint32_t i) const { return m_infos[i].attr.get_form(); }
+  dw_form_t FormAtIndex(uint32_t i) const { return m_infos[i].attr.get_form(); }
   DWARFFormValue::ValueType ValueAtIndex(uint32_t i) const {
     return m_infos[i].attr.get_value();
   }
   bool ExtractFormValueAtIndex(uint32_t i, DWARFFormValue &form_value) const;
   DWARFDIE FormValueAsReferenceAtIndex(uint32_t i) const;
   DWARFDIE FormValueAsReference(dw_attr_t attr) const;
   uint32_t FindAttributeIndex(dw_attr_t attr) const;
   void Clear() { m_infos.clear(); }

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp

else {
         case DW_FORM_GNU_addr_index:
         case DW_FORM_GNU_str_index:
         case DW_FORM_strx:
           data.Skip_LEB128(&offset);
           break;
 
         case DW_FORM_indirect:
           form_is_indirect = true;
-          form = data.GetULEB128(&offset);
+          form = static_cast<dw_form_t>(data.GetULEB128(&offset));
           break;
 
         case DW_FORM_strp:
         case DW_FORM_line_strp:
         case DW_FORM_sec_offset:
           data.GetU32(&offset);
           break;
 

lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h

public:
 
   DWARFFormValue() = default;
   DWARFFormValue(const DWARFUnit *unit) : m_unit(unit) {}
   DWARFFormValue(const DWARFUnit *unit, dw_form_t form)
       : m_unit(unit), m_form(form) {}
   const DWARFUnit *GetUnit() const { return m_unit; }
   void SetUnit(const DWARFUnit *unit) { m_unit = unit; }
   dw_form_t Form() const { return m_form; }
-  dw_form_t& FormRef() { return m_form; }
+  dw_form_t &FormRef() { return m_form; }
   void SetForm(dw_form_t form) { m_form = form; }
   const ValueType &Value() const { return m_value; }
   ValueType &ValueRef() { return m_value; }
   void SetValue(const ValueType &val) { m_value = val; }
 
   void Dump(lldb_private::Stream &s) const;
   bool ExtractValue(const lldb_private::DWARFDataExtractor &data,
                     lldb::offset_t *offset_ptr);
public:
   static int Compare(const DWARFFormValue &a, const DWARFFormValue &b);
   void Clear();
   static bool FormIsSupported(dw_form_t form);
 
 protected:
   // Compile unit where m_value was located.
   // It may be different from compile unit where m_value refers to.
   const DWARFUnit *m_unit = nullptr; // Unit for this form
-  dw_form_t m_form = 0;              // Form for this value
+  dw_form_t m_form = dw_form_t(0);   // Form for this value
   ValueType m_value;            // Contains all data for the form
 };
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFFORMVALUE_H

lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp

 
 class DWARFUnit;
 
 using namespace lldb_private;
 using namespace lldb_private::dwarf;
 
 void DWARFFormValue::Clear() {
   m_unit = nullptr;
-  m_form = 0;
+  m_form = dw_form_t(0);
   m_value = ValueTypeTag();
 }
 
 bool DWARFFormValue::ExtractValue(const DWARFDataExtractor &data,
                                   lldb::offset_t *offset_ptr) {
   if (m_form == DW_FORM_implicit_const)
     return true;
 
case DW_FORM_ref_addr:
       assert(m_unit);
       if (m_unit->GetVersion() <= 2)
         ref_addr_size = m_unit->GetAddressByteSize();
       else
         ref_addr_size = 4;
       m_value.value.uval = data.GetMaxU64(offset_ptr, ref_addr_size);
       break;
     case DW_FORM_indirect:
-      m_form = data.GetULEB128(offset_ptr);
+      m_form = static_cast<dw_form_t>(data.GetULEB128(offset_ptr));
       indirect = true;
       break;
     case DW_FORM_flag_present:
       m_value.value.uval = 1;
       break;
     default:
       return false;
     }
case DW_FORM_implicit_const:
     case DW_FORM_ref_udata:
     case DW_FORM_GNU_addr_index:
     case DW_FORM_GNU_str_index:
     case DW_FORM_strx:
       debug_info_data.Skip_LEB128(offset_ptr);
       return true;
 
   case DW_FORM_indirect: {
-    dw_form_t indirect_form = debug_info_data.GetULEB128(offset_ptr);
-    return DWARFFormValue::SkipValue(indirect_form, debug_info_data, offset_ptr,
-                                     unit);
+      auto indirect_form =
+          static_cast<dw_form_t>(debug_info_data.GetULEB128(offset_ptr));
+      return DWARFFormValue::SkipValue(indirect_form, debug_info_data,
+                                       offset_ptr, unit);
   }
 
   default:
     break;
   }
   return false;
 }
 

 bool DWARFFormValue::IsBlockForm(const dw_form_t form) {
   switch (form) {
   case DW_FORM_exprloc:
   case DW_FORM_block:
   case DW_FORM_block1:
   case DW_FORM_block2:
   case DW_FORM_block4:
     return true;
-  }
+  default:
     return false;
   }
+  llvm_unreachable("All cases handled above!");
+}
 
 bool DWARFFormValue::IsDataForm(const dw_form_t form) {
   switch (form) {
   case DW_FORM_sdata:
   case DW_FORM_udata:
   case DW_FORM_data1:
   case DW_FORM_data2:
   case DW_FORM_data4:
   case DW_FORM_data8:
     return true;
-  }
+  default:
     return false;
   }
+  llvm_unreachable("All cases handled above!");
+}
 
 bool DWARFFormValue::FormIsSupported(dw_form_t form) {
   switch (form) {
     case DW_FORM_addr:
     case DW_FORM_addrx:
     case DW_FORM_loclistx:
     case DW_FORM_rnglistx:
     case DW_FORM_block2:

lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp

while (data.GetU32(&offset)) {
       /* do nothing */;
     }
 
     // Hardcode to the only known value for now.
     AppendAtom(eAtomTypeDIEOffset, DW_FORM_data4);
   } else {
     for (uint32_t i = 0; i < atom_count; ++i) {
       AtomType type = (AtomType)data.GetU16(&offset);
-      dw_form_t form = (dw_form_t)data.GetU16(&offset);
+      auto form = static_cast<dw_form_t>(data.GetU16(&offset));
       AppendAtom(type, form);
     }
   }
   return offset;
 }
 
 size_t DWARFMappedHash::Prologue::GetByteSize() const {
   // Add an extra count to the atoms size for the zero termination Atom that