[dwarf2yaml] Change the return type of dumping functions to Error.

[Higuoxing]

Summary

This patch changes the return type of dumping functions to Error. This
is useful when the DWARF parser encounters an error, clients can consume
this error and dump the section as a raw content section.

[grimar]

LGTM

[jhenderson]

I like that it makes this makes all the dumpDebug* functions consistent. However, are there going to actually be changes to all of these functions to make the new error handling path useful?

[Higuoxing]

In D85505#2206076, @jhenderson wrote:

I like that it makes this makes all the dumpDebug* functions consistent. However, are there going to actually be changes to all of these functions to make the new error handling path useful?

This change is intended to make elf2yaml and macho2yaml share the same logic when dumping DWARF sections. If the parser fails, we are able to dump them as raw content sections.

I think not all of the error handling path are useful. For example, the .debug_str section's parser never fails.

[jhenderson]

In D85505#2206165, @Higuoxing wrote:

In D85505#2206076, @jhenderson wrote:

I like that it makes this makes all the dumpDebug* functions consistent. However, are there going to actually be changes to all of these functions to make the new error handling path useful?

This change is intended to make elf2yaml and macho2yaml share the same logic when dumping DWARF sections. If the parser fails, we are able to dump them as raw content sections.

I think not all of the error handling path are useful. For example, the .debug_str section's parser never fails.

Does this mean we could return void for some functions instead of changing their signatures like you are doing? It seems like that would be the right way round for functions that can never fail.

[Higuoxing]

In D85505#2206194, @jhenderson wrote:

In D85505#2206165, @Higuoxing wrote:

In D85505#2206076, @jhenderson wrote:

I like that it makes this makes all the dumpDebug* functions consistent. However, are there going to actually be changes to all of these functions to make the new error handling path useful?

This change is intended to make elf2yaml and macho2yaml share the same logic when dumping DWARF sections. If the parser fails, we are able to dump them as raw content sections.

I think not all of the error handling path are useful. For example, the .debug_str section's parser never fails.

Does this mean we could return void for some functions instead of changing their signatures like you are doing? It seems like that would be the right way round for functions that can never fail.

I think it's acceptable to make the return type to be void. But the dumpDebugSection() function in D85506 should be changed to

static Error dumpDebugSection(StringRef SecName, DWARFContext &DCtx,
                              DWARFYAML::Data &DWARF) {
  if (SecName == "__debug_abbrev") {
    dumpDebugAbbrev(DCtx, DWARF);
    return Error::sucess();
  }
  if (SecName == "__debug_aranges")
    return dumpDebugARanges(DCtx, DWARF);
  if (SecName == "__debug_info") {
    dumpDebugInfo(DCtx, DWARF);
    return Error::sucess();
  }
  if (SecName == "__debug_line") {
    dumpDebugLines(DCtx, DWARF);
    return Error::success();
  }
  if (SecName == "__debug_ranges")
    return dumpDebugRanges(DCtx, DWARF);
  if (SecName == "__debug_str") {
    dumpDebugStrings(DCtx, DWARF);
    return Error::success();
  }
  return createStringError(errc::not_supported,
                           "dumping " + SecName + " section is not supported");
}

[jhenderson]

In D85505#2206200, @Higuoxing wrote:

In D85505#2206194, @jhenderson wrote:

In D85505#2206165, @Higuoxing wrote:

In D85505#2206076, @jhenderson wrote:

I like that it makes this makes all the dumpDebug* functions consistent. However, are there going to actually be changes to all of these functions to make the new error handling path useful?

This change is intended to make elf2yaml and macho2yaml share the same logic when dumping DWARF sections. If the parser fails, we are able to dump them as raw content sections.

I think not all of the error handling path are useful. For example, the .debug_str section's parser never fails.

Does this mean we could return void for some functions instead of changing their signatures like you are doing? It seems like that would be the right way round for functions that can never fail.

I think it's acceptable to make the return type to be void. But the dumpDebugSection() function in D85506 should be changed to

static Error dumpDebugSection(StringRef SecName, DWARFContext &DCtx,
                              DWARFYAML::Data &DWARF) {
  if (SecName == "__debug_abbrev") {
    dumpDebugAbbrev(DCtx, DWARF);
    return Error::sucess();
  }
  if (SecName == "__debug_aranges")
    return dumpDebugARanges(DCtx, DWARF);
  if (SecName == "__debug_info") {
    dumpDebugInfo(DCtx, DWARF);
    return Error::sucess();
  }
  if (SecName == "__debug_line") {
    dumpDebugLines(DCtx, DWARF);
    return Error::success();
  }
  if (SecName == "__debug_ranges")
    return dumpDebugRanges(DCtx, DWARF);
  if (SecName == "__debug_str") {
    dumpDebugStrings(DCtx, DWARF);
    return Error::success();
  }
  return createStringError(errc::not_supported,
                           "dumping " + SecName + " section is not supported");
}

Right, that would be my personal preference. Limiting the scope of where errors need handling by not emitting them earlier than necessary seems beneficial to me. @grimar, any thoughts?

[Higuoxing]

In D85505#2206209, @jhenderson wrote:

Right, that would be my personal preference. Limiting the scope of where errors need handling by not emitting them earlier than necessary seems beneficial to me. @grimar, any thoughts?

I think your opinion makes more sense. Actually, some of the DWARF dumpers are fallible. We can keep the return type to be void by now and refactor them step by step. Then, this change can be abandoned?

[Higuoxing]

This comment was removed by Higuoxing.

[jhenderson]

In D85505#2206219, @Higuoxing wrote:

In D85505#2206209, @jhenderson wrote:

Right, that would be my personal preference. Limiting the scope of where errors need handling by not emitting them earlier than necessary seems beneficial to me. @grimar, any thoughts?

I think your opinion makes more sense. Actually, some of the DWARF dumpers are fallible. We can keep the return type to be void by now and refactor them step by step. Then, this change can be abandoned?

Yes, I think so.

[grimar]

In D85505#2206258, @jhenderson wrote:

In D85505#2206219, @Higuoxing wrote:

In D85505#2206209, @jhenderson wrote:

Right, that would be my personal preference. Limiting the scope of where errors need handling by not emitting them earlier than necessary seems beneficial to me. @grimar, any thoughts?

I think your opinion makes more sense. Actually, some of the DWARF dumpers are fallible. We can keep the return type to be void by now and refactor them step by step. Then, this change can be abandoned?

Yes, I think so.

Sorry, I've missed the question for me yesterday. This resolution looks fine to me either.

llvm/tools/obj2yaml/dwarf2yaml.cpp

 
 void dumpInitialLength(DataExtractor &Data, uint64_t &Offset,
                        DWARFYAML::InitialLength &InitialLength) {
   InitialLength.TotalLength = Data.getU32(&Offset);
   if (InitialLength.isDWARF64())
     InitialLength.TotalLength64 = Data.getU64(&Offset);
 }
 
-void dumpDebugAbbrev(DWARFContext &DCtx, DWARFYAML::Data &Y) {
+Error dumpDebugAbbrev(DWARFContext &DCtx, DWARFYAML::Data &Y) {
   auto AbbrevSetPtr = DCtx.getDebugAbbrev();
   if (AbbrevSetPtr) {
     for (auto AbbrvDeclSet : *AbbrevSetPtr) {
       for (auto AbbrvDecl : AbbrvDeclSet.second) {
         DWARFYAML::Abbrev Abbrv;
         Abbrv.Code = AbbrvDecl.getCode();
         Abbrv.Tag = AbbrvDecl.getTag();
         Abbrv.Children = AbbrvDecl.hasChildren() ? dwarf::DW_CHILDREN_yes
                                                  : dwarf::DW_CHILDREN_no;
         for (auto Attribute : AbbrvDecl.attributes()) {
           DWARFYAML::AttributeAbbrev AttAbrv;
           AttAbrv.Attribute = Attribute.Attr;
           AttAbrv.Form = Attribute.Form;
           if (AttAbrv.Form == dwarf::DW_FORM_implicit_const)
             AttAbrv.Value = Attribute.getImplicitConstValue();
           Abbrv.Attributes.push_back(AttAbrv);
         }
         Y.AbbrevDecls.push_back(Abbrv);
       }
     }
   }
+
+  return Error::success();
 }
 
-void dumpDebugStrings(DWARFContext &DCtx, DWARFYAML::Data &Y) {
+Error dumpDebugStrings(DWARFContext &DCtx, DWARFYAML::Data &Y) {
   StringRef RemainingTable = DCtx.getDWARFObj().getStrSection();
   while (RemainingTable.size() > 0) {
     auto SymbolPair = RemainingTable.split('\0');
     RemainingTable = SymbolPair.second;
     Y.DebugStrings.push_back(SymbolPair.first);
   }
+
+  return Error::success();
 }
 
 Error dumpDebugARanges(DWARFContext &DCtx, DWARFYAML::Data &Y) {
   DWARFDataExtractor ArangesData(DCtx.getDWARFObj().getArangesSection(),
                                  DCtx.isLittleEndian(), 0);
   uint64_t Offset = 0;
   DWARFDebugArangeSet Set;
   std::vector<DWARFYAML::ARange> DebugAranges;
if (IsGNUStyle)
       NewEntry.Descriptor = PubSectionData.getU8(&Offset);
     NewEntry.Name = PubSectionData.getCStr(&Offset);
     Y.Entries.push_back(NewEntry);
   }
 
   return Y;
 }
 
-void dumpDebugPubSections(DWARFContext &DCtx, DWARFYAML::Data &Y) {
+Error dumpDebugPubSections(DWARFContext &DCtx, DWARFYAML::Data &Y) {
   const DWARFObject &D = DCtx.getDWARFObj();
 
   const DWARFSection PubNames = D.getPubnamesSection();
   if (!PubNames.Data.empty())
     Y.PubNames = dumpPubSection(DCtx, PubNames, /*IsGNUStyle=*/false);
 
   const DWARFSection PubTypes = D.getPubtypesSection();
   if (!PubTypes.Data.empty())
     Y.PubTypes = dumpPubSection(DCtx, PubTypes, /*IsGNUStyle=*/false);
 
   const DWARFSection GNUPubNames = D.getGnuPubnamesSection();
   if (!GNUPubNames.Data.empty())
     // TODO: Test dumping .debug_gnu_pubnames section.
     Y.GNUPubNames = dumpPubSection(DCtx, GNUPubNames, /*IsGNUStyle=*/true);
 
   const DWARFSection GNUPubTypes = D.getGnuPubtypesSection();
   if (!GNUPubTypes.Data.empty())
     // TODO: Test dumping .debug_gnu_pubtypes section.
     Y.GNUPubTypes = dumpPubSection(DCtx, GNUPubTypes, /*IsGNUStyle=*/true);
+
+  return Error::success();
 }
 
-void dumpDebugInfo(DWARFContext &DCtx, DWARFYAML::Data &Y) {
+Error dumpDebugInfo(DWARFContext &DCtx, DWARFYAML::Data &Y) {
   for (const auto &CU : DCtx.compile_units()) {
     DWARFYAML::Unit NewUnit;
     NewUnit.Format = CU->getFormat();
     NewUnit.Length = CU->getLength();
     NewUnit.Version = CU->getVersion();
     if (NewUnit.Version >= 5)
       NewUnit.Type = (dwarf::UnitType)CU->getUnitType();
     NewUnit.AbbrOffset = CU->getAbbreviations()->getOffset();
for (auto DIE : CU->dies()) {
       auto AbbrevDecl = DIE.getAbbreviationDeclarationPtr();
       if (AbbrevDecl) {
         for (const auto &AttrSpec : AbbrevDecl->attributes()) {
           DWARFYAML::FormValue NewValue;
           NewValue.Value = 0xDEADBEEFDEADBEEF;
           DWARFDie DIEWrapper(CU.get(), &DIE);
           auto FormValue = DIEWrapper.find(AttrSpec.Attr);
           if (!FormValue)
-            return;
+            return Error::success();
           auto Form = FormValue.getValue().getForm();
           bool indirect = false;
           do {
             indirect = false;
             switch (Form) {
             case dwarf::DW_FORM_addr:
             case dwarf::DW_FORM_GNU_addr_index:
               if (auto Val = FormValue.getValue().getAsAddress())
for (auto DIE : CU->dies()) {
           NewEntry.Values.push_back(NewValue);
         }
       }
 
       NewUnit.Entries.push_back(NewEntry);
     }
     Y.CompileUnits.push_back(NewUnit);
   }
+
+  return Error::success();
 }
 
 bool dumpFileEntry(DataExtractor &Data, uint64_t &Offset,
                    DWARFYAML::File &File) {
   File.Name = Data.getCStr(&Offset);
   if (File.Name.empty())
     return false;
   File.DirIdx = Data.getULEB128(&Offset);
   File.ModTime = Data.getULEB128(&Offset);
   File.Length = Data.getULEB128(&Offset);
   return true;
 }
 
-void dumpDebugLines(DWARFContext &DCtx, DWARFYAML::Data &Y) {
+Error dumpDebugLines(DWARFContext &DCtx, DWARFYAML::Data &Y) {
   for (const auto &CU : DCtx.compile_units()) {
     auto CUDIE = CU->getUnitDIE();
     if (!CUDIE)
       continue;
     if (auto StmtOffset =
             dwarf::toSectionOffset(CUDIE.find(dwarf::DW_AT_stmt_list))) {
       DWARFYAML::LineTable DebugLines;
       DataExtractor LineData(DCtx.getDWARFObj().getLineSection().Data,
if (auto StmtOffset =
               NewOp.StandardOpcodeData.push_back(LineData.getULEB128(&Offset));
           }
         }
         DebugLines.Opcodes.push_back(NewOp);
       }
       Y.DebugLines.push_back(DebugLines);
     }
   }
+
+  return Error::success();
 }
 
 llvm::Error dwarf2yaml(DWARFContext &DCtx, DWARFYAML::Data &Y) {
-  dumpDebugAbbrev(DCtx, Y);
-  dumpDebugStrings(DCtx, Y);
+  if (Error E = dumpDebugAbbrev(DCtx, Y))
+    return E;
+  if (Error E = dumpDebugStrings(DCtx, Y))
+    return E;
   if (Error E = dumpDebugARanges(DCtx, Y))
     return E;
   if (Error E = dumpDebugRanges(DCtx, Y))
     return E;
-  dumpDebugPubSections(DCtx, Y);
-  dumpDebugInfo(DCtx, Y);
-  dumpDebugLines(DCtx, Y);
+  if (Error E = dumpDebugPubSections(DCtx, Y))
+    return E;
+  if (Error E = dumpDebugInfo(DCtx, Y))
+    return E;
+  if (Error E = dumpDebugLines(DCtx, Y))
+    return E;
   return ErrorSuccess();
 }