error_code for dynamically generated error messages

[kledzik]

Summary

This proposed patch adds a new kind of error_code, one which allows arbitrary
message strings.

error_code is based on the model that errors are enumerable and you may want
to programmatically handle when some function called fails with some specific
error_code.

lld uses ErrorOr<> extensively which is based on error_code. But the errors
that arise when parsing .o files or linking them are not usefully enumerable.
That is, there is nothing that can be programmatic done and you really want
an error message richer than “bad object format”. For instance, a richer
message might be “relocation 14 contains invalid relocation type (23)”.

This patch creates a subclass of error_category which dynamically associcates
a value and string. I hope this is not bending the error_code model too much.
If the idea is good, should this be pushed down from lld into llvm/Support?

As an example usage, I updated lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp
to use make_dynamic_error_code().

[ruiu]

The idea looks good to me.

lib/Core/Error.cpp

182

What is this for?

190

Why don't you use std::mutex and std::lock_guard?

[Bigcheese]

One of the intents of ErrorOr was for this type of stuff to live there. This functionality was removed from ErrorOr because it was unused, but I would rather add it back to ErrorOr in some way than extend error_code like this.

I would also actually like to change ErrorOr to behave more like: https://github.com/boostcon/cppnow_presentations_2014/blob/master/files/expected.pdf?raw=true

The main part is to make the error type changeable and figure out a good default. Another feature that would make using ErrorOr easier is the monadic syntax with map.

[kledzik]

Michael, I can see how ErrorOr<> could be improved. But should we block this on that improvement? Or is can we do this in lld in a way that is easy to update once ErrorOr<> is improved? For instance, use ErrorOr<void> for return types instead of error_code and have all dynamic error strings constructed though one utility function that can be rewritten later?

lib/Core/Error.cpp

182

The value of zero is reserved to mean "no error". The zeroth slot in the vector is lazily allocated.

190

No good reason. Why does llvm have is own Mutex if we now use C++11 that has std::mutex?

[Bigcheese]

I suppose this is fine for now as it's a rather simple change. We can just change make_dynamic_error_code in the future.

lib/Core/Error.cpp

190

Historical reasons. For example we should switch to std::error_code eventually.

[• shankar.easwaran]

lib/Core/Error.cpp

180

Nit pick spell error of success

189

How do we use the vector of messages ? Are these for predefined messages ? If so should it be a vector of StringRef ?

[kledzik]

lib/Core/Error.cpp

189

Each time make_dynamic_error_code() is called it copies the string to the vector which dynamically creates a new error code value. So rather that fixed error values and messages, we get dynamic error values and messages. Of course, this only makes sense if the clients never look at the value (other than testing against zero).

[• shankar.easwaran]

LGTM

[atanasyan]

LGTM

[• rafael]

Will this work with the std::error_code once we switch?

As far as style goes I probably like the clang model best: pass an
DiagnosticsEngine down instead of passing the error back up. That way
we separate two concepts

• an error we want to report to the user: foo.o in bar.a is corrupted.
• an error condition we want to handle: open failed with no_such_file

in /usr/lib/libfoo.so. The caller will check the error_code and try
the next directory.

But I agree that this is isolated enough that it is fine if it also
works with std::error_code.

[kledzik]

Committed in r209681

include/lld/Core/Error.h

 //===- Error.h - system_error extensions for lld ----------------*- C++ -*-===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This declares a new error_category for the lld library.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_CORE_ERROR_H
 #define LLD_CORE_ERROR_H
 
+#include "lld/Core/LLVM.h"
 #include "llvm/Support/system_error.h"
 
 namespace lld {
 
 const llvm::error_category &native_reader_category();
 
 enum class NativeReaderError {
   success = 0,
enum class ReaderError {
   success = 0,
   unknown_file_format = 1
 };
 
 inline llvm::error_code make_error_code(ReaderError e) {
   return llvm::error_code(static_cast<int>(e), ReaderErrorCategory());
 }
 
+
+/// Creates an error_code object that has associated with it an arbitrary
+/// error messsage.  The value() of the error_code will always be non-zero
+/// but its value is meaningless. The messsage() will be (a copy of) the 
+/// supplied error string.
+llvm::error_code make_dynamic_error_code(StringRef msg);
+llvm::error_code make_dynamic_error_code(const Twine &msg);
+
 } // end namespace lld
 
 namespace llvm {
 
 template <>
 struct is_error_code_enum<lld::NativeReaderError> : std::true_type {};
 template <> struct is_error_code_enum<lld::YamlReaderError> : std::true_type {};
 template <>
 struct is_error_code_enum<lld::LinkerScriptReaderError> : std::true_type {};
 template <> struct is_error_code_enum<lld::InputGraphError> : std::true_type {};
 template <> struct is_error_code_enum<lld::ReaderError> : std::true_type {};
 } // end namespace llvm
 
 #endif

lib/Core/Error.cpp

 //===- Error.cpp - system_error extensions for lld --------------*- C++ -*-===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lld/Core/Error.h"
 
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/Mutex.h"
+
+#include <string>
+#include <vector>
 
 using namespace lld;
 
 class _NativeReaderErrorCategory : public llvm::_do_message {
 public:
   const char* name() const override {
     return "lld.native.reader";
   }
llvm::error_condition default_error_condition(int ev) const override {
     return llvm::errc::invalid_argument;
   }
 };
 
 const llvm::error_category &lld::ReaderErrorCategory() {
   static _ReaderErrorCategory i;
   return i;
 }
+
+
+
+
+namespace lld {
+
+class dynamic_error_category : public llvm::_do_message {
+public:
+  const char *name() const override { return "lld.dynamic_error"; }
+
+  std::string message(int ev) const override {
+    assert(ev >= 0);
+    assert(ev < (int)_messages.size());
+    // The value is an index into the string vector.
+    return _messages[ev];
+  }
+  
+  int add(std::string msg) {
+    llvm::sys::SmartScopedLock<true> lock(_mutex);
+    // Value zero is always the successs value.
+    if (_messages.empty())
+      _messages.push_back("Success");
+    _messages.push_back(msg);
+    // Return the index of the string just appended.
+    return _messages.size() - 1;
+  }
+  
+private:
+  std::vector<std::string> _messages;
+  llvm::sys::SmartMutex<true> _mutex;
+};
+
+static dynamic_error_category categorySingleton;
+
+
+llvm::error_code make_dynamic_error_code(StringRef msg) {
+  return llvm::error_code(categorySingleton.add(msg), categorySingleton);
+}
+
+llvm::error_code make_dynamic_error_code(const Twine &msg) {
+  return llvm::error_code(categorySingleton.add(msg.str()), categorySingleton);
+}
+
+}
+

lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp

 ///                    +-------+
 ///                    | Atoms |
 ///                    +-------+
 
 #include "MachONormalizedFile.h"
 #include "File.h"
 #include "Atoms.h"
 
+#include "lld/Core/Error.h"
 #include "lld/Core/LLVM.h"
 
 #include "llvm/Support/MachO.h"
 
 using namespace llvm::MachO;
 
 namespace lld {
 namespace mach_o {
for (size_t i = 0, e = section.content.size(); i != e; ++i) {
         file.addDefinedAtom(StringRef(), DefinedAtom::scopeLinkageUnit,
                             DefinedAtom::typeCString, strContent, copyRefs);
         offset = i + 1;
       }
     }
     break;
   case llvm::MachO::S_4BYTE_LITERALS:
     if ((section.content.size() % 4) != 0)
-      return llvm::make_error_code(llvm::errc::executable_format_error);
+      return make_dynamic_error_code(Twine("Section ") + section.segmentName
+                                     + "/" + section.sectionName 
+                                     + " has type S_4BYTE_LITERALS but its "
+                                     "size (" + Twine(section.content.size()) 
+                                     + ") is not a multiple of 4"); 
     for (size_t i = 0, e = section.content.size(); i != e; i += 4) {
       ArrayRef<uint8_t> byteContent = section.content.slice(offset, 4);
       file.addDefinedAtom(StringRef(), DefinedAtom::scopeLinkageUnit,
                           DefinedAtom::typeLiteral4, byteContent, copyRefs);
       offset += 4;
     }
     break;
   case llvm::MachO::S_8BYTE_LITERALS:
     if ((section.content.size() % 8) != 0)
-      return llvm::make_error_code(llvm::errc::executable_format_error);
+      return make_dynamic_error_code(Twine("Section ") + section.segmentName
+                                     + "/" + section.sectionName 
+                                     + " has type S_8YTE_LITERALS but its "
+                                     "size (" + Twine(section.content.size()) 
+                                     + ") is not a multiple of 8"); 
     for (size_t i = 0, e = section.content.size(); i != e; i += 8) {
       ArrayRef<uint8_t> byteContent = section.content.slice(offset, 8);
       file.addDefinedAtom(StringRef(), DefinedAtom::scopeLinkageUnit,
                           DefinedAtom::typeLiteral8, byteContent, copyRefs);
       offset += 8;
     }
     break;
   case llvm::MachO::S_16BYTE_LITERALS:
     if ((section.content.size() % 16) != 0)
-      return llvm::make_error_code(llvm::errc::executable_format_error);
+      return make_dynamic_error_code(Twine("Section ") + section.segmentName
+                                     + "/" + section.sectionName 
+                                     + " has type S_16BYTE_LITERALS but its "
+                                     "size (" + Twine(section.content.size()) 
+                                     + ") is not a multiple of 16"); 
     for (size_t i = 0, e = section.content.size(); i != e; i += 16) {
       ArrayRef<uint8_t> byteContent = section.content.slice(offset, 16);
       file.addDefinedAtom(StringRef(), DefinedAtom::scopeLinkageUnit,
                           DefinedAtom::typeLiteral16, byteContent, copyRefs);
       offset += 16;
     }
     break;
   default: