This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by avl on Sep 24 2020, 4:02 AM.

Details

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.

Diff Detail

Event Timeline

avl created this revision.Sep 24 2020, 4:02 AM
Herald added a project: Restricted Project. · View Herald Transcript
avl requested review of this revision.Sep 24 2020, 4:02 AM
avl updated this revision to Diff 294103.Sep 24 2020, 9:53 AM

renamed variable.

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 added inline comments.Sep 25 2020, 12:38 AM
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 added inline comments.Sep 25 2020, 12:39 AM
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 added inline comments.Sep 25 2020, 1:52 AM
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 added inline comments.Sep 25 2020, 3:10 AM
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 added inline comments.Sep 25 2020, 3:16 AM
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 added inline comments.Sep 25 2020, 3:22 AM
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 added inline comments.Sep 25 2020, 3:34 AM
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 added inline comments.Sep 25 2020, 3:38 AM
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 added inline comments.Sep 25 2020, 3:51 AM
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 added inline comments.Sep 25 2020, 3:51 AM
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 updated this revision to Diff 294268.Sep 25 2020, 3:54 AM

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

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 accepted this revision.Sep 25 2020, 4:36 AM

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.

This revision is now accepted and ready to land.Sep 25 2020, 4:36 AM
grimar added inline comments.Sep 25 2020, 4:40 AM
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.

I have no more comments. This LG.

alexander-shaposhnikov added inline comments.
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

This revision was automatically updated to reflect the committed changes.