[llvm-commits] PATCH: lld ReaderELF.cpp update

[Sid Manning]

On 07/13/12 18:16, Michael Spencer wrote:
> On Fri, Jul 13, 2012 at 11:44 AM, Sid Manning<sidneym at codeaurora.org>  wrote:

>> +  const Elf_Shdr *getSection(DataRefImpl index) const;
>> +  const Elf_Sym  *getSymbol(DataRefImpl Symb) const;
>>     const Elf_Dyn  *getDyn(DataRefImpl DynData) const;
>>     error_code getSymbolVersion(SymbolRef Symb, StringRef&Version,
>>                                 bool&IsDefault) const;
>>
>
> I realize you were following the example of getDyn here. However, all
> of these should take a {section,symbol,dynasymbol,relocation}_iterator
> instead as COFF does. DataRefImpl should never be used by clients of
> LLVMObject. There's no need for you to change getDyn over if it's not
> trivial.
So I would add new methods to ELF.h similar to getCOFFSection/Symbol.  I 
will give this a try.


>> +:# RUN: lld-core -reader ELF %p/Inputs/object-test.elf-i386 | FileCheck %s -check-prefix ELF-i386
>> +:# RUN: lld-core -reader ELF %p/Inputs/object-test.elf-hexagon | FileCheck %s -check-prefix ELF-hexagon
>> +
>
> What's with the : at the front of the run lines?
>
Removed.  These are leftovers from when I prefixed the ELF-XX: results.


> Index: lib/ReaderWriter/ELF/ReaderELF.cpp
>> +//===---------------------------------------------------------------------===//
>
> These lines are the wrong length.

Easy to fix, didn't notice.

>
>>
>>   #include "lld/ReaderWriter/ReaderELF.h"
>>   #include "lld/Core/File.h"
>>
>> +#include "llvm/ADT/ArrayRef.h"
>> +#include "llvm/ADT/StringRef.h"
>> +#include "llvm/Support/Allocator.h"
>>   #include "llvm/Support/Casting.h"
>>   #include "llvm/Support/ErrorHandling.h"
>>   #include "llvm/Support/Memory.h"
>>   #include "llvm/Support/MemoryBuffer.h"
>>   #include "llvm/Support/raw_ostream.h"
>>   #include "llvm/Support/system_error.h"
>> +#include "llvm/Support/Endian.h"
>> +#include "llvm/Support/MathExtras.h"
>> +#include "llvm/Support/ELF.h"
>> +#include "llvm/Object/ObjectFile.h"
>> +#include "llvm/Object/ELF.h"
>
> These need to be sorted in alphabetical order.
>
No problem.

>> +class ELFAbsoluteAtom: public AbsoluteAtom {
>> +
>> +public:
>> +  ELFAbsoluteAtom(const File&F,
>> +                  llvm::StringRef N,
>> +                  uint64_t V)
>> +    : OwningFile(F)
>> +    , Name(N)
>> +    , Value(V)
>> +  {}
>> +
>> +  virtual const class File&  file() const {
>
> &  on the right side. A few other places need this too.
>
The above declaration was copied from the PECOFF reader and I'm not sure 
what to change.


>> +    for (; si != se; si.increment(EC)) {
>> +      if (EC)
>> +        llvm::report_fatal_error("Could not read all symbols");
>> +      llvm::StringRef SymbolName;
>> +      llvm::StringRef SectionName;
>> +
>> +      llvm::object::SymbolRef::Type type;
>> +      uint32_t flags = 0;
>> +
>> +      EC = si->getName(SymbolName);
>
> For this and all the below code, why are you using the generic
> interface instead of the ELF specific interface? Quite a bit of this
> would be a lot cleaner with the ELF interface.
>
The generic interface seemed to work, si->getName returns the symbol 
name.  If methods similar to getCOFFSection/Symbol taking the iterator 
are available and getSymbolName (const Elf_Shdr *,const Elf_sym *, 
StringRef) is made public then I think the generic interface calls can 
be done away with.  I will try to work this out.



>> +      if (EC)
>> +        llvm::report_fatal_error("Could not get symbol name");
>> +
>> +      EC = si->getType(type);
>> +      if (EC)
>> +        llvm::report_fatal_error("Could not get symbol type");
>> +
>> +      EC = si->getFlags(flags);
>> +      if (EC)
>> +        llvm::report_fatal_error("Could not get symbol flags");
>> +
>> +      if ((flags&  llvm::object::SymbolRef::SF_Absolute)
>> +           == llvm::object::SymbolRef::SF_Absolute) {
>> +        uint64_t AbsValue;
>> +
>> +        EC = si->getAddress(AbsValue);
>> +        if (EC)
>> +          llvm::report_fatal_error(
>> +              "Could not get absolute symbol st_value");
>> +
>> +        AbsoluteAtoms._atoms.push_back(
>> +                             new (AtomStorage.Allocate<ELFAbsoluteAtom>  ())
>> +                             ELFAbsoluteAtom(*this, SymbolName, AbsValue));
>> +
>> +      } else if ((flags&  llvm::object::SymbolRef::SF_Undefined)
>> +                       == llvm::object::SymbolRef::SF_Undefined) {
>> +
>> +        // Get a pointer to the symbol:
>> +        const Elf_Sym *Symbol = (Obj->getSymbol(si->getRawDataRefImpl()));
>> +
>> +        UndefinedAtoms._atoms.push_back(
>> +            new (AtomStorage.Allocate<ELFUndefinedAtom<
>> +                 target_endianness, is64Bits>  >  ())
>
> No space needed between the>  and>. Also there should be no space
> between the>  and (.
>
No problem.

>> +                 ELFUndefinedAtom<target_endianness, is64Bits>  (
>> +                                 *this, SymbolName, Symbol));
>> +
>> +      } else {
>> +        uint32_t SymbolFlags;
>> +        EC = si->getFlags(SymbolFlags);
>> +        if (EC)
>> +          llvm::report_fatal_error("Could not get symbol flags");
>> +
>> +        //TODO We need to think about how to put other
>> +        //symbol types into the defined atom model.
>> +
>> +        if (type == llvm::object::SymbolRef::ST_Data ||
>> +            type == llvm::object::SymbolRef::ST_Function ||
>> +            type == llvm::object::SymbolRef::ST_Unknown) {
>> +
>> +          llvm::object::SectionRef SR;
>> +          llvm::object::section_iterator section(SR);
>> +
>> +          EC = si->getSection(section);
>> +          if (EC)
>> +            llvm::report_fatal_error("Could not get section iterator");
>> +
>> +          EC = section->getName(SectionName);
>> +          if (EC)
>> +            llvm::report_fatal_error("Could not get section name");
>> +
>> +          StringRef symbolContents;
>> +          EC = section->getContents (symbolContents);
>> +          if (EC)
>> +              llvm::report_fatal_error("Could not get section contents");
>> +
>> +          uint64_t symbolSize;
>> +          EC = si->getSize (symbolSize);
>> +          if (EC)
>> +            llvm::report_fatal_error("Could not get symbol size");
>> +
>> +          uint64_t symbolOffset;
>> +          EC = si->getAddress (symbolOffset);
>> +          if (EC)
>> +            llvm::report_fatal_error("Could not get symbol address");
>> +
>> +          bool IsCommon = false;
>> +          if ((SymbolFlags&  llvm::object::SymbolRef::SF_Common)
>> +               == llvm::object::SymbolRef::SF_Common)
>> +            IsCommon = true;
>> +
>> +          // Get the symbol's content:
>> +          llvm::ArrayRef<uint8_t>  SymbolData((uint8_t *)symbolContents.data()
>> +                                             + symbolOffset,
>> +                                             (IsCommon) ? 0 : symbolSize);
>
> st_size is not guaranteed to be the size of the data associated with
> the symbol. See the MachO and COFF reader for how to properly deal
> with this. This is important because every byte in a section must be
> allocated to an atom, as sections can have internal references that we
> don't know about that must be maintained.
>
I think get this.  When functions don't use .size their size will be 
recored as zero so we have to take the difference between this address 
and the next.


>> +
>> +          // Get a pointer to the section the symbol lives in:
>> +          llvm::object::DataRefImpl ShdrRef = section->getRawDataRefImpl();
>> +          Elf_Shdr *Section = const_cast<Elf_Shdr*>
>> +                              (reinterpret_cast<const Elf_Shdr *>(ShdrRef.p));
>
> Why is there a const cast here? ELFDefinedAtom takes a const Elf_Shdr
> *. Also this should not rely on the implementation details of
> DataRefImpl to get the Elf_Shdr pointer.
>

I ripped this off from DyldELFObject in RuntimeDynldELF.cpp, If some of 
the interfaces changes mentioned above are done then this shouldn't be 
needed.

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum