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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D85505#2206200 <https://reviews.llvm.org/D85505#2206200>, @Higuoxing wrote:

> In D85505#2206194 <https://reviews.llvm.org/D85505#2206194>, @jhenderson wrote:
>
>> In D85505#2206165 <https://reviews.llvm.org/D85505#2206165>, @Higuoxing wrote:
>>
>>> In D85505#2206076 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85505