[PATCH] Convert CoreInputGraph.

[Shankar Easwaram]

> On Dec 15, 2014, at 00:16, Rui Ueyama <ruiu at google.com> wrote:
> 
> ================
> Comment at: include/lld/Core/File.h:270-299
> @@ -268,3 +269,32 @@
> +
> +/// 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
> ----------------
> shankarke wrote:
>> 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.
> 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.
> 
> ================
> Comment at: include/lld/Driver/WrapperInputGraph.h:22-27
> @@ +21,8 @@
> +
> +class WrapperNode : public FileNode {
> +public:
> +  WrapperNode(std::unique_ptr<File> file) : FileNode(file->path()) {
> +    _files.push_back(std::move(file));
> +  }
> +};
> +
> ----------------
> shankarke wrote:
>> We would need to move this to CoreInputGraph.
> No it's not. As I wrote in the description, this is going to be used by all drivers.
> 
> ================
> Comment at: lib/Driver/Driver.cpp:10-13
> @@ -9,4 +9,6 @@
> 
> #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"
> ----------------
> shankarke wrote:
>> sort includes.
> Will do.
> 
> ================
> Comment at: lib/Driver/Driver.cpp:39-50
> @@ +38,14 @@
> +
> +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;
> +}
> +
> ----------------
> shankarke wrote:
>> The driver should not know about ArchiveLibraryFiles. This increases the circular dependency when building lld as a shared library.
> 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.
> 
> ================
> Comment at: lib/Driver/Driver.cpp:60-61
> @@ +59,4 @@
> +    return makeErrorFile(path, ec);
> +  if (wholeArchive)
> +    return parseMemberFiles(files);
> +  return files;
> ----------------
> shankarke wrote:
>> FileNode::parse would need to check if wholeArchive is set and parse all members, why is this being done in the Driver ?
> 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.
> 

Where are you planning to store positional command line arguments, example -Bdynamic, -Bstatic, whole archive, {start,end} group, as-needed etc. Again the elf writer needs to know all of this information which is possible with an filenode or input element model IMO. Are you going to be storing the positional arguments elsewhere ?

If it's a flat hierarchy I would think storing it as std::vector<fileortag> files.

Let us know your plan.



> http://reviews.llvm.org/D6653
> 
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/
> 
>