This is an archive of the discontinued LLVM Phabricator instance.

[lld] unified COFF and ELF error handling on new Common/ErrorHandler
ClosedPublic

Authored by inglorion on Oct 24 2017, 2:08 PM.

Details

Summary

The COFF linker and the ELF linker have long had similar but separate
Error.h and Error.cpp files to implement error handling. This change
introduces new error handling code in Common/ErrorHandler.h, changes the
COFF and ELF linkers to use it, and removes the old, separate
implementations.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Oct 24 2017, 2:08 PM

Summary of the changes:

  • Instead of #include "Error.h", #include "lld/Common/ErrorHandler.h".
  • Instead of ColorDiagnostics, ErrorLimit, and FatalWarnings being properties of the Config object, they are now set on the ErrorHandler object.
  • Removed the fatal() variants that take Error or std::error_code. This was previously done for ELF, so I've done it here for COFF too.
  • Instead of ErrorCount as a global variable, the value is now retrieved by calling the errorCount() function.

I chose ErrorHandler instead of Error as the name of the header file, because there is already an Error.h in lld/Core, and also in LLVM.

ruiu added inline comments.Oct 24 2017, 9:06 PM
lld/include/lld/Common/ErrorHandler.h
50–81 ↗(On Diff #120133)

These Java-ish method chains are not very lld-ish. It is better to directly expose the members.

85–93 ↗(On Diff #120133)

You can use C++11-style member initialization because all these values are constant. E.g.

bool ColorDiagnostics = false;

converted setters to public fields

ruiu added inline comments.Oct 25 2017, 2:35 PM
lld/COFF/Driver.cpp
797 ↗(On Diff #120292)

clang-format?

lld/ELF/DriverUtils.cpp
56 ↗(On Diff #120292)

You don't need to pass ErrorHandler because you can call errorHandler() in this function.

lld/include/lld/Common/ErrorHandler.h
50–54 ↗(On Diff #120292)

I believe you are not using this ctor.

inglorion added inline comments.Oct 25 2017, 2:52 PM
lld/include/lld/Common/ErrorHandler.h
50–54 ↗(On Diff #120292)

The default ctor delegates to it. I suppose I could just only have a default ctor.

ruiu added inline comments.Oct 25 2017, 2:53 PM
lld/include/lld/Common/ErrorHandler.h
50–54 ↗(On Diff #120292)

Yeah, and initializing a member with an ELF-ish default value seems a bit odd if you move this file to Common directory.

inglorion added inline comments.Oct 25 2017, 2:58 PM
lld/include/lld/Common/ErrorHandler.h
50–54 ↗(On Diff #120292)

Yeah. The thinking is basically that this is the common case, with the Windows variant being the odd one out, but I'm happy to do it a different way if you have suggestions. I wasn't able to think of a way I really like better.

inglorion updated this revision to Diff 120324.Oct 25 2017, 3:02 PM
  • Removed ErrorHandler parameter to handleColorDiagnostics.
  • Cut number of constructors for ErrorHandler down to one.
ruiu added inline comments.Oct 25 2017, 3:06 PM
lld/include/lld/Common/ErrorHandler.h
41–43 ↗(On Diff #120324)

You can initialize here with = ....

50–54 ↗(On Diff #120292)

Maybe, just initialize it to "too many errors emitted"?

inglorion updated this revision to Diff 120326.Oct 25 2017, 3:23 PM
  • Initialize all fields in their definition instead of in the contructor.
  • Replaced default ErrorLimitExceededMsg with something more generic.
  • clang-format
ruiu accepted this revision.Oct 25 2017, 3:24 PM

LGTM

This revision is now accepted and ready to land.Oct 25 2017, 3:24 PM
This revision was automatically updated to reflect the committed changes.
lld/trunk/ELF/EhFrame.cpp