[PATCH] D49964: [DWARF] Refactor DWARF classes to use unified error reporting. NFC.

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a reviewer: jhenderson.
jhenderson added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:398-399
   if (!AS.isValidOffsetForDataOfSize(*Offset, AugmentationStringSize))
-    return make_error<StringError>(
-        "Section too small: cannot read header augmentation.",
-        inconvertibleErrorCode());
+    return createStringError(errc::invalid_argument,
+                       "Section too small: cannot read header augmentation.");
   AugmentationString.resize(AugmentationStringSize);
----------------
Nit: The indentation looks a bit weird here. Is this how clang-format formats this?


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:516
+    if (!Abbrevs.insert(std::move(*AbbrevOr)).second)
+      return createStringError(errc::file_exists,
+                               "Duplicate abbreviation code.");
----------------
No. Don't use this errc value here. file_exists is for files, as the name implies. This should be invalid_argument again, I guess (there isn't a code for "malformed input file").


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:617
     if (!Value.extractValue(AS, Offset, FormParams))
-      return make_error<StringError>("Error extracting index attribute values.",
-                                     inconvertibleErrorCode());
+      return createStringError(inconvertibleErrorCode(),
+                               "Error extracting index attribute values.");
----------------
You missed an inconvertibleErrorCode here...

Actually, looking at the function itself, it can NEVER fail (always returns true), so this is technically dead code. I'd consider changing the signature of the function to return void. But that should be a separate change. If you are going to do this, I'm okay with it continuing to be inconvertibleErrorCode, but otherwise, maybe io_error?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1583
   if (!TargetLookupError.empty())
-    return make_error<StringError>(TargetLookupError, inconvertibleErrorCode());
+    return createStringError(inconvertibleErrorCode(),
+                             TargetLookupError.c_str());
----------------
This should be invalid_argument, looking at the lookupTarget function (which actually should just return an Error itself rather than setting a string...)


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:51
       default:
-        return make_error<StringError>(
-            "Invalid primary CFI opcode",
-            std::make_error_code(std::errc::illegal_byte_sequence));
+        return createStringError(errc::illegal_byte_sequence,
+                                 "Invalid primary CFI opcode 0x%" PRIx8,
----------------
I didn't know about this errc value. Good spot! Maybe it's worth using in one or two other cases where the input is malformed? What do you think?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:629
       if (*OffsetPtr - ExtOffset != Len)
-        return createError("unexpected line op length at offset 0x%8.8" PRIx32
+        return createStringError(errc::invalid_argument,
+                           "unexpected line op length at offset 0x%8.8" PRIx32
----------------
This might want to be illegal byte sequence.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:837
     RecoverableErrorCallback(
-        createError("last sequence in debug line table is not terminated!"));
+        createStringError(errc::invalid_argument,
+                    "last sequence in debug line table is not terminated!"));
----------------
This should be illegal_byte_sequence, I reckon.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp:93
   default:
-    return createError("unknown rnglists encoding 0x%" PRIx32
+    return createStringError(errc::invalid_argument,
+                       "unknown rnglists encoding 0x%" PRIx32
----------------
Maybe not_supported here?


Repository:
  rL LLVM

https://reviews.llvm.org/D49964