[PATCH] D18330: [ELF/AArch64] Implemented set of R_AARCH64_TLSDESC_* relocations.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/OutputSections.cpp:293
@@ +292,3 @@
+  // Tail here is amount of such special relocations.
+  Relocs.insert(Relocs.end() - Tail, Reloc);
+}
----------------
zatrazz wrote:
> emaste wrote:
> > grimar wrote:
> > > emaste wrote:
> > > > zatrazz wrote:
> > > > > emaste wrote:
> > > > > > I wonder if there's a nicer way to handle this?
> > > > > I though about this, but I think to remove the Tail it would require to maintain two or move relocation vectors (one for normal and another for tlsdesc) and this will just add more complexity IMHO.
> > > > > 
> > > > > However we might change in later if this shows to be a performance issue.
> > > > > However we might change in later if this shows to be a performance issue.
> > > > 
> > > > To me having two vectors seems more clear. Anyway, we can certainly try it both ways alter on and evaluate the performance difference.
> > > Problem here is that 2 vectors are not really a solution. I mean for this place we just need to put these relocations after all others, but actually there are other similar group of relocations also: IRelative, which are also should be placed at tail. If we one day we will want to implement them (I mean dynamic case, for static one we already did it and static case  does not have that problem), then 2 vectors probably is not the best design as we will need 3 of them then ?
> > > 
> > > At the same time, probably, to have two vectors for now would be more clear, I just kept in mind that all stuff above when implemented that in my version of patch and decided to have one vector basing on that thoughts.
> > I tried a quick experiment with a std::vector<int>. First case called push_back 10 times, then `insert(v.end() - 10, i)`
> Indeed 2 vectors is not a definitely solution, but I think we can use current approach now and think about a more flexible solution later (since now only aarch64 requires tlsdesc support in this layout).
> 
> I think a better solution would be to add a variable number of vectors with the addTls* function to add on the specific one (for instance 'jump slot', 'tlsdesc', and 'irelative'). It could be a DenseMap or similar associating an type to a vector. The Target will then provide the desirable layout for the got.plt and the class will write down as specified. What do you think?
I would not use 2 vectors. Because that is probably not solves the problem in general at all. Having 2 of them now and 3 later just will bring the complication of whole code.
I am sure there should be much simplier solution for that. I have no ideas currently unfortunately though.
So I would leave that as is now, that place anyways needs refactoring, I think we all agree with that.


Repository:
  rL LLVM

http://reviews.llvm.org/D18330