[PATCH] D87987: [llvm-objcopy][NFC] refactor error handling. part 3.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: MaskRay.
jhenderson added a comment.

Before you go too much further with this, I'd like some comments from others on the approach. The fact that this code is far more extensive and has far more error paths suggests to me that we might be on the wrong approach here, and perhaps we should actually be using the error handler callback approach after all. @grimar/@rupprecht/@MaskRay - any thoughts?

If we had an error handler callback, we'd probably want it added to the objcopy config class, to avoid having to pass the callbacks themselves everywhere (only as far as the point where the config is created).



================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:332
   DenseMap<SectionBase *, SectionBase *> FromTo;
   for (SectionBase *S : ToReplace)
+    if (Expected<SectionBase *> NewSection = addSection(S))
----------------
I'd find it more readable now that this is a non-trivial for loop, if it were to have braces.`monospaced text`


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:54
 Error SectionBase::removeSectionReferences(
-    bool AllowBrokenLinks,
-    function_ref<bool(const SectionBase *)> ToRemove) {
+    bool AllowBrokenLinks, function_ref<bool(const SectionBase *)> ToRemove) {
   return Error::success();
----------------
Unrelated clang-formatting?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:549-558
 CompressedSection::CompressedSection(const SectionBase &Sec,
-                                     DebugCompressionType CompressionType)
+                                     DebugCompressionType CompressionType,
+                                     std::error_code &ErrorCode)
     : SectionBase(Sec), CompressionType(CompressionType),
       DecompressedSize(Sec.OriginalData.size()), DecompressedAlign(Sec.Align) {
-  if (Error E = zlib::compress(
+  if (Error Err = zlib::compress(
           StringRef(reinterpret_cast<const char *>(OriginalData.data()),
----------------
This isn't the way to report the error back. The message will have lost fidelity. You should probably change this to use the fallible constructor idiom - see https://llvm.org/docs/ProgrammersManual.html#fallible-constructors. That way, the error can be propagated back to the caller in the way it always was.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:633-634
+    setSymTab(*Sec);
+  } else
+    return Sec.takeError();
+
----------------
See https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements: "readability is also harmed if an if/else chain does not use braced bodies for either all or none of its members".

But actually, this is one of several examples in this patch where you'd be better off doing something like this:

```
Expected<SymbolTableSection *> Sec = SecTable.getSectionOfType<SymbolTableSection>(
              Link,
              "Link field value " + Twine(Link) + " in section " + Name +
                  " is invalid",
              "Link field value " + Twine(Link) + " in section " + Name +
                  " is not a symbol table"));
if (!Sec)
  return Sec.takeError();

setSymTab(*Sec);
Symbols->setShndxTable(this);
return Error::success();
```

In this case, your code for what to do in the "good" case is kept all next to each other, rather than partially split between the if body and the post if/else bit.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:776
   Size = 0;
-  setStrTab(SecTable.getSectionOfType<StringTableSection>(
-      Link,
-      "Symbol table has link index of " + Twine(Link) +
-          " which is not a valid index",
-      "Symbol table has link index of " + Twine(Link) +
-          " which is not a string table"));
+  if (Expected<StringTableSection *> Sec =
+          SecTable.getSectionOfType<StringTableSection>(
----------------
Same as above.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1099-1104
+  if (Expected<SectionBase *> Sec = SecTable.getSection(
+          Link, "Link field value " + Twine(Link) + " in section " + Name +
+                    " is invalid")) {
+    LinkSection = *Sec;
+  } else
+    return Sec.takeError();
----------------
Same as above.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1386
+
+  auto Headers = HeadersFile.program_headers();
+  if (!Headers)
----------------
I'd avoid the `auto` here. It's not clear to me what sort of thing `program_headers` returns (in this case an `Expected`).


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1485-1491
     uint32_t Index = support::endian::read32<ELFT::TargetEndianness>(Word);
-    GroupSec->addMember(SecTable.getSection(
-        Index, "group member index " + Twine(Index) + " in section '" +
-                   GroupSec->Name + "' is invalid"));
+    if (Expected<SectionBase *> Sec = SecTable.getSection(
+            Index, "group member index " + Twine(Index) + " in section '" +
+                       GroupSec->Name + "' is invalid")) {
+      GroupSec->addMember(*Sec);
+    } else
+      return Sec.takeError();
----------------
Same as above. This is also different to the style used elsewhere in this function.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1601-1604
+      if (SymByIndex)
+        ToAdd.RelocSymbol = *SymByIndex;
+      else
+        return SymByIndex.takeError();
----------------
Avoid unnecessary else.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1718
   uint32_t Index = 0;
-  for (const auto &Shdr : unwrapOrError(ElfFile.sections())) {
+  auto Sections = ElfFile.sections();
+  if (!Sections)
----------------
Don't use `auto`. The type isn't obvious.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1722
+
+  for (const auto &Shdr : *Sections) {
     if (Index == 0) {
----------------
As you are changing this line, I'd get rid of this `auto` too. Same goes elsewhere.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1807
     if (auto RelSec = dyn_cast<RelocationSection>(&Sec)) {
-      auto Shdr = unwrapOrError(ElfFile.sections()).begin() + RelSec->Index;
-      if (RelSec->Type == SHT_REL)
-        initRelocations(RelSec, Obj.SymbolTable,
-                        unwrapOrError(ElfFile.rels(*Shdr)));
-      else
-        initRelocations(RelSec, Obj.SymbolTable,
-                        unwrapOrError(ElfFile.relas(*Shdr)));
+      auto Sections = ElfFile.sections();
+      if (!Sections)
----------------
Here and throughout this function, don't use `auto` if the type isn't obvious from the RHS, unless it's an iterator.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:114-117
+  virtual Error visit(const Section &Sec) override;
+  virtual Error visit(const OwnedDataSection &Sec) override;
+  virtual Error visit(const StringTableSection &Sec) override;
+  virtual Error visit(const DynamicRelocationSection &Sec) override;
----------------
No need to specify `virtual` when a function is marked as `override`.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:59-63
 ErrorSuccess reportWarning(Error E) {
   assert(E);
   WithColor::warning(errs(), ToolName) << toString(std::move(E)) << '\n';
   return Error::success();
 }
----------------
This might be an indication that you should use an error handler after all. You don't really want your library code printing warnings directly itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87987