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

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

s/implemet/implement/

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

David

lib/CodeGen/AsmPrinter/AsmPrinter.cpp
381

Should GVSym be just overwritten for emulated tls?

388

This comment seems redundent

412

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

422

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

440

createEmuTLSInitSym is called later again which seems wrong.

505

Just overwrite GVSym?

506

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

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

lib/CodeGen/AsmPrinter/AsmPrinter.cpp
381

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

440

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

505

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

506

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

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

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.