[PATCH] Convert CoreInputGraph.

[Rui Ueyama]

It depends on what option we are handling. For example, -Bdynamic and
-Bstatic naturally fit to be handled in the driver, because now the driver
knows the types of input files, it can easily handle these options. No need
to propagate the option information outside the driver (which is nice). For
--as-needed, we should probably make the driver save the library file names
for --as-needed to the linking context, so that the writer can write the
appropriate DT_NEEDED tags later. (I should note that --as-needed is not
supported yet by LLD.)

Command line option handling is not a difficult part of implementing a
linker. If we need to propagate information from driver to backend, we can
do that in some way. That's case by case. Please don't over-think and
over-design one thing to cover all hypothetical use cases. That has really
made LLD development hard. That's a thing we all have learned from the
recent development of LLD.

On Mon, Dec 15, 2014 at 10:58 PM, Shankar Easwaram <shankarke at gmail.com>
wrote:
>
>
>
>
>
> > 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/
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141215/ded00b1b/attachment.html>