Convert CoreInputGraph.

[ruiu]

Summary

This is a part of InputGraph cleanup to represent input files as a flat list of Files (and some meta-nodes for group etc.)

We cannot achieve that goal in one gigantic patch (well, I could, but no one wants to review such patch), so I split the task into small steps as shown below.

(But before that, recap the progress I've made so far: Currently InputGraph contains a list of InputElements. Each InputElement contain one File (that used to have multiple Files, but I eliminated that use case in r223867). Files are currently instantiated in Driver::link(), but I already made a change to separate file parsing from object instantiation (r224102), so we can safely instantiate Files when we need them, instead of instantiating them through the wrapper class (FileNode subclasses). InputGraph used to have a logic to act like a generator by interpreting groups itself, but it's now just a container of a list of InputElements (r223867). The logic has moved to the resolver.)

1. Instantiate Files in the driver and wrap them with WrapperNode. WrapperNode is a temporary class that allows us to instantiate Files in the driver while keep using the current InputGraph data structure.

This patch demonstrates how this step 1 looks like, using Core driver as an example.

2. Do the same thing for the other drivers.

When step 2 is done, an InputGraph consists of GroupEnd objects or WrapperNodes each of which contains one File. Other types of FileNode subclasses are gone.

3. Replace InputGraph with a list of InputElements. InputGraph is already just a container of InputElement list, so this step retires the wrapper.

4. Replace WrapperNode with File to remove the temporary scaffolding.

We need some code cleanup between each step, because many classes do a bit odd things (e.g. InputGraph::getGroupSize()). I'll straight things up as I need to.

After step 4, we can clean things up by removing code that was written to work-around InputGraph limitation (e.g. LinkingContext::maybeSortInputFile). But that's a future plan atm.

So, that's a plan. Please review!

[shankarke]

include/lld/Core/File.h

270–299

parseFile could still return an error code. If we want to just use this class for error reporting purposes, this classes could go in the unittests for InputGraph.

include/lld/Driver/WrapperInputGraph.h

22–27

We would need to move this to CoreInputGraph.

lib/Driver/CoreDriver.cpp

154–159

couldnt the InputElement determine that a file has not been created and automatically create a File at parsing time ?

lib/Driver/Driver.cpp

10–13

sort includes.

39–50

The driver should not know about ArchiveLibraryFiles. This increases the circular dependency when building lld as a shared library.

60–61

FileNode::parse would need to check if wholeArchive is set and parse all members, why is this being done in the Driver ?

[ruiu]

include/lld/Core/File.h

270–299

It doesn't sound like a good idea. What do we get from making parseFile return an error? The behavior remains the same in practice, but it needs more code and more interface to customize the behavior.

include/lld/Driver/WrapperInputGraph.h

22–27

No it's not. As I wrote in the description, this is going to be used by all drivers.

lib/Driver/Driver.cpp

10–13

Will do.

39–50

That's not a problem. ArchiveLibraryFile is in include/lld/Core. include/lld/Driver can depends on Core. No one in Core should depends on Driver. If there's a circular dependency between Driver and Core, we need to eliminate the dependency from Core to Driver, not the opposite one.

60–61

As I wrote in the description, we are going to represent a input file list as a File list. FileNode's features are being deprecated.

[atanasyan]

LGTM But please wait one more +1 at least

[ruiu]

Ping. (I'm sure this is not a good timing, but this is blocking many
follow-up patches...)

[ruiu]

Ping?

[Bigcheese]

lgtm

[kledzik]

LGTM

Two comments:

1. This refactoring has taken so many steps, I'm beginning to think one big patch where we can see the end design might have been better...
2. The ErrorFile concept makes me wonder if that should be the approach for all parseFile() implementations. Rather than having a result parameter is that is a vector they append to and returning an error_code, they simply return a vector of Files and any errors are encoded as ErrorFiles in that vector.

[ruiu]

Thank you for reviewing!

[Eugene.Zelenko]

Committed in r225313.

include/lld/Core/File.h

 #define LLD_CORE_FILE_H
 
 #include "lld/Core/AbsoluteAtom.h"
 #include "lld/Core/DefinedAtom.h"
 #include "lld/Core/SharedLibraryAtom.h"
 #include "lld/Core/UndefinedAtom.h"
 #include "lld/Core/range.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <functional>
 #include <memory>
 #include <vector>
 
 namespace lld {
 
 class LinkingContext;
public:
 
   virtual void
   removeDefinedAtomsIf(std::function<bool(const DefinedAtom *)> pred) = 0;
 
 protected:
   /// \brief only subclasses of MutableFile can be instantiated
   MutableFile(StringRef p) : File(p, kindObject) {}
 };
+
+/// An ErrorFile represents a file that doesn't exist.
+/// If you try to parse a file which doesn't exist, an instance of this
+/// class will be returned. That's parse method always returns an error.
+/// This is useful to delay erroring on non-existent files, so that we
+/// can do unit testing a driver using non-existing file paths.
+class ErrorFile : public File {
+public:
+  ErrorFile(StringRef p, std::error_code ec) : File(p, kindObject), _ec(ec) {}
+
+  std::error_code doParse() override { return _ec; }
+
+  const atom_collection<DefinedAtom> &defined() const override {
+    llvm_unreachable("internal error");
+  }
+  const atom_collection<UndefinedAtom> &undefined() const override {
+    llvm_unreachable("internal error");
+  }
+  const atom_collection<SharedLibraryAtom> &sharedLibrary() const override {
+    llvm_unreachable("internal error");
+  }
+  const atom_collection<AbsoluteAtom> &absolute() const override {
+    llvm_unreachable("internal error");
+  }
+
+private:
+  std::error_code _ec;
+};
+
 } // end namespace lld
 
 #endif

include/lld/Core/InputGraph.h

public:
   /// \brief Return the next File thats part of this node to the
   /// resolver.
   File *getNextFile() override {
     if (_nextFileIndex == _files.size())
       return nullptr;
     return _files[_nextFileIndex++].get();
   }
 
+  std::error_code parse(const LinkingContext &, raw_ostream &) override;
+
 protected:
   StringRef _path;                       // The path of the Input file
   InputGraph::FileVectorT _files;        // A vector of lld File objects
 
   // The next file that would be processed by the resolver
   uint32_t _nextFileIndex;
 };
 
 /// \brief Represents Internal Input files
 class SimpleFileNode : public FileNode {
 public:
   SimpleFileNode(StringRef path) : FileNode(path) {}
   SimpleFileNode(StringRef path, std::unique_ptr<File> f)
       : FileNode(path) {
     _files.push_back(std::move(f));
   }
 
   virtual ~SimpleFileNode() {}
 
   /// \brief add a file to the list of files
   virtual void appendInputFile(std::unique_ptr<File> f) {
     _files.push_back(std::move(f));
   }
-
-  /// \brief parse the input element
-  std::error_code parse(const LinkingContext &, raw_ostream &) override {
-    return std::error_code();
-  }
 };
+
 } // namespace lld
 
 #endif // LLD_CORE_INPUT_GRAPH_H

include/lld/Driver/CoreInputGraph.h

-//===- lld/Driver/CoreInputGraph.h - Input Graph Node for Core linker -----===//
-//
-//                             The LLVM Linker
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-///
-/// \file
-///
-/// Handles Options for CORE linking and provides InputElements
-/// for the CORE linker
-///
-//===----------------------------------------------------------------------===//
-
-#ifndef LLD_DRIVER_CORE_INPUT_GRAPH_H
-#define LLD_DRIVER_CORE_INPUT_GRAPH_H
-
-#include "lld/Core/InputGraph.h"
-#include "lld/ReaderWriter/CoreLinkingContext.h"
-#include "lld/ReaderWriter/Reader.h"
-#include "llvm/Support/Errc.h"
-#include <map>
-#include <memory>
-
-namespace lld {
-
-/// \brief Represents a Core File
-class CoreFileNode : public FileNode {
-public:
-  CoreFileNode(CoreLinkingContext &, StringRef path) : FileNode(path) {}
-
-  /// \brief Parse the input file to lld::File.
-  std::error_code parse(const LinkingContext &ctx,
-                        raw_ostream &diagnostics) override {
-    ErrorOr<StringRef> filePath = getPath(ctx);
-    if (filePath.getError() == llvm::errc::no_such_file_or_directory)
-      return make_error_code(llvm::errc::no_such_file_or_directory);
-
-    // Create a memory buffer
-    ErrorOr<std::unique_ptr<MemoryBuffer>> mb =
-        MemoryBuffer::getFileOrSTDIN(*filePath);
-    if (std::error_code ec = mb.getError())
-      return ec;
-
-    return ctx.registry().parseFile(std::move(mb.get()), _files);
-  }
-};
-
-} // namespace lld
-
-#endif

include/lld/Driver/Driver.h

 
 namespace lld {
 class LinkingContext;
 class CoreLinkingContext;
 class MachOLinkingContext;
 class PECOFFLinkingContext;
 class ELFLinkingContext;
 
+typedef std::vector<std::unique_ptr<File>> FileVector;
+
+FileVector makeErrorFile(StringRef path, std::error_code ec);
+FileVector parseMemberFiles(FileVector &files);
+FileVector parseFile(LinkingContext &ctx, StringRef path, bool wholeArchive);
+
 /// Base class for all Drivers.
 class Driver {
 protected:
 
   /// Performs link using specified options
   static bool link(LinkingContext &context,
                    raw_ostream &diagnostics = llvm::errs());
 

include/lld/Driver/WrapperInputGraph.h

+//===- lld/Driver/WrapperInputGraph.h - dummy InputGraph node -------------===//
+//
+//                             The LLVM Linker
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLD_DRIVER_WRAPPER_INPUT_GRAPH_H
+#define LLD_DRIVER_WRAPPER_INPUT_GRAPH_H
+
+#include "lld/Core/InputGraph.h"
+#include "lld/ReaderWriter/CoreLinkingContext.h"
+#include "lld/ReaderWriter/Reader.h"
+#include "llvm/Support/Errc.h"
+#include <map>
+#include <memory>
+
+namespace lld {
+
+class WrapperNode : public FileNode {
+public:
+  WrapperNode(std::unique_ptr<File> file) : FileNode(file->path()) {
+    _files.push_back(std::move(file));
+  }
+};
+
+}
+
+#endif

lib/Core/InputGraph.cpp

 
 bool FileNode::getReplacements(InputGraph::InputElementVectorT &result) {
   if (_files.size() < 2)
     return false;
   for (std::unique_ptr<File> &file : _files)
     result.push_back(llvm::make_unique<SimpleFileNode>(_path, std::move(file)));
   return true;
 }
+
+std::error_code FileNode::parse(const LinkingContext &, raw_ostream &) {
+  for (std::unique_ptr<File> &file : _files)
+    if (std::error_code ec = file->parse())
+      return ec;
+  return std::error_code();
+}

lib/Driver/CoreDriver.cpp

 //===- lib/Driver/CoreDriver.cpp ------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lld/Driver/Driver.h"
-#include "lld/Driver/CoreInputGraph.h"
+#include "lld/Driver/WrapperInputGraph.h"
 #include "lld/ReaderWriter/CoreLinkingContext.h"
 #include "lld/ReaderWriter/Reader.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
static const Registry::KindStrings coreKindStrings[] = {
   { CoreLinkingContext::TEST_RELOC_GOT_LOAD32,    "gotLoad32" },
   { CoreLinkingContext::TEST_RELOC_GOT_USE32,     "gotUse32" },
   { CoreLinkingContext::TEST_RELOC_LEA32_WAS_GOT, "lea32wasGot" },
   LLD_KIND_STRING_END
 };
 
 bool CoreDriver::link(int argc, const char *argv[], raw_ostream &diagnostics) {
   CoreLinkingContext ctx;
-  if (!parse(argc, argv, ctx))
-    return false;
 
   // Register possible input file parsers.
   ctx.registry().addSupportNativeObjects();
   ctx.registry().addSupportYamlFiles();
-
   ctx.registry().addKindTable(Reference::KindNamespace::testing,
                               Reference::KindArch::all, coreKindStrings);
 
+  if (!parse(argc, argv, ctx))
+    return false;
   return Driver::link(ctx);
 }
 
 bool CoreDriver::parse(int argc, const char *argv[], CoreLinkingContext &ctx,
                        raw_ostream &diagnostics) {
   // Parse command line options using CoreOptions.td
   std::unique_ptr<llvm::opt::InputArgList> parsedArgs;
   CoreOptTable table;
for (auto inputArg : *parsedArgs) {
     case OPT_commons_search_archives:
       ctx.setSearchArchivesToOverrideTentativeDefinitions(true);
       break;
 
     case OPT_add_pass:
       ctx.addPassNamed(inputArg->getValue());
       break;
 
-    case OPT_INPUT:
+    case OPT_INPUT: {
+      std::vector<std::unique_ptr<File>> files
+        = parseFile(ctx, inputArg->getValue(), false);
+      for (std::unique_ptr<File> &file : files) {
         inputGraph->addInputElement(std::unique_ptr<InputElement>(
-          new CoreFileNode(ctx, inputArg->getValue())));
+            new WrapperNode(std::move(file))));
+      }
       break;
+    }
 
     default:
       break;
     }
   }
 
   if (!inputGraph->size()) {
     diagnostics << "No input files\n";

lib/Driver/Driver.cpp

 //===- lib/Driver/Driver.cpp - Linker Driver Emulator ---------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lld/Driver/Driver.h"
+#include "lld/Core/ArchiveLibraryFile.h"
+#include "lld/Core/File.h"
 #include "lld/Core/Instrumentation.h"
 #include "lld/Core/LLVM.h"
 #include "lld/Core/Parallel.h"
 #include "lld/Core/PassManager.h"
 #include "lld/Core/Resolver.h"
 #include "lld/Passes/RoundTripNativePass.h"
 #include "lld/Passes/RoundTripYAMLPass.h"
 #include "lld/ReaderWriter/Reader.h"
 #include "lld/ReaderWriter/Writer.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <mutex>
 
 namespace lld {
 
+FileVector makeErrorFile(StringRef path, std::error_code ec) {
+  std::vector<std::unique_ptr<File>> result;
+  result.push_back(llvm::make_unique<ErrorFile>(path, ec));
+  return result;
+}
+
+FileVector parseMemberFiles(FileVector &files) {
+  std::vector<std::unique_ptr<File>> members;
+  for (std::unique_ptr<File> &file : files) {
+    if (auto *archive = dyn_cast<ArchiveLibraryFile>(file.get())) {
+      if (std::error_code ec = archive->parseAllMembers(members))
+        return makeErrorFile(file->path(), ec);
+    } else {
+      members.push_back(std::move(file));
+    }
+  }
+  return members;
+}
+
+FileVector parseFile(LinkingContext &ctx, StringRef path, bool wholeArchive) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> mb
+      = MemoryBuffer::getFileOrSTDIN(path);
+  if (std::error_code ec = mb.getError())
+    return makeErrorFile(path, ec);
+  std::vector<std::unique_ptr<File>> files;
+  if (std::error_code ec = ctx.registry().parseFile(std::move(mb.get()), files, false))
+    return makeErrorFile(path, ec);
+  if (wholeArchive)
+    return parseMemberFiles(files);
+  return files;
+}
+
 /// This is where the link is actually performed.
 bool Driver::link(LinkingContext &context, raw_ostream &diagnostics) {
   // Honor -mllvm
   if (!context.llvmOptions().empty()) {
     unsigned numArgs = context.llvmOptions().size();
     const char **args = new const char *[numArgs + 2];
     args[0] = "lld (LLVM option parsing)";
     for (unsigned i = 0; i != numArgs; ++i)

test/core/empty.objtxt

-# RUN: lld -core %s | FileCheck %s
-
-#
-# Test that an empty file is handled properly
-#
-
----
-...
-
-# CHECK:       ---
-# CHECK:       ...