Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FR] auto-enable ELF TLS in clang driver when minSdkVersion is sufficiently high #1679

Closed
AndroidInternal opened this issue Mar 14, 2022 · 13 comments

Comments

@AndroidInternal
Copy link

AndroidInternal commented Mar 14, 2022

Description

Hi,

Disabled emulate tls With cflags to enable TLS:

-fno-emulated-tls
-Wl,-plugin-opt=-emulated-tls=0

Then ELF TLS works fine with the static library, and I can see .tbss section in the binary :

llvm-readelf libkmalloc_scudo.a -S | grep tbss

[275] .tbss._ZN5scudo18TSDRegistrySharedTINS_9AllocatorINS_13AndroidConfigEXadL_Z21scudo_malloc_postinitEEEELj32ELj8EE9ThreadTSDE NOBITS 0000000000000000 008f80 000008 00 WAGT  0   0  8
[281] .tbss._ZN5scudo18TSDRegistrySharedTINS_9AllocatorINS_19AndroidSvelteConfigEXadL_Z28scudo_svelte_malloc_postinitEEEELj2ELj1EE9ThreadTSDE NOBITS 0000000000000000 009280 000008 00 WAGT  0   0  8

But when adding LTO in flag:

-flto

Then the static library compiled is not an ELF format any more, instead it is LLVM IR code:

file flags_parser.cpp.o 
flags_parser.cpp.o: LLVM IR bitcode

Then .tbss section is not in the static library, and when a shared library compiled with this prebuilt static library, it will use emulate tls rather than elf tls.

Thanks.

Affected versions

r23

Canary version

No response

Host OS

Mac

Host OS version

macOS 12.2.1

Affected ABIs

arm64-v8a

Build system

CMake

Other build system

No response

minSdkVersion

30

Device API level

No response

@DanAlbert
Copy link
Member

@stephenhines Is this WAI, or is there some way that the IR can express the intended TLS mode?

@rprichard
Copy link
Collaborator

AFAIK we haven't added an ELF TLS -vs- emutls flag to the IR. i.e. This is WAI, at least for now.

Disabled emulate tls With cflags to enable TLS:

-fno-emulated-tls
-Wl,-plugin-opt=-emulated-tls=0

If I understand correctly: -Wl,-plugin-opt=-emulated-tls=0 is an ldflag, passed to the linker, which then configures an instance of Clang launched by the linker to compile IR. Archiving object files into a static library (whether IR or machine code) doesn't use the linker.

@AndroidInternal
Copy link
Author

Thanks for the reply and pointing out the improper ld flag.

By removing the ldflag

-Wl,-plugin-opt=-emulated-tls=0

And just use cflags

-fno-emulated-tls

Then will have the same result: works find when LTO disabled, but lost .tbss section in the static library.

Now this is WAI(works as intended),

Whether I can assume that it is not supported using ELF TLS in static library with LTO in Android?

Or whether there is another way to use both ELF TLS and LTO at the same time in static library?

@rprichard
Copy link
Collaborator

Or whether there is another way to use both ELF TLS and LTO at the same time in static library?

I think -Wl,-plugin-opt=-emulated-tls=0 is the right flag, but you need to pass it to clang when you link something (e.g. executable or shared library) that uses the static library. It would go on a list of LDFLAGS, not CFLAGS.

@AndroidInternal
Copy link
Author

AndroidInternal commented Mar 15, 2022

Thanks for the reminding.

I do miss the -Wl,-plugin-opt=-emulated-tls=0 in the LDFLAGS of the shared library in this case, since it works fine when LTO disabled I forgot to add it in the shared library LDFLAGS.

I'll close the issue.

@AndroidInternal
Copy link
Author

AndroidInternal commented Mar 15, 2022

Sorry I need to reopen this issue.. :)

Because I find it do have the same result after adding the ldflags, there isn't .tbss section in the final shared library either.

When LTO is not enabled, it have .tbss section like this:

llvm-readelf libkmalloc.so  -S
There are 32 section headers, starting at offset 0x89808:
...
  [21] .bss              NOBITS          0000000000014500 011500 04c4b8 00  WA  0   0 64
...

I've already add -Wl,-plugin-opt=-emulated-tls=0 in the LDFLAGS of the shared library

@AndroidInternal
Copy link
Author

Update: By adding the ldflag correctly can do solve this problem.

The wired behavior not works fine described above is causing by a confusing cmake ldflags behavior.

This can add the -Wl,-plugin-opt=-emulated-tls=0 in the final build.ninja rightly:

#CMakeLists.txt
SET (CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-plugin-opt=-emulated-tls=0")

#build.ninja
LINK_FLAGS = -Wl,--build-id=sha1 -Wl,--fatal-warnings -Wl,--no-undefined -Qunused-arguments -Wl,-plugin-opt=-emulated-tls=0

This can't generate wright ldflags in the final build.ninja:

#CMakeLists.txt
set_target_properties(
        libkmalloc_scudo
        PROPERTIES LINK_FLAGS -Wl,-plugin-opt=-emulated-tls=0
)

But according cmake document set_target_properties is supposed to works, but it isn't.

Whatever, by adding the ldflag correctly can do solve this problem, thanks again.

@DanAlbert
Copy link
Member

Whether I can assume that it is not supported using ELF TLS in static library with LTO in Android?

I probably should have said this is working as expected rather than as intended. The follow up work we have planned that will make this work smoothly is to teach the clang driver to do this automatically when your minSdkVersion is high enough for ELF TLS to be available. There's no reason each developer should need to configure this on their own; clang has all the information it needs to do the right thing by default.

It doesn't appear we have a bug filed to track that (it doesn't show up when I search for "tls", anyway), so I'll reopen this to track since it contains some useful context.

@DanAlbert DanAlbert reopened this Mar 16, 2022
@DanAlbert DanAlbert added this to Awaiting triage in LLVM via automation Mar 16, 2022
@DanAlbert DanAlbert changed the title [BUG] ELF TLS is not supported in static library when clang LTO enabled Mar 16, 2022
@brad0
Copy link

brad0 commented Jul 3, 2023

I happened upon this issue searching via Google, but I think the diff I commited was a start in the right direction..

llvm/llvm-project@b71f2fc

@DanAlbert
Copy link
Member

That may actually be "done" aside from us needing to pull that update (that might be in r26 but I'd have to dig to check)? Thanks again for that patch 👍

@rprichard do you know of anything else we'd need to do?

@rprichard
Copy link
Collaborator

No, I think that LLVM change is all we would need.

@brad0
Copy link

brad0 commented Aug 20, 2023

So the Android NDK r26 beta includes an update to LLVM 17 that would include this?

@DanAlbert DanAlbert moved this from Awaiting triage to Prebuilts submitted in LLVM Aug 23, 2023
@DanAlbert
Copy link
Member

It seems so. Updated the changelog: https://android-review.googlesource.com/c/platform/ndk/+/2724316

For future reference, you can look at the clang_source_info.md file in the NDK to learn that kind of thing:

Base revision: [14f0776550b5a49e1c42f49a00213f7f3fa047bf](https://github.com/llvm/llvm-project/commits/14f0776550b5a49e1c42f49a00213f7f3fa047bf)

That's newer than that commit, so yes.

rooteduniverse1003 pushed a commit to rooteduniverse1003/Android-Platform-NDK that referenced this issue Nov 22, 2023
Bug: android/ndk#1679
Test: None
(cherry picked from https://android-review.googlesource.com/q/commit:7d6e9b021806d6943cc13fc46ba5eb447f8283fe)
Merged-In: Ic6bd847379a99ad18910192fb3b3295f095259fa
Change-Id: Ic6bd847379a99ad18910192fb3b3295f095259fa
rooteduniverse1003 pushed a commit to rooteduniverse1003/Android-Platform-NDK that referenced this issue Nov 22, 2023
Bug: android/ndk#1679
Test: None
(cherry picked from https://android-review.googlesource.com/q/commit:7d6e9b021806d6943cc13fc46ba5eb447f8283fe)
Merged-In: Ic6bd847379a99ad18910192fb3b3295f095259fa
Change-Id: Ic6bd847379a99ad18910192fb3b3295f095259fa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants