This is an archive of the discontinued LLVM Phabricator instance.

Implement target independent TLS compatible with glibc's emutls.c.
ClosedPublic

Authored by chh on Jun 17 2015, 4:39 PM.

Details

Summary
  • The 'common' section TLS is not implemented. Current C/C++ TLS variables are not placed in common section.
  • DWARF debug info to get the address of TLS variables is not generated yet.

clang and driver: (in http://reviews.llvm.org/D10524)

  • Added -femulated-tls flag to select the emulated TLS model, which will be used for old targets like Android that do not support ELF TLS models.
  • Added unit tests to check the effect of -femulated-tls.

llvm:

  • Added TargetLowering::LowerToTLSEmulatedModel as a target-independent function to convert a SDNode of TLS variable address to a function call to __emutls_get_address.
  • Added into lib/Target/*/*ISelLowering.cpp to call LowerToTLSEmulatedModel for TLSModel::Emulated. Although all targets supporting ELF TLS models are enhanced, emulated TLS model has been tested only for Android ELF targets.
  • Modified AsmPrinter.cpp to print the emutls_v.* and emutls_t.* variables for emulated TLS variables.
  • Modified DwarfCompileUnit.cpp to skip some DIE for emulated TLS variabls. TODO: Add proper DIE for emulated TLS variables.
  • Added new unit tests with emulated TLS.

Diff Detail

Repository
rL LLVM

Event Timeline

chh updated this revision to Diff 27895.Jun 17 2015, 4:39 PM
chh retitled this revision from to Implement target independent TLS compatible with glibc's emutls.c..
chh updated this object.
chh edited the test plan for this revision. (Show Details)
chh added a comment.Jun 17 2015, 4:44 PM

The required changes to clang and driver are in http://reviews.llvm.org/D10524.

chh set the repository for this revision to rL LLVM.Jun 17 2015, 4:52 PM
chh added a subscriber: enh.Jun 17 2015, 5:06 PM
chh updated this object.Jun 18 2015, 4:53 PM
chh added a subscriber: resistor.
chh removed a subscriber: resistor.
chh added a subscriber: Unknown Object (MLST).Jun 24 2015, 5:17 PM

Hi David,

Me, Chandler, Tim, or Owen (already on) are probably the best bets to review this.

-eric

danalbert added a subscriber: danalbert.

Chris: Nick and Chandler mentioned that you're probably interested in this support too.

beanz edited edge metadata.Jul 14 2015, 2:15 PM

I'm definitely not the right person to approve this patch, but I skimmed it anyways. I did notice one small spelling error in one of the comments I called it out inline.

Really excited to see this going in! Thanks for putting this together.

docs/LangRef.rst
490 ↗(On Diff #27895)

s/implemet/implement/

The compile_rt runtime support is still missing. Is it in a different patch?

David

lib/CodeGen/AsmPrinter/AsmPrinter.cpp
381 ↗(On Diff #27895)

Should GVSym be just overwritten for emulated tls?

388 ↗(On Diff #27895)

This comment seems redundent

412 ↗(On Diff #27895)

Note that this check is not necessary -- see previous early return.

422 ↗(On Diff #27895)

Should this be moved into a helper routine to improve readability?

440 ↗(On Diff #27895)

createEmuTLSInitSym is called later again which seems wrong.

505 ↗(On Diff #27895)

Just overwrite GVSym?

506 ↗(On Diff #27895)

This was called before.

chh added a comment.Jul 14 2015, 3:04 PM

David, I have no patch for compile_rt since __emutls_get_address is already in libc.so of Android. Is that your concern?

enh added a comment.Jul 14 2015, 3:49 PM

not really though --- i think you're getting that from libgcc. so
although short term you don't need to add this stuff to compiler-rt to
test, it will be necessary for anyone not using libgcc. (like us, at
some point in the future.)

chh added a comment.Jul 14 2015, 3:55 PM

I see. It's probably more a package and license issue. GCC's code is small in
https://github.com/gcc-mirror/gcc/blob/master/libgcc/emutls.c
Maybe llvm compiler-rt expert can suggest a solution?

The emulated tls support won't be complete until the builtins are
supported by llvm runtime. You will need to implemented. See
http://compiler-rt.llvm.org/ for project info.

David

chh added a comment.Jul 14 2015, 5:25 PM

I will try an alternative flag as suggested in http://reviews.llvm.org/D10524.
This patch will be quite different with the new flag.

chh marked 3 inline comments as done.Jul 16 2015, 5:31 PM
chh added inline comments.
docs/LangRef.rst
490 ↗(On Diff #27895)

This part will be removed due to new simplified clang/driver flags.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp
381 ↗(On Diff #27895)

They are two different symbols in the emulated mode.
It helps debugging when GVSym is available even not emitted.

440 ↗(On Diff #27895)

It's on purpose to get-or-create the symbol from one place, and createEmuTLSInitSym can be called multiple times.

505 ↗(On Diff #27895)

It's useful to keep under emulated mode and debug both GVSym and EmittedInitSym.

506 ↗(On Diff #27895)

It's okay. It returns the previously created symbol.

srhines added inline comments.Jul 16 2015, 5:38 PM
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
440 ↗(On Diff #27895)

In that case, let's not call the function "create...", since that implies something different. It might be better to call it getEmuTLSInitSym().

davidxl added inline comments.Jul 16 2015, 5:38 PM
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
440 ↗(On Diff #27895)

make it clear to change the interface name to be something like 'getOrCreat ...'

chh updated this revision to Diff 29974.Jul 16 2015, 8:35 PM
chh updated this object.
chh edited edge metadata.
chh removed rL LLVM as the repository for this revision.

Diff 2 uses -femulated-tls flag instead of the "emulated" attribute in C/C++ and IL level.
This will work with Diff 3 in http://reviews.llvm.org/D10524.

chh added a comment.Jul 23 2015, 5:13 PM

Any comment on the new Diff 2, 29974?
Does it address all previous comments?

rnk accepted this revision.Jul 27 2015, 3:05 PM
rnk added a reviewer: rnk.
rnk added a subscriber: rnk.

Looks good to me.

This revision is now accepted and ready to land.Jul 27 2015, 3:05 PM
This revision was automatically updated to reflect the committed changes.