[llvm-commits] [lld] Support reading archive libraries

[Shankar Easwaran]

Hi Nick,

Thanks for the review.

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.
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.

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.

I have replied to all the other comments below.


>> +  ReaderOptionsArchive readerOptionsArchive(reader, argc, argv);
>> +  ReaderArchive *readerArchive = new 
>> ReaderArchive(readerOptionsArchive);
>> +
>> +
>>    for (auto path : cmdLineInputFilePaths) {
>>      std::vector<std::unique_ptr<File>> files;
>> -    if ( error(reader->readFile(path, files)) )
>> -      return 1;
>> +    File::Kind kind;
>> +    error_code EC;
Removed changes and removed File::FileType, and moved changes to Reader.

>> +
>> +    if (readerSelected == readerYAML) {
>> +        kind = File::kindYAML;
>> +    }
>> +    else
>> +    if ((EC = (File::FileType(path, kind)))) {
>> + return 1;
>> +    }
>> +    switch(kind)
>> +    {
>> +      case File::kindObject:
>> +      case File::kindYAML:
>> +        if ( error(reader->readFile(path, files)))
>> + return 1;
>> +        break;
>> +
>> +      case File::kindArchiveLibrary:
>> +        if (error(readerArchive->parseFile(path, files)))
>> + return 1;
>> +        break;
>> +
>> +      case File::kindSharedLibrary:
>> +        if (error(readerArchive->parseFile(path, files)))
>> + return 1;
>> +        break;
> Why are shared libraries going to the archive reader?
>
This was an error, and removed it.
>
>> +
>> +      default:
>> +        return 1;
>> +    }
>>      inputFiles.appendFiles(files);
>>    }
> 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.
>
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.
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.

The approach you mention would be very clean, if there is nice way to 
pass in the ReaderArchive and in future, ReaderSharedLibrary.

>
>> +///
>> +/// The ReaderOptionsArchive encapsulates the options used by the 
>> ReaderArchive.
>> +/// The option objects are the only way to control the behaviour of 
>> Readers.
>> +///
>> +class ReaderOptionsArchive
>> +{
>> +public:
>> +  /* Create a reader options archive object with the parameters
>> +   * reader - the reader object to read files
>> +   * argc,argv - for iterating through the command line options
>> +   */
>> +  ReaderOptionsArchive(Reader *reader, int argc, char *argv[]):
>> +                                 _is_force_load(false),
>> +                                 _reader(reader)
>> +  {
>> +  }
>> +
>> +  bool is_force_load() const
>> +  {
>> +    return _is_force_load;
>> +  }
>> +
>> +  Reader *reader() const
>> +  {
>> +    return _reader;
>> +  }
>> +
>> +private:
>> +  bool _is_force_load;
>> +  Reader *_reader;
>> +};
> lld uses use camelCase (e.g. isForceLoad() and _isForceLoad).
Will change this as appropriate.

>
>>
>>    /// Kinds of files that are supported.
>>    enum Kind {
>> +    kindYAML,              /// < yaml file (.yaml)
>>      kindObject,            ///< object file (.o)
>>      kindSharedLibrary,     ///< shared library (.so)
>>      kindArchiveLibrary,    ///< archive (.a)
>> @@ -52,6 +61,63 @@
>>      return kindObject;
>>    }
>>
> There should not be kindYAML here.   These kinds represent the role 
> not the format.
>
Ok.
>
>> +  /// Check if any member of the archive contains an Atom with the
>> +  /// specified name and return the File object for that member, or 
>> nullptr.
>> +  virtual const File *find(StringRef name, bool dataSymbolOnly) const
>> +  {
>> +    error_code ec;
>> +    StringRef symname;
>> +    std::vector<std::unique_ptr<File>> result;
> Move the variables down to their first use.
>

Ok.
>
>> +    llvm::object::Archive *_archive;
> If this is local variable it should not have an underscore prefix.
>
ok.
>
>
>> +
>> +    _archive = new llvm::object::Archive(_mb, ec);
> Why is the llvm Archive object instantiated on each call to find(). 
>  It should be instantiated once.
>
Will move it to the constructor.

>> +
>> +    if (ec)
>> +      return NULL;
>> +
>> +    for (auto bs = _archive->begin_symbols(),
>> +              es = _archive->end_symbols(); bs != es; ++bs)
>> +    {
>> +      if ((ec = bs->getName(symname)))
>> +          return NULL;
>> +
>> +      if (symname == name) {
>> +        llvm::object::Archive::child_iterator ci;
>> +        if ((ec = bs->getMember(ci)))
>> +          return NULL;
>> +
> Can we add a find() method to  llvm::object::Archive?  and let it walk 
> the symbols or use whatever hashing is already provided?

I will add a find() methd in llvm::object::Archive which will return a 
child_iterator.

>> +        if ((ec = getSymbolForType(ci->getBuffer(), symname, 
>> dataSymbolOnly)))
>> +          return NULL;
> The isDataSym is rarely true.  The fast case should be when it is 
> false to skip this test.
>
>
yes, right. will change it.

>> +
>> +        if ((ec = 
>> _options.reader()->parseFile(std::unique_ptr<MemoryBuffer>
>> + (ci->getBuffer()), result)))
>> +          return NULL;
>> +
>> +        for (std::unique_ptr<File> &f : result) {
>> +          return f.release();
>> +        }
> This needs more comments explaining all the ownership transferring 
> that is going on.
>
Ok.
>
>> +      }
>> +    }
>> +    return NULL;
>> +  }
>> +
>> +  virtual void addAtom(const Atom&) {
>> +    llvm_unreachable("cannot add atoms to native .o files");
>> +  }
>> +
>> +  virtual const atom_collection<DefinedAtom> &defined() const {
>> +    return _definedAtoms;
>> +  }
>> +
>> +  virtual const atom_collection<UndefinedAtom> &undefined() const {
>> +    return _undefinedAtoms;
>> +  }
>> +
>> +  virtual const atom_collection<SharedLibraryAtom> &sharedLibrary() 
>> const {
>> +    return _sharedLibraryAtoms;
>> +  }
>> +
>> +  virtual const atom_collection<AbsoluteAtom> &absolute() const {
>> +    return _absoluteAtoms;
>> +  }
>> +
>> +protected:
>> +  error_code getSymbolForType(MemoryBuffer *mb, StringRef &symbol,
>> +                              bool &isDataSym) const
> Why are the last two parameters by reference?  Are they in-out?
>
> 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.
>
This is the pointer to the File thats contained within the Archive. will 
change the function name.

>
>> +namespace lld
>> +{
>> +  /* Returns a vector of Files that are contained in the archive file */
>> +  error_code ReaderArchive::parseFile(StringRef path,
>> + std::vector<std::unique_ptr<File>> &result)
>> +  {
>> +    error_code ec;
>> +    OwningPtr<llvm::MemoryBuffer> opmb;
>> +    if ((ec = llvm::MemoryBuffer::getFile(path, opmb)))
>> + return ec;
> 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.
>
I will add another function in ReaderArchive which takes in a 
MemoryBuffer, and leave this around.
>
>> +
>> +    if (_options.is_force_load())
>> +    {
>> +      _archive = new llvm::object::Archive(opmb.take(), ec);
> How is _archive ever deleted?  If you make the ivar a 
> std::unique_ptr() it will be destroyed automatically in the 
> ReaderArchive() destructor.
>
Ok. Will use a smart pointer.

Thanks

Shankar Easwaran

-- 
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/20121102/f504cd95/attachment.html>