[llvm-commits] [lld] add support for followon references

[Shankar Easwaran]

Hi Michael,

Thanks for your comments.

+  enum {
+    kindFollowOnReference = -1,
+  };
+

> This should be (along with changing the typedef above):
>
> enum class Kind : int32_t {
>    FollowOn = -1
> };

[Shankar] Will change.

>>     /// What sort of reference this is.
>>     virtual Kind kind() const = 0;
>>
>> @@ -72,7 +76,44 @@
>>     virtual ~Reference() {}
>>   };
>>
>> +/// FollowOnReference supports forcing layout of atoms, that one atom must
>> +/// follow another atom.
> Doxygen comments should follow:

> http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

Removing the class, per Nick's comments.

>
> +  void updateReferences(std::vector<Reference *> &referenceList)
> +  {
> No newline before {.
Sure.
>> +      ELFDefinedAtom<target_endianness, is64Bits> *previous_atom = NULL;
>> +      const Elf_Sym *previous_sym = NULL;
> Use nullptr.

Sure.

>> +
>>         // i.first is the section the symbol lives in
>>         for (auto si = symbols.begin(), se = symbols.end(); si != se; ++si) {
>>
>> @@ -640,7 +652,24 @@
>>           symbolData = llvm::ArrayRef<uint8_t>((uint8_t *)symbolContents.data()
>>                                       + (*si)->st_value, contentSize);
>>
>> +        Reference *followOn = NULL;
> nullptr.

Sure.
>> +        // Check to see if we need to add the FollowOn Reference
>> +        // We add a follow on reference only if current_symbol_value -
>> +        // previous_symbol_value + previous_symbol_size > 0
>> +        if (!isCommon && previous_sym) {
>> +          if (((*si)->st_value - (previous_sym->st_value +
>> +                                  previous_sym->st_size)) > 0) {
>> +
> No newline here.

Sure.

>
>           // Only relocations that are inside the domain of the atom are
> @@ -682,6 +711,17 @@
>                                *si, i.first, symbolData,
>                                referenceStart, _references.size(), _references);
>
> +        // Check to see if we need to add the FollowOn Reference
> +        // We add a follow on reference only if current_symbol_value -
> +        // previous_symbol_value + previous_symbol_size > 0
> This comment does not describe the code.
Will change the comment.

>> +        if (followOn) {
>> +          fprintf(stderr, "adding a followon reference\n");
> We don't use this. Use http://llvm.org/docs/ProgrammersManual.html#DEBUG
Will remove the fprintf.
>> +          followOn->setTarget(newAtom);
>> +        }
>> +
>> +        previous_atom = newAtom;
>> +        previous_sym = *si;
>> +
>>           _definedAtoms._atoms.push_back(newAtom);
>>           _symbolToAtomMapping.insert(std::make_pair((*si), newAtom));
>>
>> @@ -691,8 +731,11 @@
>>   // All the Atoms and References are created.  Now update each Reference's
>>   // target with the Atom pointer it refers to.
>>       for (auto &ri : _references) {
>> -      const Elf_Sym  *Symbol  = _objFile->getElfSymbol(ri->targetSymbolIndex());
>> -      ri->setTarget(findAtom (Symbol));
>> +      if (ri->kind() != Reference::kindFollowOnReference) {
>> +          ELFReference<target_endianness, is64Bits> *eri = (llvm::dyn_cast<ELFReference<target_endianness,is64Bits>>(ri));
>> +          const Elf_Sym  *Symbol  = _objFile->getElfSymbol(eri->targetSymbolIndex());
> Both of these lines are beyond 80 col.
>
>> +          ri->setTarget(findAtom (Symbol));
> No space before (

Will fix.

> .
>
>> +      }
>>       }
>>     }
>>
>> @@ -740,7 +783,7 @@
>>     std::map<llvm::StringRef, std::vector<const Elf_Rel *> >
>>              _relocationReferences;
>>
>> -  std::vector<ELFReference<target_endianness, is64Bits> *> _references;
>> +  std::vector<Reference *> _references;
>>     llvm::DenseMap<const Elf_Sym *, Atom *> _symbolToAtomMapping;
>>
>>     llvm::BumpPtrAllocator _readerStorage;
>>
> I can't confirm the test because I don't have followon-reference-test.elf-i386
> Can you send that too?
Attached.

Thanks

Shankar Easwaran

-- 
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: followon-reference-test.elf-i386
Type: application/octet-stream
Size: 1336 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121101/79b36941/attachment.obj>