[llvm-objcopy][NFC] refactor error handling. part 2.

[avl]

Summary

Remove usages of special error reporting functions(error(),
reportError()). This patch is extracted from D87987.
Errors are reported as Expected<>/Error returning values.
This part is for COFF subfolder of llvm-objcopy.

Testing: check-all.

[avl]

renamed variable.

[mstorsjo]

LGTM overall, one change that seems superfluous though.

I'll leave out the formal approval for now, to wait if @jhenderson has more input.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

281

This bit seems like unrelated renamings?

llvm/tools/llvm-objcopy/COFF/Object.cpp

63

This implementation looks ok, but it's not entirely trivial - hopefully the tests cover all relevant cases here.

Does this happen to be similar to what the implementation of std::remove_if looks like?

[jhenderson]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

182–183

I think lambdas are one of the exceptions to the "don't use auto rule", so you can simpify this to auto ToRemove. Of course, you could also just leave it inline it like it was before, but I don't mind if you'd prefer to keep it separate.

llvm/tools/llvm-objcopy/COFF/Object.cpp

63

Similar comment - there might be a simpler way than this:

Error Errs = Error::success();
Symbols.erase(std:::remove_if(std::begin(Symbols), std::end(Symbols),
              [ToRemove, &Errs](const Symbol &Sym) {
    Expected<bool> ShouldRemove = ToRemove(Sym);
    if (ShouldRemove) {
      Errs = joinErrors(std::move(Errs), ShouldRemove.takeError());
      return false;
    }
    return *ShouldRemove;
  }), std::end(Symbols));
return Errs;

(I haven't tested for syntax and exact usage errors, but this should be approximately correct). Essentially, we intercept the errors from the ToRemove function, join them all together, and then pass them all up the stack. The iteration logic remains unchanged - we've just added error handling.

[jhenderson]

llvm/tools/llvm-objcopy/COFF/Object.cpp

63

I missed the call to updateSymbols here - make sure to include that before the return Errs; line.

[grimar]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

56

You don't need this move I believe. Move constructor of Expected<> will be called anyways.

281

And ObjOrErr was probably a better name. It is common to name Expected variables as "<something>OrErr".

[avl]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

281

During previous review it was asked to not to use such naming style for similar case - https://reviews.llvm.org/D88113#inline-818190 . So I changed it to match with that requirement.

[avl]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

56

It looks like in gcc case it does not work - https://reviews.llvm.org/rG4da6927de47074f56531c2e7e2eecc4d6a1f09ec

281

It was argued that such a naming match with ErrorOr class. Since it is not the case here - it looks ambitions. https://reviews.llvm.org/D88113#inline-818190

[mstorsjo]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

281

While these argument seem contradictory, the main point in this argument here, is that to implement what this patch sets out to do, you don't need to touch these lines at all, they already do exactly what they need to.

If you want to rename unrelated variables for consistency, that's a separate change.

[grimar]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

56

Have you checked this place? The commit message does not say what was the BB error.
But in LLVM code we have many places that don't have std::move and feel fine.

https://github.com/llvm/llvm-project/blob/master/llvm/lib/Object/ArchiveWriter.cpp#L510
https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-exegesis/llvm-exegesis.cpp#L287
https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-readobj/ELFDumper.cpp#L582

Perhaps, the issue was related to std::unique_ptr.

[avl]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

56

yes, the error was about unique_ptr:

MachOReader.cpp:337:10: error: could not convert ‘Obj’ from ‘std::unique_ptr<llvm::objcopy::macho::Object>’ to ‘llvm::Expected<std::unique_ptr<llvm::objcopy::macho::Object> >’

return Obj;

[grimar]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

281

If you want to rename unrelated variables for consistency, that's a separate change.

Right.

It was argued that such a naming match with ErrorOr class. Since it is not the case here - it looks ambitions.

Ok. I'd like to answer this. I disagree that it looks ambitions, because when you write something like:

Expected<ArrayRef<..>> Data = function();

The Data is not really data, but something that might contain either data or an error (Expected::takeError()).
In LLVMs code "OrErr" suffix is widely used and we don't have anything else, like "ExpectedData" or alike I think.

It is also not so rare to see the following:

Expected<ArrayRef<..>> DataOrErr = function();
if (!DataOrErr)
  return DataOrErr.takeError();
ArrayRef<..> Data = *DataOrErr;

Where it is obvious that DataOrErr and Data are completely different things.

But now I see that llvm-objcopy code is inconsistent and uses different styles when using Expected<>. And the rest of LLVM code is also not consistent.

[avl]

llvm/tools/llvm-objcopy/COFF/Object.cpp

63

The iteration logic seems changed. Version from the current patch will stop iterating as soon as first error encountered(the same as without patch). The suggested version would iterate all Symbols and accumulate the errors. But that is probably OK.

[avl]

addressed comments(removed variable renaming, removed std::move() usage, used std::remove_if() implementation.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

56

@grimar:

https://reviews.llvm.org/D43322

if there is a mismatch of the return type and the type of the local variable then std::move can be useful

if I am not mistaken, in fact, https://github.com/llvm/llvm-project/blob/master/llvm/lib/Object/ArchiveWriter.cpp#L510 is not a good example.

281

P.S. Doing quick grep of the repository shows that there is no consistency in naming Expected<..> variables. Certainly this is not ideal (I can see arguments for the both sides though personally don't have a very strong opinion on this).

[jhenderson]

LGTM, but please make sure others are happy too.

llvm/tools/llvm-objcopy/COFF/Object.cpp

63

Yes, I think it's probably fine. If there was a simple way to stop collecting errors and bail out after a certain point, that would also be fine, but I think the code is better off like this otherwise.

[grimar]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

56

if there is a mismatch of the return type and the type of the local variable then std::move can be useful

Expected<> has a move constructor that is expected to handle the case here I believe (I've tested that MSVS 2017 calls it):

  template <typename OtherT>
  Expected(OtherT &&Val,
           std::enable_if_t<std::is_convertible<OtherT, T>::value> * = nullptr)
      : HasError(false)
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
        // Expected is unchecked upon construction in Debug builds.
        ,
        Unchecked(true)
#endif
  {
    new (getStorage()) storage_type(std::forward<OtherT>(Val));
  }

My point mostly is based on the fact that using of std::move when returning a named local variable is generally should be avoided, because might affect compiler optimizations.

[grimar]

I have no more comments. This LG.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

56

Apart from the diff which I mentioned above (D43322) there is also
D70963 which contains some extra context.

I'm not really sure what the current policy is / would be interesting to find out (perhaps, it depends on which compiler versions LLVM's build currently supports), but I'm not the best person to answer this question.
cc: @rsmith

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

 static uint64_t getNextRVA(const Object &Obj) {
   if (Obj.getSections().empty())
     return 0;
   const Section &Last = Obj.getSections().back();
   return alignTo(Last.Header.VirtualAddress + Last.Header.VirtualSize,
                  Obj.IsPE ? Obj.PeHeader.SectionAlignment : 1);
 }
 
-static std::vector<uint8_t> createGnuDebugLinkSectionContents(StringRef File) {
-  ErrorOr<std::unique_ptr<MemoryBuffer>> LinkTargetOrErr =
-      MemoryBuffer::getFile(File);
-  if (!LinkTargetOrErr)
-    error("'" + File + "': " + LinkTargetOrErr.getError().message());
-  auto LinkTarget = std::move(*LinkTargetOrErr);
+static Expected<std::vector<uint8_t>>
+createGnuDebugLinkSectionContents(StringRef File) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> FileData = MemoryBuffer::getFile(File);
+  if (!FileData)
+    return createFileError(File, FileData.getError());
+  auto LinkTarget = std::move(*FileData);
   uint32_t CRC32 = llvm::crc32(arrayRefFromStringRef(LinkTarget->getBuffer()));
 
   StringRef FileName = sys::path::filename(File);
   size_t CRCPos = alignTo(FileName.size() + 1, 4);
   std::vector<uint8_t> Data(CRCPos + 4);
   memcpy(Data.data(), FileName.data(), FileName.size());
   support::endian::write32le(Data.data() + CRCPos, CRC32);
   return Data;
 }
 
 // Adds named section with given contents to the object.
 static void addSection(Object &Obj, StringRef Name, ArrayRef<uint8_t> Contents,
                        uint32_t Characteristics) {
   bool NeedVA = Characteristics & (IMAGE_SCN_MEM_EXECUTE | IMAGE_SCN_MEM_READ |
                                    IMAGE_SCN_MEM_WRITE);
 
static void addSection(Object &Obj, StringRef Name, ArrayRef<uint8_t> Contents,
   Sec.Header.PointerToLinenumbers = 0;
   // Sec.Header.NumberOfRelocations is filled in by the writer.
   Sec.Header.NumberOfLinenumbers = 0;
   Sec.Header.Characteristics = Characteristics;
 
   Obj.addSections(Sec);
 }
 
-static void addGnuDebugLink(Object &Obj, StringRef DebugLinkFile) {
-  std::vector<uint8_t> Contents =
+static Error addGnuDebugLink(Object &Obj, StringRef DebugLinkFile) {
+  Expected<std::vector<uint8_t>> Contents =
       createGnuDebugLinkSectionContents(DebugLinkFile);
-  addSection(Obj, ".gnu_debuglink", Contents,
+  if (!Contents)
+    return Contents.takeError();
+
+  addSection(Obj, ".gnu_debuglink", *Contents,
              IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ |
                  IMAGE_SCN_MEM_DISCARDABLE);
+
+  return Error::success();
 }
 
 static void setSectionFlags(Section &Sec, SectionFlag AllFlags) {
   // Need to preserve alignment flags.
   const uint32_t PreserveMask =
       IMAGE_SCN_ALIGN_1BYTES | IMAGE_SCN_ALIGN_2BYTES | IMAGE_SCN_ALIGN_4BYTES |
       IMAGE_SCN_ALIGN_8BYTES | IMAGE_SCN_ALIGN_16BYTES |
       IMAGE_SCN_ALIGN_32BYTES | IMAGE_SCN_ALIGN_64BYTES |
if (Error E = Obj.markSymbols())
       return E;
 
   for (Symbol &Sym : Obj.getMutableSymbols()) {
     auto I = Config.SymbolsToRename.find(Sym.Name);
     if (I != Config.SymbolsToRename.end())
       Sym.Name = I->getValue();
   }
 
-  // Actually do removals of symbols.
-  Obj.removeSymbols([&](const Symbol &Sym) {
+  std::function<Expected<bool>(const Symbol &)> ToRemove =
+      [&](const Symbol &Sym) -> Expected<bool> {
     // For StripAll, all relocations have been stripped and we remove all
     // symbols.
     if (Config.StripAll || Config.StripAllGNU)
       return true;
 
     if (Config.SymbolsToRemove.matches(Sym.Name)) {
       // Explicitly removing a referenced symbol is an error.
       if (Sym.Referenced)
-        reportError(Config.OutputFilename,
-                    createStringError(llvm::errc::invalid_argument,
-                                      "not stripping symbol '%s' because it is "
-                                      "named in a relocation",
-                                      Sym.Name.str().c_str()));
+        return createStringError(
+            llvm::errc::invalid_argument,
+            "'" + Config.OutputFilename + "': not stripping symbol '" +
+                Sym.Name.str() + "' because it is named in a relocation");
       return true;
     }
 
     if (!Sym.Referenced) {
       // With --strip-unneeded, GNU objcopy removes all unreferenced local
       // symbols, and any unreferenced undefined external.
       // With --strip-unneeded-symbol we strip only specific unreferenced
       // local symbol instead of removing all of such.
       if (Sym.Sym.StorageClass == IMAGE_SYM_CLASS_STATIC ||
           Sym.Sym.SectionNumber == 0)
         if (Config.StripUnneeded ||
             Config.UnneededSymbolsToRemove.matches(Sym.Name))
           return true;
 
       // GNU objcopy keeps referenced local symbols and external symbols
       // if --discard-all is set, similar to what --strip-unneeded does,
       // but undefined local symbols are kept when --discard-all is set.
       if (Config.DiscardMode == DiscardType::All &&
           Sym.Sym.StorageClass == IMAGE_SYM_CLASS_STATIC &&
           Sym.Sym.SectionNumber != 0)
         return true;
     }
 
     return false;
-  });
+  };
+
+  // Actually do removals of symbols.
+  if (Error Err = Obj.removeSymbols(ToRemove))
+    return Err;
 
   if (!Config.SetSectionFlags.empty())
     for (Section &Sec : Obj.getMutableSections()) {
       const auto It = Config.SetSectionFlags.find(Sec.Name);
       if (It != Config.SetSectionFlags.end())
         setSectionFlags(Sec, It->second.NewFlags);
     }
 
for (const auto &Flag : Config.AddSection) {
     addSection(
         Obj, SecName,
         makeArrayRef(reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
                      Buf->getBufferSize()),
         IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_ALIGN_1BYTES);
   }
 
   if (!Config.AddGnuDebugLink.empty())
-    addGnuDebugLink(Obj, Config.AddGnuDebugLink);
+    if (Error E = addGnuDebugLink(Obj, Config.AddGnuDebugLink))
+      return E;
 
   if (Config.AllowBrokenLinks || !Config.BuildIdLinkDir.empty() ||
       Config.BuildIdLinkInput || Config.BuildIdLinkOutput ||
       !Config.SplitDWO.empty() || !Config.SymbolsPrefix.empty() ||
       !Config.AllocSectionsPrefix.empty() || !Config.DumpSection.empty() ||
       !Config.KeepSection.empty() || Config.NewSymbolVisibility ||
       !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToKeep.empty() ||
       !Config.SymbolsToLocalize.empty() || !Config.SymbolsToWeaken.empty() ||
static Error handleArgs(const CopyConfig &Config, Object &Obj) {
   }
 
   return Error::success();
 }
 
 Error executeObjcopyOnBinary(const CopyConfig &Config, COFFObjectFile &In,
                              Buffer &Out) {
   COFFReader Reader(In);
-  Expected<std::unique_ptr<Object>> ObjOrErr = Reader.create();
-  if (!ObjOrErr)
-    return createFileError(Config.InputFilename, ObjOrErr.takeError());
-  Object *Obj = ObjOrErr->get();
+  Expected<std::unique_ptr<Object>> O = Reader.create();
+  if (!O)
+    return createFileError(Config.InputFilename, O.takeError());
+  Object *Obj = O->get();
   assert(Obj && "Unable to deserialize COFF object");
   if (Error E = handleArgs(Config, *Obj))
     return createFileError(Config.InputFilename, std::move(E));
   COFFWriter Writer(*Obj, Out);
   if (Error E = Writer.write())
     return createFileError(Config.OutputFilename, std::move(E));
   return Error::success();
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm

llvm/tools/llvm-objcopy/COFF/Object.h

struct Object {
   // of symbols itself.
   iterator_range<std::vector<Symbol>::iterator> getMutableSymbols() {
     return make_range(Symbols.begin(), Symbols.end());
   }
 
   const Symbol *findSymbol(size_t UniqueId) const;
 
   void addSymbols(ArrayRef<Symbol> NewSymbols);
-  void removeSymbols(function_ref<bool(const Symbol &)> ToRemove);
+  Error removeSymbols(function_ref<Expected<bool>(const Symbol &)> ToRemove);
 
   // Set the Referenced field on all Symbols, based on relocations in
   // all sections.
   Error markSymbols();
 
   ArrayRef<Section> getSections() const { return Sections; }
   // This allows mutating individual Sections, but not mutating the list
   // of sections itself.

llvm/tools/llvm-objcopy/COFF/Object.cpp

 
 const Symbol *Object::findSymbol(size_t UniqueId) const {
   auto It = SymbolMap.find(UniqueId);
   if (It == SymbolMap.end())
     return nullptr;
   return It->second;
 }
 
-void Object::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
-  Symbols.erase(
-      std::remove_if(std::begin(Symbols), std::end(Symbols),
-                     [ToRemove](const Symbol &Sym) { return ToRemove(Sym); }),
-      std::end(Symbols));
+Error Object::removeSymbols(
+    function_ref<Expected<bool>(const Symbol &)> ToRemove) {
+  std::vector<Symbol>::iterator Current = Symbols.begin();
+  std::vector<Symbol>::iterator End = Symbols.end();
+  Optional<std::vector<Symbol>::iterator> LastRemoved;
+
+  for (; Current != End; Current++) {
+    Expected<bool> Remove = ToRemove(*Current);
+    if (!Remove)
+      return Remove.takeError();
+
+    if (*Remove) {
+      if (!LastRemoved)
+        LastRemoved = Current;
+    } else {
+      if (LastRemoved) {
+        **LastRemoved = std::move(*Current);
+        (*LastRemoved)++;
+      }
+    }
+  }
+
+  if (LastRemoved)
+    Symbols.erase(*LastRemoved, Symbols.end());
+
   updateSymbols();
+  return Error::success();
 }
 
 Error Object::markSymbols() {
   for (Symbol &Sym : Symbols)
     Sym.Referenced = false;
   for (const Section &Sec : Sections) {
     for (const Relocation &R : Sec.Relocs) {
       auto It = SymbolMap.find(R.Target);