[PATCH] D15060: [ELF] - Implemented some GD, LD and IE TLS access models for x86 target.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/OutputSections.cpp:84
@@ -83,3 +83,3 @@
 template <class ELFT> void GotSection<ELFT>::addDynTlsEntry(SymbolBody *Sym) {
-  Sym->GotIndex = Target->getGotHeaderEntriesNum() + Entries.size();
+  Sym->GlobDynIndex = Target->getGotHeaderEntriesNum() + Entries.size();
   // Global Dynamic TLS entries take two GOT slots.
----------------
ruiu wrote:
> grimar wrote:
> > ruiu wrote:
> > > grimar wrote:
> > > > ruiu wrote:
> > > > > Why do we need a different member variable than GotIndex?
> > > > Thats needed for the next code sequence to work correctly:
> > > > 
> > > > ```
> > > > leal tls0 at tlsgd(,%ebx,1),%eax
> > > > call __tls_get_addr at plt
> > > > movl %gs:0,%eax
> > > > addl tls0 at gotntpoff(%ebx),%eax
> > > > ```
> > > > 
> > > > @tlsgd(x) - Allocates two contiguous entries in the GOT to hold a TLS_index structure.
> > > > @gotntpoff(x) - Allocates a entry in the GOT, and initializes it with the negative tlsoffset relative to the static TLS block. This is performed at runtime via the R_386_TLS_TPOFF relocation.
> > > > 
> > > > Problem is that next part of code terminates execution flow: 
> > > > 
> > > > ```
> > > > void Writer<ELFT>::scanRelocs(
> > > > .....
> > > >     if (Body && Body->isTLS() && Target->isTlsGlobalDynamicReloc(Type)) {
> > > >       if (Target->isTlsOptimized(Type, Body))
> > > >         continue;
> > > >       if (Body->isInGot())
> > > >         continue;
> > > > ```
> > > > Make it continue and unable to move forward and create R_386_TLS_TPOFF relocation (and single entry) below at the end of that method.
> > > Understood, but can you do that in a different way? I think the new variable is rather confusing, since symbols added to GOT using addDynTlsEntry are in GOT, but isInGot returns false for them (although they are in GOT).
> > First of all: I reproduced the same situation on x64 target and decided to split patch for that from this one: http://reviews.llvm.org/D15105.
> > 
> > About making that in a different way:
> > I think we can`t avoid using additional index variable because we need to use different got entry ids for different places. I mean we must know id of dynamic tls entry and the normal tls entry and use them both.
> > 
> > But I dont see problem here for design. At fact we dont use isInGot() for GD reloc because it is special case and processed separately everywhere. So its even more confusing to have this to respond isInGot() call when this call looks like should never be used in according to current logic.
> > 
> > I prepared that separate patch to make code look more consistent with code for LocalModuleTlsIndexOffset to hightlight their special nature (also updated a test case for x64 there).
> Mostly my concern is about the name isInGot(). If something is in GOT, that function should return true no matter how that is in GOT, because the function is named is-in-got.
There is no more that code in this patch :) It dissolved during other commits, so I guess its fine now.


http://reviews.llvm.org/D15060