[PATCH] Convert CoreInputGraph.
Rui Ueyama
ruiu at google.com
Tue Jan 6 12:52:37 PST 2015
Michael has actually signed it off on Phab, so I'm going to submit this.
On Wed, Jan 7, 2015 at 5:37 AM, Rui Ueyama <ruiu at google.com> wrote:
> Yes, I don't think it has signed off yet, that's why I sent pings on this
> thread.
>
> On Wed, Jan 7, 2015 at 5:32 AM, Shankar Easwaran <shankare at codeaurora.org>
> wrote:
>
>> 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> <shankarke at gmail.com>
>> wrote:
>>
>> On Dec 15, 2014, at 00:16, Rui Ueyama <ruiu at google.com> <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 listllvm-commits at cs.uiuc.eduhttp://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/20150107/d44eea88/attachment.html>
More information about the llvm-commits
mailing list