[llvm-commits] [lld] Supporting archive libraries in lld

[Nick Kledzik]

On Nov 6, 2012, at 8:30 AM, Shankar Easwaran wrote:

> Hi Nick, Michael,
> 
> I incorporated your comments in lld / llvm for supporting reading archive libraries.
> 
> Please look at the new changes and let me know if you are ok to commit this in.
Shankar, 

Looking good!  Just a few tweaks left:



The class FileArchive should just be a class local to lib/ReaderWriter/ReaderArchive.cpp

> Index: include/lld/Core/FileArchive.h
> ===================================================================
> --- include/lld/Core/FileArchive.h	(revision 0)
> +++ include/lld/Core/FileArchive.h	(revision 0)
> @@ -0,0 +1,142 @@
> +//===- Core/FileArchive.h - Support static library -----------------------===//


> +class FileArchive : public ArchiveLibraryFile {
> +public:
> +
> +  virtual ~FileArchive() { }
> +
> +  /// 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;  
> +    llvm::object::Archive::child_iterator ci;
> +
> +    if ((ec = (_archive.get()->findSym(name, ci))))
> +        return nullptr;
> +    
> +    if (dataSymbolOnly && (ec = isDataSymbol(ci->getBuffer(), name)))
> +      return nullptr;
> +    
> +    std::vector<std::unique_ptr<File>> result;
> +
> +    if ((ec = _options.reader()->parseFile(std::unique_ptr<MemoryBuffer>
> +                                           (ci->getBuffer()), result)))
> +      return nullptr;
> +
should be an assert() here that result.size() == 1, just to make sure parseFile() does not return multiple Files and we only use one of them.

> +    // give up the pointer so that this object no longer manages it
> +    for (std::unique_ptr<File> &f : result) {
> +      return f.release();
> +    }


> 


> Index: lib/ReaderWriter/ReaderArchive.cpp
> ===================================================================
> --- lib/ReaderWriter/ReaderArchive.cpp	(revision 0)
> +++ lib/ReaderWriter/ReaderArchive.cpp	(revision 0)
> @@ -0,0 +1,56 @@
> +//===- lib/ReaderWriter/ReaderArchive.cpp - Archive Library Reader--------===//
> +//
> +//                             The LLVM Linker
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===---------------------------------------------------------------------===//
> +#include "lld/ReaderWriter/ReaderArchive.h"
> +#include "lld/Core/FileArchive.h"
> +
> +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;
> +
> +    std::unique_ptr<MemoryBuffer> mb(opmb.take());
> +    
> +    return parseFile(std::move(mb), result);
> +  }

ReaderArchive::parseFile() should not be needed.  The base class Reader defines readFile() which already does the above.

> 
>  
>  #include <map>
> @@ -751,20 +752,36 @@
>  
>  class ReaderELF: public Reader {
>  public:
> -  ReaderELF(const ReaderOptionsELF &) {}
> +  ReaderELF(const ReaderOptionsELF &readerELFOptions,
> +            ReaderOptionsArchive &readerOptionsArchive)
> +         : _readerELFOptions(readerELFOptions),
> +           _readerOptionsArchive(readerOptionsArchive)
> +  { 
> +    _readerOptionsArchive.setReader(this);
> +    _readerArchive.reset(new ReaderArchive(_readerOptionsArchive));
> +  }
> +
>    error_code parseFile(std::unique_ptr<MemoryBuffer> mb, std::vector<
>        std::unique_ptr<File> > &result) {
>  
>      std::pair<unsigned char, unsigned char> Ident =
>          llvm::object::getElfArchType(&*mb);
>      llvm::error_code ec;
> +
> +    llvm::sys::LLVMFileType fileType = 
> +          llvm::sys::IdentifyFileType(mb->getBufferStart(),
> +                                static_cast<unsigned>(mb->getBufferSize()));
> +
> +    std::unique_ptr<File> f;
> +
> +    switch (fileType) {
> +
> +      case llvm::sys::ELF_Relocatable_FileType:
>      //    Instantiate the correct FileELF template instance
>      //    based on the Ident pair. Once the File is created
>      //     we push the file to the vector of files already
>      //     created during parser's life.
>  
> -    std::unique_ptr<File> f;
> -
>      if (Ident.first == llvm::ELF::ELFCLASS32 && Ident.second
>          == llvm::ELF::ELFDATA2LSB) {
>        f.reset(new FileELF<llvm::support::little, false>(std::move(mb), ec));
> @@ -781,13 +798,29 @@
>          == llvm::ELF::ELFDATA2LSB) {
>        f.reset(new FileELF<llvm::support::little, true> (std::move(mb), ec));
>      }
The getElfArchType() is currently always called.  It only needs to be called if the filetype is ELF_Relocatable_FileType.


> +        if (!ec)
> +          result.push_back(std::move(f));
> +        break;
> +
> +      case llvm::sys::Archive_FileType:
> +        ec = _readerArchive->parseFile(std::move(mb), result);
> +        break;
> +
> +      default:
> +        llvm_unreachable("not supported format");
> +        break;
> +    }
>  
>      if (ec)
>        return ec;
>  
> -    result.push_back(std::move(f));
>      return error_code::success();
>    }
> +
> +private:
> +  const ReaderOptionsELF &_readerELFOptions;
> +  ReaderOptionsArchive &_readerOptionsArchive;
> +  std::unique_ptr<ReaderArchive> _readerArchive;
>  };
Since ReaderELF constructor always  takes a ReaderOptionsArchive parameter, you can make _readerArchive be an instance of ReaderArchive rather than an owned pointer to a ReaderArchive.

-Nick