<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Nick,<br>
<br>
Thanks for the review.<br>
<br>
I had a thought on the same approach too, the problem was ReaderELF
had to be passed ReaderArchive too (or) at a bare minimum the
ReaderArchiveOptions. <br>
This would have broken things with other Readers. I discussed with
Michael on IRC and he said putting it in File should be fine for
now.<br>
<br>
The approach you mention would be very clean, if there is nice way
to pass in the ReaderArchive and in future, support for reading
shared libraries.<br>
<br>
I have replied to all the other comments below.<br>
<br>
<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div>
<blockquote type="cite">
<div> </div>
<div>+ ReaderOptionsArchive readerOptionsArchive(reader,
argc, argv);</div>
<div>+ ReaderArchive *readerArchive = new
ReaderArchive(readerOptionsArchive);</div>
<div>+</div>
<div>+</div>
<div> for (auto path : cmdLineInputFilePaths) {</div>
<div> std::vector<std::unique_ptr<File>>
files;</div>
<div>- if ( error(reader->readFile(path, files)) )</div>
<div>- return 1;</div>
<div>+ File::Kind kind;</div>
<div>+ error_code EC;</div>
</blockquote>
</div>
</blockquote>
Removed changes and removed File::FileType, and moved changes to
Reader.<br>
<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div>
<blockquote type="cite">
<div>+ </div>
<div>+ if (readerSelected == readerYAML) {</div>
<div>+ kind = File::kindYAML;</div>
<div>+ }</div>
<div>+ else </div>
<div>+ if ((EC = (File::FileType(path, kind)))) {</div>
<div>+ <span class="Apple-tab-span" style="white-space:pre">
</span>return 1;</div>
<div>+ }</div>
<div>+ switch(kind)</div>
<div>+ {</div>
<div>+ case File::kindObject:</div>
<div>+ case File::kindYAML:</div>
<div>+ if ( error(reader->readFile(path, files)))</div>
<div>+ <span class="Apple-tab-span"
style="white-space:pre"> </span>return 1;</div>
<div>+ break;</div>
<div>+ </div>
<div>+ case File::kindArchiveLibrary:</div>
<div>+ if (error(readerArchive->parseFile(path,
files)))</div>
<div>+ <span class="Apple-tab-span"
style="white-space:pre"> </span>return 1;</div>
<div>+ break;</div>
<div>+ </div>
<div>+ case File::kindSharedLibrary:</div>
<div>+ if (error(readerArchive->parseFile(path,
files)))</div>
<div>+ <span class="Apple-tab-span"
style="white-space:pre"> </span>return 1;</div>
<div>+ break;</div>
</blockquote>
<div>Why are shared libraries going to the archive reader?</div>
<div><br>
</div>
</div>
</blockquote>
This was an error, and removed it.<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div><br>
<blockquote type="cite">
<div>+ </div>
<div>+ default:</div>
<div>+ return 1;</div>
<div>+ }</div>
<div> inputFiles.appendFiles(files);</div>
<div> }</div>
</blockquote>
<div>We need a clean way to invoke the correct Reader for each
format. This work above would have to be replicated by other
code wanting to do linking functionality. I know Michael and
I have discussed different ways to organize this. I think it
would be cleaner to leave lld-core's main() as-is and instead
in each Reader (e.g. ReaderELF, after it has created the
MemoryBuffer, have it call llvm::sys::IdentifyFileType() and
if the result is an archive, have it (ReaderELF) hand off the
file to ReaderArchive. </div>
<div><br>
</div>
</div>
</blockquote>
I had a thought on the same approach too, the problem was ReaderELF
had to be passed ReaderArchive too (or) at a bare minimum the
ReaderArchiveOptions. <br>
This would have broken things with other Readers. I discussed with
Michael on IRC and he said putting it in File should be fine for
now.<br>
<br>
The approach you mention would be very clean, if there is nice way
to pass in the ReaderArchive and in future, ReaderSharedLibrary.<br>
<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div><br>
<blockquote type="cite">
<div>+///</div>
<div>+/// The ReaderOptionsArchive encapsulates the options
used by the ReaderArchive.</div>
<div>+/// The option objects are the only way to control the
behaviour of Readers.</div>
<div>+///</div>
<div>+class ReaderOptionsArchive</div>
<div>+{</div>
<div>+public:</div>
<div>+ /* Create a reader options archive object with the
parameters</div>
<div>+ * reader - the reader object to read files</div>
<div>+ * argc,argv - for iterating through the command line
options</div>
<div>+ */</div>
<div>+ ReaderOptionsArchive(Reader *reader, int argc, char
*argv[]):</div>
<div>+ _is_force_load(false),</div>
<div>+ _reader(reader)</div>
<div>+ {</div>
<div>+ }</div>
<div>+ </div>
<div>+ bool is_force_load() const</div>
<div>+ {</div>
<div>+ return _is_force_load;</div>
<div>+ }</div>
<div>+ </div>
<div>+ Reader *reader() const</div>
<div>+ {</div>
<div>+ return _reader;</div>
<div>+ }</div>
<div>+ </div>
<div>+private:</div>
<div>+ bool _is_force_load;</div>
<div>+ Reader *_reader;</div>
<div>+};</div>
</blockquote>
<div>lld uses use camelCase (e.g. isForceLoad() and
_isForceLoad).</div>
</div>
</blockquote>
Will change this as appropriate.<br>
<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div>
<div><br>
</div>
<blockquote type="cite"><font class="Apple-style-span"
color="#000000"><br>
</font>
<div> /// Kinds of files that are supported.</div>
<div> enum Kind {</div>
<div>+ kindYAML, /// < yaml file (.yaml)</div>
<div> kindObject, ///< object file (.o)</div>
<div> kindSharedLibrary, ///< shared library (.so)</div>
<div> kindArchiveLibrary, ///< archive (.a)</div>
<div>@@ -52,6 +61,63 @@</div>
<div> return kindObject;</div>
<div> }</div>
<div><br>
</div>
</blockquote>
<div>There should not be kindYAML here. These kinds represent
the role not the format.</div>
<div><br>
</div>
</div>
</blockquote>
Ok.<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div>
<div><br>
</div>
<blockquote type="cite">
<div>+ /// Check if any member of the archive contains an
Atom with the</div>
<div>+ /// specified name and return the File object for that
member, or nullptr.</div>
<div>+ virtual const File *find(StringRef name, bool
dataSymbolOnly) const </div>
<div>+ {</div>
<div>+ error_code ec; </div>
<div>+ StringRef symname;</div>
<div>+ std::vector<std::unique_ptr<File>>
result;</div>
</blockquote>
<div>Move the variables down to their first use.</div>
<div><br>
</div>
</div>
</blockquote>
<br>
Ok.<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div><br>
<blockquote type="cite">
<div>+ llvm::object::Archive *_archive;</div>
</blockquote>
<div>If this is local variable it should not have an underscore
prefix.</div>
<div><br>
</div>
</div>
</blockquote>
ok.<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div>
<div><br>
</div>
<br>
<blockquote type="cite">
<div>+ </div>
<div>+ _archive = new llvm::object::Archive(_mb, ec);</div>
</blockquote>
<div>Why is the llvm Archive object instantiated on each call to
find(). It should be instantiated once.</div>
<br>
</div>
</blockquote>
Will move it to the constructor.<br>
<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div>
<blockquote type="cite">
<div>+ </div>
<div>+ if (ec)</div>
<div>+ return NULL;</div>
<div>+ </div>
<div>+ for (auto bs = _archive->begin_symbols(), </div>
<div>+ es = _archive->end_symbols(); bs != es;
++bs)</div>
<div>+ {</div>
<div>+ if ((ec = bs->getName(symname)))</div>
<div>+ return NULL;</div>
<div>+ </div>
<div>+ if (symname == name) {</div>
</blockquote>
<blockquote type="cite">
<div>+ llvm::object::Archive::child_iterator ci;</div>
<div>+ if ((ec = bs->getMember(ci)))</div>
<div>+ return NULL;</div>
<div>+ </div>
</blockquote>
<div>Can we add a find() method to llvm::object::Archive? and
let it walk the symbols or use whatever hashing is already
provided?</div>
</div>
</blockquote>
<br>
I will add a find() methd in llvm::object::Archive which will return
a child_iterator.<br>
<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div>
<blockquote type="cite">
<div>+ if ((ec = getSymbolForType(ci->getBuffer(),
symname, dataSymbolOnly)))</div>
<div>+ return NULL;</div>
</blockquote>
<div>The isDataSym is rarely true. The fast case should be when
it is false to skip this test.</div>
<div><br>
</div>
<br>
</div>
</blockquote>
yes, right. will change it.<br>
<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div>
<blockquote type="cite">
<div>+ </div>
<div>+ if ((ec =
_options.reader()->parseFile(std::unique_ptr<MemoryBuffer></div>
<div>+
(ci->getBuffer()), result)))</div>
<div>+ return NULL;</div>
<div>+ </div>
<div>+ for (std::unique_ptr<File> &f :
result) {</div>
<div>+ return f.release();</div>
<div>+ }</div>
</blockquote>
<div>This needs more comments explaining all the ownership
transferring that is going on.</div>
<div><br>
</div>
</div>
</blockquote>
Ok.<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div><br>
<blockquote type="cite">
<div>+ }</div>
<div>+ }</div>
<div>+ return NULL;</div>
<div>+ }</div>
<div>+</div>
<div>+ virtual void addAtom(const Atom&) {</div>
<div>+ llvm_unreachable("cannot add atoms to native .o
files");</div>
<div>+ }</div>
<div>+</div>
<div>+ virtual const atom_collection<DefinedAtom>
&defined() const {</div>
<div>+ return _definedAtoms;</div>
<div>+ }</div>
<div>+</div>
<div>+ virtual const atom_collection<UndefinedAtom>
&undefined() const {</div>
<div>+ return _undefinedAtoms;</div>
<div>+ }</div>
<div>+</div>
<div>+ virtual const atom_collection<SharedLibraryAtom>
&sharedLibrary() const {</div>
<div>+ return _sharedLibraryAtoms;</div>
<div>+ }</div>
<div>+</div>
<div>+ virtual const atom_collection<AbsoluteAtom>
&absolute() const {</div>
<div>+ return _absoluteAtoms;</div>
<div>+ }</div>
<div>+</div>
<div>+protected:</div>
<div>+ error_code getSymbolForType(MemoryBuffer *mb,
StringRef &symbol, </div>
<div>+ bool &isDataSym) const</div>
</blockquote>
<div>Why are the last two parameters by reference? Are they
in-out? </div>
<div><br>
</div>
<div>I think what you are trying to do in this function is check
if a particular symbol name in an object file is a data symbol
(as opposed to function). If so, the function could be
name isDataSymbol(). You might want to look into using a
StringRef instead of a MemoryBuffer, since this is just
pointers into the mapped archive file.</div>
<div><br>
</div>
</div>
</blockquote>
This is the pointer to the File thats contained within the Archive.
will change the function name.<br>
<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div><br>
<blockquote type="cite">
<div>+namespace lld </div>
<div>+{</div>
<div>+ /* Returns a vector of Files that are contained in the
archive file */</div>
<div>+ error_code ReaderArchive::parseFile(StringRef path,</div>
<div>+ <span class="Apple-tab-span" style="white-space:pre">
</span>std::vector<std::unique_ptr<File>>
&result)</div>
<div>+ {</div>
<div>+ error_code ec;</div>
<div>+ OwningPtr<llvm::MemoryBuffer> opmb;</div>
<div>+ if ((ec = llvm::MemoryBuffer::getFile(path, opmb)))</div>
<div>+ <span class="Apple-tab-span" style="white-space:pre">
</span>return ec;</div>
</blockquote>
<div>If we change ReaderArchive::parseFile() to be called by
specific readers after they've mapped the file, you'll want to
change the interface to this method to take a MemoryBuffer
instead of the file's path. </div>
<div><br>
</div>
</div>
</blockquote>
I will add another function in ReaderArchive which takes in a
MemoryBuffer, and leave this around.<br>
<blockquote
cite="mid:B0536C14-5E62-4653-A1B9-25F2C94559B5@apple.com"
type="cite">
<div><br>
<blockquote type="cite">
<div>+ </div>
<div>+ if (_options.is_force_load())</div>
<div>+ {</div>
<div>+ _archive = new llvm::object::Archive(opmb.take(),
ec);</div>
</blockquote>
<div>How is _archive ever deleted? If you make the ivar a
std::unique_ptr() it will be destroyed automatically in the
ReaderArchive() destructor.</div>
<div><br>
</div>
</div>
</blockquote>
Ok. Will use a smart pointer.<br>
<br>
Thanks<br>
<br>
Shankar Easwaran<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>