Allow context to report errors on files linked in the resolver

[pete]

Summary

Hi all

I'm trying to find a good place to give errors on files which differ in an unrecoverable way. For example, one file has been linked for the iOS simulator while another is normal x86 code, or perhaps linking arm and x86 code.

I've not yet added the checks for these errors, but wanted to see if you are all happy with the placement of the callback and the plumbing necessary to get the errors through the resolver.

Thanks,
Pete

[lhames]

This seems generically useful, but Is there a test case or use case?

include/lld/Core/Resolver.h

53 ↗

(On Diff #44572)

handleSharedLibrary doesn't add any undefined symbols, so this can remain 'void'.

lib/Core/Resolver.cpp

113 ↗

(On Diff #44572)

I think that it's reasonable to just assert that there was no error or undef added here.

126 ↗

(On Diff #44572)

Ditto here - we should just be able to assert no error here.

[pete]

In D16093#326240, @lhames wrote:

This seems generically useful, but Is there a test case or use case?

I should be able to add one very soon in a follow up commit. As I said in the comment earlier, I'm hoping to fill in the hook with things like arch, OS checks, but i need to add some code to actually pull that information from the file we are linking and have a copy in the context to compare against.

include/lld/Core/Resolver.h

53 ↗

(On Diff #44572)

Good point. I'll make it return std::error_code as its call to handleFile could still get an error from the new hook here.

lib/Core/Resolver.cpp

113 ↗

(On Diff #44572)

Yep, will do.

126 ↗

(On Diff #44572)

I should have explained the hook better. So what i hope to use it for in upcoming patches is to check that the file we link against has correct architecture, OS, ObjC flags, etc. So i think we could still throw an error here if the dylib is wrong in any of these respects.

[lhames]

Ahh - that makes sense. Thanks Pete - looks good to me.

lld/trunk/include/lld/Core/LinkingContext.h

public:
 
   /// Return the next ordinal and Increment it.
   virtual uint64_t getNextOrdinalAndIncrement() const { return _nextOrdinal++; }
 
   // This function is called just before the Resolver kicks in.
   // Derived classes may use it to change the list of input files.
   virtual void finalizeInputFiles() {}
 
+  /// Callback invoked for each file the Resolver decides we are going to load.
+  /// This can be used to update context state based on the file, and emit
+  /// errors for any differences between the context state and a loaded file.
+  /// For example, we can error if we try to load a file which is a different
+  /// arch from that being linked.
+  virtual std::error_code handleLoadedFile(File &file) {
+    return std::error_code();
+  }
+
   TaskGroup &getTaskGroup() { return _taskGroup; }
 
   /// @}
 protected:
   LinkingContext(); // Must be subclassed
 
   /// Abstract method to lazily instantiate the Writer.
   virtual Writer &writer() const = 0;

lld/trunk/include/lld/Core/Resolver.h

 
 #include "lld/Core/ArchiveLibraryFile.h"
 #include "lld/Core/File.h"
 #include "lld/Core/SharedLibraryFile.h"
 #include "lld/Core/Simple.h"
 #include "lld/Core/SymbolTable.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/Support/ErrorOr.h"
 #include <set>
 #include <unordered_map>
 #include <unordered_set>
 #include <vector>
 
 namespace lld {
 
 class Atom;
public:
   // InputFiles::Handler methods
   void doDefinedAtom(const DefinedAtom&);
   bool doUndefinedAtom(const UndefinedAtom &);
   void doSharedLibraryAtom(const SharedLibraryAtom &);
   void doAbsoluteAtom(const AbsoluteAtom &);
 
   // Handle files, this adds atoms from the current file thats
   // being processed by the resolver
-  bool handleFile(File &);
+  ErrorOr<bool> handleFile(File &);
 
   // Handle an archive library file.
-  bool handleArchiveFile(File &);
+  ErrorOr<bool> handleArchiveFile(File &);
 
   // Handle a shared library file.
-  void handleSharedLibrary(File &);
+  std::error_code handleSharedLibrary(File &);
 
   /// @brief do work of merging and resolving and return list
   bool resolve();
 
   std::unique_ptr<SimpleFile> resultFile() { return std::move(_result); }
 
 private:
-  typedef std::function<void(StringRef, bool)> UndefCallback;
+  typedef std::function<ErrorOr<bool>(StringRef, bool)> UndefCallback;
 
   bool undefinesAdded(int begin, int end);
   File *getFile(int &index);
 
   /// \brief Add section group/.gnu.linkonce if it does not exist previously.
   void maybeAddSectionGroupOrGnuLinkOnce(const DefinedAtom &atom);
 
   /// \brief The main function that iterates over the files to resolve
   void updatePreloadArchiveMap();
   bool resolveUndefines();
   void updateReferences();
   void deadStripOptimize();
   bool checkUndefines();
   void removeCoalescedAwayAtoms();
   void checkDylibSymbolCollisions();
-  void forEachUndefines(File &file, bool searchForOverrides, UndefCallback callback);
+  ErrorOr<bool> forEachUndefines(File &file, bool searchForOverrides,
+                                 UndefCallback callback);
 
   void markLive(const Atom *atom);
   void addAtoms(const std::vector<const DefinedAtom *>&);
   void maybePreloadArchiveMember(StringRef sym);
 
   class MergedFile : public SimpleFile {
   public:
     MergedFile() : SimpleFile("<linker-internal>") {}

lld/trunk/include/lld/ReaderWriter/MachOLinkingContext.h

public:
   static bool isBigEndian(Arch arch);
 
   /// Construct 32-bit value from string "X.Y.Z" where
   /// bits are xxxx.yy.zz.  Largest number is 65535.255.255
   static bool parsePackedVersion(StringRef str, uint32_t &result);
 
   void finalizeInputFiles() override;
 
+  std::error_code handleLoadedFile(File &file) override;
+
   bool customAtomOrderer(const DefinedAtom *left, const DefinedAtom *right,
                          bool &leftBeforeRight) const;
 
   /// Return the 'flat namespace' file. This is the file that supplies
   /// atoms for otherwise undefined symbols when the -flat_namespace or
   /// -undefined dynamic_lookup options are used.
   File* flatNamespaceFile() const { return _flatNamespaceFile; }
 

lld/trunk/lib/Core/Resolver.cpp

 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <utility>
 #include <vector>
 
 namespace lld {
 
-bool Resolver::handleFile(File &file) {
+ErrorOr<bool> Resolver::handleFile(File &file) {
+  if (auto ec = _ctx.handleLoadedFile(file))
+    return ec;
   bool undefAdded = false;
   for (const DefinedAtom *atom : file.defined())
     doDefinedAtom(*atom);
   for (const UndefinedAtom *atom : file.undefined()) {
     if (doUndefinedAtom(*atom)) {
       undefAdded = true;
       maybePreloadArchiveMember(atom->name());
     }
   }
   for (const SharedLibraryAtom *atom : file.sharedLibrary())
     doSharedLibraryAtom(*atom);
   for (const AbsoluteAtom *atom : file.absolute())
     doAbsoluteAtom(*atom);
   return undefAdded;
 }
 
-void Resolver::forEachUndefines(File &file, bool searchForOverrides,
+ErrorOr<bool> Resolver::forEachUndefines(File &file, bool searchForOverrides,
                                          UndefCallback callback) {
   size_t i = _undefineIndex[&file];
+  bool undefAdded = false;
   do {
     for (; i < _undefines.size(); ++i) {
       StringRef undefName = _undefines[i];
       if (undefName.empty())
         continue;
       const Atom *atom = _symbolTable.findByName(undefName);
       if (!isa<UndefinedAtom>(atom) || _symbolTable.isCoalescedAway(atom)) {
         // The symbol was resolved by some other file. Cache the result.
         _undefines[i] = "";
         continue;
       }
-      callback(undefName, false);
+      auto undefAddedOrError = callback(undefName, false);
+      if (undefAddedOrError.getError())
+        return undefAddedOrError;
+      undefAdded |= undefAddedOrError.get();
     }
     if (!searchForOverrides)
       continue;
     for (StringRef tentDefName : _symbolTable.tentativeDefinitions()) {
       // Load for previous tentative may also have loaded
       // something that overrode this tentative, so always check.
       const Atom *curAtom = _symbolTable.findByName(tentDefName);
       assert(curAtom != nullptr);
-      if (const DefinedAtom *curDefAtom = dyn_cast<DefinedAtom>(curAtom))
-        if (curDefAtom->merge() == DefinedAtom::mergeAsTentative)
-          callback(tentDefName, true);
+      if (const DefinedAtom *curDefAtom = dyn_cast<DefinedAtom>(curAtom)) {
+        if (curDefAtom->merge() == DefinedAtom::mergeAsTentative) {
+          auto undefAddedOrError = callback(tentDefName, true);
+          if (undefAddedOrError.getError())
+            return undefAddedOrError;
+          undefAdded |= undefAddedOrError.get();
+        }
+      }
     }
   } while (i < _undefines.size());
   _undefineIndex[&file] = i;
+  return undefAdded;
 }
 
-bool Resolver::handleArchiveFile(File &file) {
+ErrorOr<bool> Resolver::handleArchiveFile(File &file) {
   ArchiveLibraryFile *archiveFile = cast<ArchiveLibraryFile>(&file);
   bool searchForOverrides =
       _ctx.searchArchivesToOverrideTentativeDefinitions();
-  bool undefAdded = false;
-  forEachUndefines(file, searchForOverrides,
-                   [&](StringRef undefName, bool dataSymbolOnly) {
+  return forEachUndefines(file, searchForOverrides,
+                          [&](StringRef undefName,
+                              bool dataSymbolOnly)->ErrorOr<bool> {
     if (File *member = archiveFile->find(undefName, dataSymbolOnly)) {
       member->setOrdinal(_ctx.getNextOrdinalAndIncrement());
       member->beforeLink();
       updatePreloadArchiveMap();
-      undefAdded = handleFile(*member) || undefAdded;
+      return handleFile(*member);
     }
+    return false;
   });
-  return undefAdded;
 }
 
-void Resolver::handleSharedLibrary(File &file) {
+std::error_code Resolver::handleSharedLibrary(File &file) {
   // Add all the atoms from the shared library
   SharedLibraryFile *sharedLibrary = cast<SharedLibraryFile>(&file);
-  handleFile(*sharedLibrary);
+  auto undefAddedOrError = handleFile(*sharedLibrary);
+  if (undefAddedOrError.getError())
+    return undefAddedOrError.getError();
   bool searchForOverrides =
       _ctx.searchSharedLibrariesToOverrideTentativeDefinitions();
-  forEachUndefines(file, searchForOverrides,
-                   [&](StringRef undefName, bool dataSymbolOnly) {
+  undefAddedOrError = forEachUndefines(file, searchForOverrides,
+                                       [&](StringRef undefName,
+                                           bool dataSymbolOnly)->ErrorOr<bool> {
     if (const SharedLibraryAtom *atom =
             sharedLibrary->exports(undefName, dataSymbolOnly))
       doSharedLibraryAtom(*atom);
+    return false;
   });
+
+  if (undefAddedOrError.getError())
+    return undefAddedOrError.getError();
+  return std::error_code();
 }
 
 bool Resolver::doUndefinedAtom(const UndefinedAtom &atom) {
   DEBUG_WITH_TYPE("resolver", llvm::dbgs()
                     << "       UndefinedAtom: "
                     << llvm::format("0x%09lX", &atom)
                     << ", name=" << atom.name() << "\n");
 
if (std::error_code ec = file->parse()) {
                    << ": " << ec.message() << "\n";
       return false;
     }
     DEBUG_WITH_TYPE("resolver",
                     llvm::dbgs() << "Loaded file: " << file->path() << "\n");
     file->beforeLink();
     updatePreloadArchiveMap();
     switch (file->kind()) {
-    case File::kindObject:
+    case File::kindObject: {
       // The same file may be visited more than once if the file is
       // in --start-group and --end-group. Only library files should
       // be processed more than once.
       if (seen.count(file))
         break;
       seen.insert(file);
       assert(!file->hasOrdinal());
       file->setOrdinal(_ctx.getNextOrdinalAndIncrement());
-      undefAdded = handleFile(*file);
+      auto undefAddedOrError = handleFile(*file);
+      if (undefAddedOrError.getError()) {
+        llvm::errs() << "Error in " + file->path()
+                     << ": " << undefAddedOrError.getError().message() << "\n";
+        return false;
+      }
+      undefAdded = undefAddedOrError.get();
       break;
-    case File::kindArchiveLibrary:
+    }
+    case File::kindArchiveLibrary: {
       if (!file->hasOrdinal())
         file->setOrdinal(_ctx.getNextOrdinalAndIncrement());
-      undefAdded = handleArchiveFile(*file);
+      auto undefAddedOrError = handleArchiveFile(*file);
+      if (undefAddedOrError.getError()) {
+        llvm::errs() << "Error in " + file->path()
+                     << ": " << undefAddedOrError.getError().message() << "\n";
+        return false;
+      }
+      undefAdded = undefAddedOrError.get();
       break;
+    }
     case File::kindSharedLibrary:
       if (!file->hasOrdinal())
         file->setOrdinal(_ctx.getNextOrdinalAndIncrement());
-      handleSharedLibrary(*file);
+      if (auto EC = handleSharedLibrary(*file)) {
+        llvm::errs() << "Error in " + file->path()
+                     << ": " << EC.message() << "\n";
+        return false;
+      }
       break;
     }
     _newUndefinesAdded[file] = undefAdded;
   }
 }
 
 // switch all references to undefined or coalesced away atoms
 // to the new defined atom

lld/trunk/lib/ReaderWriter/MachO/MachOLinkingContext.cpp

std::stable_sort(elements.begin(), elements.end(),
                    [](const std::unique_ptr<Node> &a,
                       const std::unique_ptr<Node> &b) {
                      return !isLibrary(a) && isLibrary(b);
                    });
   size_t numLibs = std::count_if(elements.begin(), elements.end(), isLibrary);
   elements.push_back(llvm::make_unique<GroupEnd>(numLibs));
 }
 
+std::error_code MachOLinkingContext::handleLoadedFile(File &file) {
+  return std::error_code();
+}
+
 } // end namespace lld