[PATCH] Convert CoreInputGraph.

[Shankar Easwaran]

Hi Rui,

Thanks for waiting on this. Please wait for feedback from Nick/Bigcheese 
on this.

I am not in favor of this design as there are parts of the linker which 
need to know about positional command line flags.

The other problem that I have is the driver is trying to parse archive 
files, which sounds not a clean design to me.

Shankar Easwaran

On 12/15/2014 8:33 AM, Rui Ueyama wrote:
> 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/
>>>
>>>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150106/24560535/attachment.html>