[PATCH] Convert CoreInputGraph.

[Rui Ueyama]

Ping?
2014/12/15 23:33 "Rui Ueyama" <ruiu at google.com>:

> 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/20141218/3f4615bd/attachment.html>