[llvm-commits] ELFReader.cpp update - Add support for References.

[Sid Manning]

My comments are below and updated patches are attached.

One important new change is that I'm passing the References list to 
DefinedAtom so derefIterator can access the it.


Thanks,

On 09/01/12 00:04, Michael Spencer wrote:
> On Fri, Aug 31, 2012 at 12:40 PM, Sid Manning<sidneym at codeaurora.org>  wrote:

...

>> +// Relocation References: Defined Atom may contain
>> +// references that will need to be patched before
>> +// the executable is written.
>> +//
>> +class ELFReference : public Reference {
>> +public:
>> +  ELFReference(Reference::Kind K,
>> +               uint64_t O,
>> +               const Atom *T,
>> +               uint64_t N,
>> +               Reference::Addend A)
>> +    : Target(T)
>> +    , TargetNameOffset(N)
>> +    , OffsetInAtom(O)
>> +    , Addend(A)
>> +    , Kind(K) { }
>
> Why doesn't this just take a tagged union of Elf_Rel_Impl* (for Rel and Rela)
> and grab the info from that on a call?

Turned to a template, passing Elf_Rel(a) to

...

>> +private:
>> +  const Atom*  Target;
>> +  uint64_t     TargetNameOffset;
>
> TargetNameOffset is a weird way to handle mapping from ELF_R_SYM to Atom*. I
> think the best way to do this would be to add a public
> Elf_Sym *getSymbol(uint32_t Index)
> function to ELFObjectFile. Then add a DenseMap<ElfSym*, Atom*>  to go from
> ELF_R_SYM to Atom*.
>
DenseMap added and getSymbol(uint32_t Index) has been added to ELF.h. 
Passing ELF_Rel(a) to constructor now.

...

>> +  struct NameAtomPair {
>> +                 NameAtomPair(StringRef N,
>> +                              Atom *A)
>> +                   : name(N), atom(A) {}
>> +    StringRef name;
>> +    Atom *atom;
>> +  };
>> +
>
> Use std::pair.
Done, sorta.  Its been removed because due to the use of 
llvm::DenseMap<const Elf_Sym *, Atom *>

...

>> +      if (section->sh_type == llvm::ELF::SHT_RELA)
>> +      {
>
> No new line before {
Done.

...

>> +        sectionName = sectionName.drop_front(5);
>> +        for (unsigned int i=0; i<contents.size()/sizeof(Elf_Rela); i++) {
>
> This is incorrect. The distance between relocations is defined by
> Elf_Shdr::st_entsize. This type of iteration should probably be added to
> ELFObjectFile.
Done, using section->getEntityCount().

>
>> +          RelocationAddendReferences[sectionName].push_back(relocs+i);
>
> A reference to RelocationAddendReferences[sectionName] should be
> hoisted outside of the loop.
>
Done.

...

>> -        AbsoluteAtoms._atoms.push_back(
>> -                             new (AtomStorage.Allocate<ELFAbsoluteAtom>  ())
>> -                             ELFAbsoluteAtom(*this, SymbolName,
>> -                                             Symbol->st_value));
>> +        ELFAbsoluteAtom<target_endianness, is64Bits>  *NewAtom = new
>> +        ELFAbsoluteAtom<target_endianness, is64Bits>  (*this,
>> +                                                      SymbolName,
>> +                                                      Symbol->st_value);
>
> Why is this not using placement new in AtomStorage? Also, you can use auto here.
> Same for the rest of these.
This was mistake I made when I reworked the code, fixed.


>
>> +        // Make Elf_Rela references
>> +        typename std::vector<Elf_Rela *>::iterator rai  =
>> +                      RelocationAddendReferences[SectionName].begin();
>> +        typename std::vector<Elf_Rela *>::iterator rae =
>> +                      RelocationAddendReferences[SectionName].end();
>> +
>> +        // Only relocations that are inside the domain of the atom are
>> +        // added.
>> +        for (; rai != rae; rai++) {
>
> This can use a c++11 for loop.
Using "for (auto& rai : RelocationAddendReferences[SectionName])"

>
>> +          if (((*rai)->r_offset>= (*si)->st_value)&&
>> +              ((*rai)->r_offset<  (*si)->st_value+contentSize)) {
>> +
>> +            ELFReference *eref = new ELFReference (
>> +                (*rai)->getType(), (*rai)->r_offset-(*si)->st_value,
>> +                nullptr, (*rai)->getSymbol(), (*rai)->r_addend);
>
> This should use a BumpPtrAllocator too. Currently it's just leaked.

Added ReaderStorage to store both References and Atoms.


>
> - Michael Spencer


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ELF.h.diff
Type: text/x-patch
Size: 1210 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120907/bafbaf78/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ReaderELF.cpp.diff
Type: text/x-patch
Size: 16211 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120907/bafbaf78/attachment-0001.bin>