[PATCH] Convert CoreInputGraph.
Rui Ueyama
ruiu at google.com
Tue Jan 6 12:37:01 PST 2015
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/a05e72a8/attachment.html>
More information about the llvm-commits
mailing list