<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Rui,<br>
<br>
Thanks for waiting on this. Please wait for feedback from
Nick/Bigcheese on this.<br>
<br>
I am not in favor of this design as there are parts of the linker
which need to know about positional command line flags.<br>
<br>
The other problem that I have is the driver is trying to parse
archive files, which sounds not a clean design to me.<br>
<br>
Shankar Easwaran<br>
<br>
On 12/15/2014 8:33 AM, Rui Ueyama wrote:<br>
</div>
<blockquote
cite="mid:CAJENXguYQtCv4L6uAOUMz-JN_4fnuGNEcD3RYY=_D3uFnwDoHQ@mail.gmail.com"
type="cite">
<pre wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:shankarke@gmail.com"><shankarke@gmail.com></a>
wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">On Dec 15, 2014, at 00:16, Rui Ueyama <a class="moz-txt-link-rfc2396E" href="mailto:ruiu@google.com"><ruiu@google.com></a> 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),
</pre>
</blockquote>
<pre wrap="">_ec(ec) {}
</pre>
<blockquote type="cite">
<pre wrap="">+
+ 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
</pre>
</blockquote>
<pre wrap="">override {
</pre>
<blockquote type="cite">
<pre wrap="">+ 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:
</pre>
<blockquote type="cite">
<pre wrap="">parseFile could still return an error code. If we want to just use this
</pre>
</blockquote>
</blockquote>
<pre wrap="">class for error reporting purposes, this classes could go in the unittests
for InputGraph.
</pre>
<blockquote type="cite">
<pre wrap="">It doesn't sound like a good idea. What do we get from making parseFile
</pre>
</blockquote>
<pre wrap="">return an error? The behavior remains the same in practice, but it needs
more code and more interface to customize the behavior.
</pre>
<blockquote type="cite">
<pre wrap="">
================
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:
</pre>
<blockquote type="cite">
<pre wrap="">We would need to move this to CoreInputGraph.
</pre>
</blockquote>
<pre wrap="">No it's not. As I wrote in the description, this is going to be used by
</pre>
</blockquote>
<pre wrap="">all drivers.
</pre>
<blockquote type="cite">
<pre wrap="">
================
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:
</pre>
<blockquote type="cite">
<pre wrap="">sort includes.
</pre>
</blockquote>
<pre wrap="">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:
</pre>
<blockquote type="cite">
<pre wrap="">The driver should not know about ArchiveLibraryFiles. This increases
</pre>
</blockquote>
</blockquote>
<pre wrap="">the circular dependency when building lld as a shared library.
</pre>
<blockquote type="cite">
<pre wrap="">That's not a problem. ArchiveLibraryFile is in include/lld/Core.
</pre>
</blockquote>
<pre wrap="">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.
</pre>
<blockquote type="cite">
<pre wrap="">
================
Comment at: lib/Driver/Driver.cpp:60-61
@@ +59,4 @@
+ return makeErrorFile(path, ec);
+ if (wholeArchive)
+ return parseMemberFiles(files);
+ return files;
----------------
shankarke wrote:
</pre>
<blockquote type="cite">
<pre wrap="">FileNode::parse would need to check if wholeArchive is set and parse
</pre>
</blockquote>
</blockquote>
<pre wrap="">all members, why is this being done in the Driver ?
</pre>
<blockquote type="cite">
<pre wrap="">As I wrote in the description, we are going to represent a input file
</pre>
</blockquote>
<pre wrap="">list as a File list. FileNode's features are being deprecated.
</pre>
<blockquote type="cite">
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
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.
</pre>
<blockquote type="cite">
<pre wrap=""><a class="moz-txt-link-freetext" href="http://reviews.llvm.org/D6653">http://reviews.llvm.org/D6653</a>
EMAIL PREFERENCES
<a class="moz-txt-link-freetext" href="http://reviews.llvm.org/settings/panel/emailpreferences/">http://reviews.llvm.org/settings/panel/emailpreferences/</a>
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation</pre>
</body>
</html>