<div dir="ltr">Yes, I don't think it has signed off yet, that's why I sent pings on this thread.<div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 7, 2015 at 5:32 AM, Shankar Easwaran <span dir="ltr"><<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>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<div><div><br>
<br>
On 12/15/2014 8:33 AM, Rui Ueyama wrote:<br>
</div></div></div>
<blockquote type="cite"><div><div>
<pre>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 href="mailto:shankarke@gmail.com" target="_blank"><shankarke@gmail.com></a>
wrote:
</pre>
<blockquote type="cite">
<pre>
</pre>
<blockquote type="cite">
<pre>On Dec 15, 2014, at 00:16, Rui Ueyama <a href="mailto:ruiu@google.com" target="_blank"><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>_ec(ec) {}
</pre>
<blockquote type="cite">
<pre>+
+ 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>override {
</pre>
<blockquote type="cite">
<pre>+ 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>parseFile could still return an error code. If we want to just use this
</pre>
</blockquote>
</blockquote>
<pre>class for error reporting purposes, this classes could go in the unittests
for InputGraph.
</pre>
<blockquote type="cite">
<pre>It doesn't sound like a good idea. What do we get from making parseFile
</pre>
</blockquote>
<pre>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>================
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>We would need to move this to CoreInputGraph.
</pre>
</blockquote>
<pre>No it's not. As I wrote in the description, this is going to be used by
</pre>
</blockquote>
<pre>all drivers.
</pre>
<blockquote type="cite">
<pre>================
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>sort includes.
</pre>
</blockquote>
<pre>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>The driver should not know about ArchiveLibraryFiles. This increases
</pre>
</blockquote>
</blockquote>
<pre>the circular dependency when building lld as a shared library.
</pre>
<blockquote type="cite">
<pre>That's not a problem. ArchiveLibraryFile is in include/lld/Core.
</pre>
</blockquote>
<pre>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>================
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>FileNode::parse would need to check if wholeArchive is set and parse
</pre>
</blockquote>
</blockquote>
<pre>all members, why is this being done in the Driver ?
</pre>
<blockquote type="cite">
<pre>As I wrote in the description, we are going to represent a input file
</pre>
</blockquote>
<pre>list as a File list. FileNode's features are being deprecated.
</pre>
<blockquote type="cite">
<pre></pre>
</blockquote>
<pre>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><a href="http://reviews.llvm.org/D6653" target="_blank">http://reviews.llvm.org/D6653</a>
EMAIL PREFERENCES
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a>
</pre>
</blockquote>
<pre></pre>
</blockquote>
<pre></pre>
<br>
<fieldset></fieldset>
<br>
</div></div><pre>_______________________________________________
llvm-commits mailing list
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><span><font color="#888888">
</font></span></pre><span><font color="#888888">
</font></span></blockquote><span><font color="#888888">
<br>
<br>
<pre cols="72">--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation</pre>
</font></span></div>
</blockquote></div><br></div></div>