[PATCH] [lld][ELF] Add support for reading dynamic libraries.
Shankar Kalpathi Easwaran
shankarke at gmail.com
Thu Feb 7 21:38:53 PST 2013
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:82-84
@@ +81,5 @@
+ virtual const SharedLibraryAtom *exports(StringRef name,
+ bool dataSymbolOnly) const {
+ // See if we have the symbol.
+ auto sym = _nameToSym.find(name);
+ if (sym == _nameToSym.end())
----------------
dataSymbolOnly is not being used in the code
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:85
@@ +84,3 @@
+ auto sym = _nameToSym.find(name);
+ if (sym == _nameToSym.end())
+ return nullptr;
----------------
The dynamic symbols may have undefined symbols and absolute symbols too, which the export should not pick.
================
Comment at: include/lld/Core/File.h:168-176
@@ -167,9 +167,11 @@
virtual atom_iterator<T> begin() const {
- return atom_iterator<T>(*this, reinterpret_cast<const void*>
- (_atoms.data()));
+ return atom_iterator<
+ T>(*this, _atoms.empty() ? 0 :
+ reinterpret_cast<const void *>(_atoms.data()));
}
virtual atom_iterator<T> end() const{
- return atom_iterator<T>(*this, reinterpret_cast<const void*>
- (_atoms.data() + _atoms.size()));
+ return atom_iterator<
+ T>(*this, _atoms.empty() ? 0 :
+ reinterpret_cast<const void *>(_atoms.data() + _atoms.size()));
}
virtual const T *deref(const void *it) const {
----------------
clang-format is making this unreadable
================
Comment at: include/lld/Core/LLVM.h:87-88
@@ +86,4 @@
+ size_t operator()(const llvm::StringRef &s) const {
+ using llvm::hash_value;
+ return hash_value(s);
+ }
----------------
Wouldnt this be simpler ? return llvm::hash_value(s) ?
================
Comment at: lib/ReaderWriter/ELF/Atoms.h:476-478
@@ +475,5 @@
+
+ virtual bool canBeNullAtRuntime() const {
+ return _symbol->getBinding() == llvm::ELF::STB_WEAK;
+ }
+
----------------
Couldnt follow this in the context of shared libraries ? Weak symbols are only applicable while generating static executables is what I thought
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:33-43
@@ +32,13 @@
+
+ llvm::OwningPtr<llvm::object::Binary> binaryFile;
+ if (error_code ec = createBinary(mb.release(), binaryFile))
+ return ec;
+
+ // Point Obj to correct class and bitwidth ELF object
+ file->_objFile.reset(
+ dyn_cast<llvm::object::ELFObjectFile<ELFT>>(binaryFile.get()));
+
+ if (!file->_objFile)
+ return make_error_code(llvm::object::object_error::invalid_file_type);
+
+ binaryFile.take();
----------------
Seen this code in other places in lld, should this be made a utility function ?
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:30-31
@@ +29,4 @@
+
+ static uint32_t lastOrdinal = 0;
+ file->_ordinal = lastOrdinal++;
+
----------------
This should be the order of the file that appears in the link. Having static here would not be correct.
================
Comment at: lib/ReaderWriter/ELF/Atoms.h:447-450
@@ +446,6 @@
+template <class ELFT>
+class ELFDynamicAtom LLVM_FINAL : public SharedLibraryAtom {
+ typedef llvm::object::Elf_Sym_Impl<ELFT> Elf_Sym;
+
+public:
+ ELFDynamicAtom(const DynamicFile<ELFT> &file, StringRef symbolName,
----------------
We need ELFDynamicDefinedAtom, ELFDynamicUndefinedAtom, ELFDynamicAbsoluteAtom. Unresolved symbols from shared libraries should take part in symbol resolution as well. Similiarly Absolute symbols in shared libraries.
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:51-60
@@ +50,12 @@
+ // it exists.
+ for (auto i = obj.begin_elf_dynamic_symbols(),
+ e = obj.end_elf_dynamic_symbols();
+ i != e; ++i) {
+ StringRef name;
+ if (error_code ec =
+ obj.getSymbolName(obj.getDynamicSymbolTableSectionHeader(), &*i,
+ name))
+ return ec;
+ file->_nameToSym[name].first = &*i;
+ }
+
----------------
Just reading the top level library wont be enough.
For example :-
cat > 1.c << \!
int a;
!
cat > 2.c << \!
int b;
!
gcc -fPIC -shared 1.c -o lib1.so
gcc -fPIC -shared 2.c -L. -l1 -o lib2.so
$readelf --dyn-syms lib2.so | grep -w a\$
$readelf --dyn-syms lib1.so | grep -w a\$
9: 0000000000201014 4 OBJECT GLOBAL DEFAULT 23 a
The symbol a would be only shown in lib1.so but not in lib2.so.
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:65-66
@@ +64,4 @@
+
+ virtual const atom_collection<DefinedAtom> &defined() const {
+ return _definedAtoms;
+ }
----------------
definedAtoms. undefinedAtoms are not being populated here.
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:77-79
@@ +76,5 @@
+
+ virtual const atom_collection<AbsoluteAtom> &absolute() const {
+ return _absoluteAtoms;
+ }
+
----------------
same here for absolute atoms
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:108-110
@@ +107,5 @@
+ mutable std::unordered_map<
+ StringRef,
+ std::pair<const typename llvm::object::ELFObjectFile<ELFT>::Elf_Sym *,
+ const SharedLibraryAtom *> > _nameToSym;
+ mutable llvm::BumpPtrAllocator _alloc;
----------------
I think using a class might be better than using std::pair
================
Comment at: lib/ReaderWriter/YAML/ReaderWriterYAML.cpp:224-232
@@ -226,6 +223,11 @@
+ virtual lld::File::atom_iterator<T> begin() const {
+ return lld::File::atom_iterator<
+ T>(*this,
+ _atoms.empty() ? 0 : reinterpret_cast<const void *>(_atoms.data()));
}
- virtual lld::File::atom_iterator<T> end() const{
- return lld::File::atom_iterator<T>(*this, reinterpret_cast<const void*>
- (_atoms.data() + _atoms.size()));
+ virtual lld::File::atom_iterator<T> end() const{
+ return lld::File::atom_iterator<
+ T>(*this, _atoms.empty() ? 0 :
+ reinterpret_cast<const void *>(_atoms.data() + _atoms.size()));
}
virtual const T *deref(const void *it) const {
----------------
Was this a bug earlier ? Any testcase that was failing ?
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:111
@@ +110,3 @@
+ const SharedLibraryAtom *> > _nameToSym;
+ mutable llvm::BumpPtrAllocator _alloc;
+};
----------------
Why mutable ? Once const is removed, the mutable keyword can be removed from both the member variables.
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:23-26
@@ +22,6 @@
+namespace elf {
+template <class ELFT> class DynamicFile LLVM_FINAL : public SharedLibraryFile {
+public:
+ static ErrorOr<std::unique_ptr<DynamicFile> > create(
+ const ELFTargetInfo &ti, std::unique_ptr<llvm::MemoryBuffer> mb) {
+ std::unique_ptr<DynamicFile> file(
----------------
It might be useful to store the soname, http://tldp.org/HOWTO/Program-Library-HOWTO/shared-libraries.html
================
Comment at: lib/ReaderWriter/ELF/DynamicFile.h:82
@@ +81,3 @@
+ virtual const SharedLibraryAtom *exports(StringRef name,
+ bool dataSymbolOnly) const {
+ // See if we have the symbol.
----------------
Shankar Kalpathi Easwaran wrote:
> dataSymbolOnly is not being used in the code
maybe const can be removed ?
================
Comment at: lib/ReaderWriter/ELF/Reader.cpp:58
@@ +57,3 @@
+ template <class ELFT>
+ static result_type create(const lld::ELFTargetInfo &ti,
+ std::unique_ptr<llvm::MemoryBuffer> mb) {
----------------
why static ?
================
Comment at: lib/ReaderWriter/ELF/Reader.cpp:68
@@ +67,3 @@
+ template <class ELFT>
+ static result_type create(const lld::ELFTargetInfo &ti,
+ std::unique_ptr<llvm::MemoryBuffer> mb,
----------------
same
================
Comment at: lib/ReaderWriter/ELF/Reader.cpp:168-170
@@ -71,61 +167,5 @@
case llvm::sys::ELF_Relocatable_FileType: {
- std::pair<unsigned char, unsigned char> Ident = getElfArchType(&*mb);
- std::unique_ptr<File> f;
- // Instantiate the correct File 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.
- if (Ident.first == llvm::ELF::ELFCLASS32 &&
- Ident.second == llvm::ELF::ELFDATA2LSB) {
-#if !LLVM_IS_UNALIGNED_ACCESS_FAST
- if (MaxAlignment >= 4)
- f.reset(new ELFFile<ELFType<llvm::support::little, 4, false> >(
- _elfTargetInfo, std::move(mb), ec));
- else
-#endif
- if (MaxAlignment >= 2)
- f.reset(new ELFFile<ELFType<llvm::support::little, 2, false> >(
- _elfTargetInfo, std::move(mb), ec));
- else
- llvm_unreachable("Invalid alignment for ELF file!");
- } else if (Ident.first == llvm::ELF::ELFCLASS32 &&
- Ident.second == llvm::ELF::ELFDATA2MSB) {
-#if !LLVM_IS_UNALIGNED_ACCESS_FAST
- if (MaxAlignment >= 4)
- f.reset(new ELFFile<ELFType<llvm::support::big, 4, false> >(
- _elfTargetInfo, std::move(mb), ec));
- else
-#endif
- if (MaxAlignment >= 2)
- f.reset(new ELFFile<ELFType<llvm::support::big, 2, false> >(
- _elfTargetInfo, std::move(mb), ec));
- else
- llvm_unreachable("Invalid alignment for ELF file!");
- } else if (Ident.first == llvm::ELF::ELFCLASS64 &&
- Ident.second == llvm::ELF::ELFDATA2MSB) {
-#if !LLVM_IS_UNALIGNED_ACCESS_FAST
- if (MaxAlignment >= 8)
- f.reset(new ELFFile<ELFType<llvm::support::big, 8, true> >(
- _elfTargetInfo, std::move(mb), ec));
- else
-#endif
- if (MaxAlignment >= 2)
- f.reset(new ELFFile<ELFType<llvm::support::big, 2, true> >(
- _elfTargetInfo, std::move(mb), ec));
- else
- llvm_unreachable("Invalid alignment for ELF file!");
- } else if (Ident.first == llvm::ELF::ELFCLASS64 &&
- Ident.second == llvm::ELF::ELFDATA2LSB) {
-#if !LLVM_IS_UNALIGNED_ACCESS_FAST
- if (MaxAlignment >= 8)
- f.reset(new ELFFile<ELFType<llvm::support::little, 8, true> >(
- _elfTargetInfo, std::move(mb), ec));
- else
-#endif
- if (MaxAlignment >= 2)
- f.reset(new ELFFile<ELFType<llvm::support::little, 2, true> >(
- _elfTargetInfo, std::move(mb), ec));
- else
- llvm_unreachable("Invalid alignment for ELF file!");
- }
- if (!ec)
- result.push_back(std::move(f));
+ std::unique_ptr<File> f(createELF<ELFFile>(getElfArchType(&*mb),
+ MaxAlignment, _elfTargetInfo,
+ std::move(mb), ec));
+ if (ec)
----------------
Why is the return value not taken and compared, may be a TODO action item can be set at this location.
================
Comment at: lib/ReaderWriter/ELF/Reader.cpp:162-163
@@ -65,4 +161,4 @@
std::size_t MaxAlignment =
1ULL << llvm::CountTrailingZeros_64(uintptr_t(mb->getBufferStart()));
----------------
Why MaxAlignment for shared libraries too ?
http://llvm-reviews.chandlerc.com/D382
More information about the llvm-commits
mailing list