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

George Rimar via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 1 03:40:26 PST 2015


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:
> > > 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).

================
Comment at: ELF/OutputSections.h:126
@@ -125,1 +125,3 @@
+  uintX_t getGlobalDynAddr(const SymbolBody &B) const;
+  uintX_t getEntriesNum() const { return Entries.size(); }
 
----------------
ruiu wrote:
> getNumEntries
Will be fixed in update.

================
Comment at: ELF/Target.cpp:350
@@ +349,3 @@
+    write32le(Loc, Offset);
+  } break;
+  case R_386_TLS_LDO_32:
----------------
ruiu wrote:
>     break;
>   }
Will be fixed in update.


http://reviews.llvm.org/D15060





More information about the llvm-commits mailing list