Page MenuHomePhabricator

[LLD][ELF] - Set DF_STATIC_TLS flag for i386 target.
ClosedPublic

Authored by grimar on Feb 5 2019, 6:16 AM.

Details

Summary

DF_STATIC_TLS flag indicates that the shared object or executable contains code using a static thread-local storage scheme.

Patch checks if IE/LE relocations were used to check if the code uses a static model. If so it sets the DF_STATIC_TLS flag.

It is based on D32354 which was reverted because we decided to not support the DF_STATIC_TLS at that time.
But now it will be used in FreeBSD.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Feb 5 2019, 6:16 AM
ruiu added a comment.Feb 5 2019, 10:58 AM

Could you give me a pointer to the FreeBSD change to support this feature?

ELF/Arch/X86.cpp
80 ↗(On Diff #185294)

I don't remember if this function is called from a multi-threaded part of lld. If that's the case, mutating a variable is not considered thread-safe, even if you are just mutating a boolean variable and set it to the same value from everywhere.

joerg added a subscriber: joerg.Feb 5 2019, 11:04 AM

The description is certainly not right. IE can be used from dlopen'd objects without problems as long as there is still reserved space around and the data is not initialized non-trivially or the dynamic linker is willing to fix up all threads. The point of the flag is to allow the dynamic linker to make a quick decision without having to scan the relocation table first. A classic user of this was libGL. It also applies to many platforms, not just i386. As such, please update the description before any commit.

grimar edited the summary of this revision. (Show Details)Feb 5 2019, 11:54 AM

The description is certainly not right. IE can be used from dlopen'd objects without problems as long as there is still reserved space around and the data is not initialized non-trivially or the dynamic linker is willing to fix up all threads. The point of the flag is to allow the dynamic linker to make a quick decision without having to scan the relocation table first. A classic user of this was libGL. It also applies to many platforms, not just i386. As such, please update the description before any commit.

I updated the description to say just "DF_STATIC_TLS flag indicates that the shared object or executable contains code using a static thread-local storage scheme."
i386 was mentioned only because this patch does that for this target only.

The description is certainly not right. IE can be used from dlopen'd objects without problems as long as there is still reserved space around and the data is not initialized non-trivially or the dynamic linker is willing to fix up all threads. The point of the flag is to allow the dynamic linker to make a quick decision without having to scan the relocation table first. A classic user of this was libGL. It also applies to many platforms, not just i386. As such, please update the description before any commit.

Indeed, it appears this was originally used to have the dlopen fail, but really it's just an indicator of the object's TLS use. Search for obj->static_tls in the FreeBSD review to see the proposed use.

grimar marked an inline comment as done.Feb 5 2019, 12:17 PM
grimar added inline comments.
ELF/Arch/X86.cpp
80 ↗(On Diff #185294)

Yes, you're right. We call it for relocateNonAlloc, it should be multithreaded.

Actually, I do not really like to have such code here.

I am using it for the following place now:
https://github.com/llvm-mirror/lld/blob/master/ELF/Relocations.cpp#L997

And I thought about just doing a static helper right above scanReloc like

static bool isStaticTlsReloc(RelType Type) {
  switch (Target) {
   case TargetA:
   return Type == Reloc1 ...;
  ...
   }
}

Cons would be that we will have a switch for targets and target specific relocations there,
though we already have a helper for MIPS in that file:
https://github.com/llvm-mirror/lld/blob/master/ELF/Relocations.cpp#L298

An alternative can be adding a Target::isStaticTlsReloc(RelType Type) method,
what is probably better, though I am not sure it is not too much for this little feature.

What would you prefer?

ruiu added a comment.Feb 5 2019, 12:48 PM

I think you can just use std::atomic_bool.

grimar updated this revision to Diff 185386.Feb 5 2019, 1:09 PM

OK, used std::atomic<bool>. (Not the std::atomic_bool, because LLD already use std::atomic<bool> form in ICF).

ruiu accepted this revision.Feb 5 2019, 3:20 PM

LGTM

This revision is now accepted and ready to land.Feb 5 2019, 3:20 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 5:37 AM