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.

Sign up for GitHub

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

add support for Thread local Storage. #155

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add support for Thread local Storage. #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cafe/coreinit.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1200,7 +1200,7 @@ __PPCHalt
//__ghs_mtx_lock
//__ghs_mtx_unlock
__os_snprintf
//__tls_get_addr
__tls_get_addr
bspGetConsoleTypeRaw
bspGetEntityVersion
bspGetHardwareVersion
Expand Down
1 change: 1 addition & 0 deletions samples/cmake/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ add_subdirectory(helloworld)
add_subdirectory(helloworld_cpp)
add_subdirectory(my_first_rpl)
add_subdirectory(swkbd)
add_subdirectory(tls_test)

install(DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/content/"
DESTINATION "${CMAKE_INSTALL_PREFIX}/content")
11 changes: 11 additions & 0 deletions samples/cmake/tls_test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
cmake_minimum_required(VERSION 3.2)
project(tls_test CXX)
include("${DEVKITPRO}/wut/share/wut.cmake" REQUIRED)

add_executable(tls_test
main.cpp)

wut_create_rpx(tls_test)

install(FILES "${CMAKE_CURRENT_BINARY_DIR}/tls_test.rpx"
DESTINATION "${CMAKE_INSTALL_PREFIX}")
48 changes: 48 additions & 0 deletions samples/cmake/tls_test/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#include <coreinit/thread.h>
#include <coreinit/time.h>

#include <whb/proc.h>
#include <whb/log.h>
#include <whb/log_console.h>

#include <thread>

__thread
__attribute((tls_model("global-dynamic")))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required even with -ftls-model=global-dynamic ?

Most C++ code will just be using thread_local so it will be a shame if we cannot support unmodified code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sadly yes, I couldn't get gcc to use compatible relocations, even when compiling with -ftls-model=global-dynamic. it seems to ignore that flag unless specified directly as a variable attribute.

even when using -msdata=eabi, which clearely states that R2 is reservered for sdata2_base, gcc will just keep emitting code using R_PPC_TP* instead of R_PPC_DTP* relocations, expecting R2 to hold the Thread Pointer for whatever reason.

the only solution right now is to just #define thread_local as thead_local __attribute((tls_model("global-dynamic"))). it might even work as a compile flag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

tls_model ("tls_model")
The tls_model attribute sets thread-local storage model (see Thread-Local) of a particular __thread variable, overriding -ftls-model= command-line switch on a per-variable basis. The tls_model argument should be one of global-dynamic, local-dynamic, initial-exec or local-exec.

Not all targets support this attribute.

I suspect the -ftls-model=global-dynamic flag might not be passed properly to the compiler. Can you port this example over to standard Makefiles and see if it works that way?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did test it with a standard markefile and the result was the same. the flag does get passed, gcc just decided that it knows better and optimized it away.
from https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html

Alter the thread-local storage model to be used (see Thread-Local). The model argument should be one of ‘global-dynamic’, ‘local-dynamic’, ‘initial-exec’ or ‘local-exec’. Note that the choice is subject to optimization: the compiler may use a more efficient model for symbols not visible outside of the translation unit, or if -fpic is not given on the command line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, nice find. At this point I'm wondering if this requires us to patch the compiler to avoid this "optimization" - I believe that would be more preferable to needing to work around the TLS model every single time a TLS variable is used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would certainly be preferable, but it might be a bit harder than it looks.

the main problem here seems to be a bug in gcc that makes it emit code that contradicts the chosen ABI. as long as it thinks that R2 has a thread pointer, it will be very hard to convince it to drop the optimizations.

who knows, maybe it is even possible to make gcc restrict itself to R_PPC_DTPMOD32 and R_PPC_DTPREL32, which would make patching relocations in elf2rpl unnecessary.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, nice find. At this point I'm wondering if this requires us to patch the compiler to avoid this "optimization" - I believe that would be more preferable to needing to work around the TLS model every single time a TLS variable is used.

Adding this patch to the devkitPPC buildscripts forces gcc (14.2.0) to match the compiler flag instead of optimizing the TLS model:

diff --git a/gcc/varasm.cc b/gcc/varasm.cc
index b67a0b524db..33f59f60c7d 100644
--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -6838,7 +6838,7 @@ decl_default_tls_model (const_tree decl)
   if (kind < flag_tls_default)
     kind = flag_tls_default;
 
-  return kind;
+  return flag_tls_default;
 }

Not sure if there's a better spot for this info but it seems to be one of the last pieces this PR needed so figured here would make it visible enough

const char* thread_name = "";

int
thread1_entry(void)
{
thread_name = "Thread1";
WHBLogPrintf("Hello from %-20s (thread_name@0x%08X)\n", thread_name, &thread_name);
return 0;
}

int
main(int argc, char **argv)
{
WHBProcInit();
WHBLogConsoleInit();

thread_name = "Main Thread";
WHBLogPrintf("Hello from %-20s (thread_name@0x%08X)\n", thread_name, &thread_name);

std::thread th1 = std::thread(thread1_entry);
th1.join();

WHBLogPrintf("Hello from %-20s (thread_name@0x%08X)\n", thread_name, &thread_name);
WHBLogConsoleDraw();

while(WHBProcIsRunning()) {
OSSleepTicks(OSMillisecondsToTicks(1000));
}

WHBLogPrintf("Exiting... good bye.");
WHBLogConsoleDraw();
OSSleepTicks(OSMillisecondsToTicks(1000));

WHBLogConsoleFree();
WHBProcShutdown();
return 0;
}
23 changes: 18 additions & 5 deletions share/wut.ld
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ SECTIONS
*(.text.*)
*(.gnu.warning)
*(.gnu.linkonce.t.*)
*(.got)
*(.got1)
*(.got2)
*(.got.plt)
*(.plt)

KEEP (*(.fini))
} > codemem
Expand Down Expand Up @@ -56,11 +61,6 @@ SECTIONS
*(.fixup)
*(.gcc_except_table)
*(.gcc_except_table.*)
*(.got)
*(.got1)
*(.got2)
*(.got.plt)
*(.plt)
*(.tm_clone_table)
} > datamem

Expand Down Expand Up @@ -107,6 +107,19 @@ SECTIONS
} > datamem
__bss_end = .;

.thrdata : {
*(.tdata)
*(.tdata.*)
*(.gnu.linkonce.td.*)
} > datamem

.thrbss : {
*(.tbss)
*(.tbss.*)
*(.gnu.linkonce.tb.*)
*(.tcommon)
} > datamem

. = ORIGIN(relmem);
.rela.text ALIGN(4) : {
*(.rela.text)
Expand Down
2 changes: 1 addition & 1 deletion share/wut.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ DEPSEXT ?= d
# There's some default wut flags, they need to be added

# cpu type and hardware flags, keep all relocations (for elf2rpl), wut includes
WUT_CFLAGS := -mcpu=750 -meabi -mhard-float -isystem $(WUT_ROOT)/include
WUT_CFLAGS := -mcpu=750 -meabi -mhard-float -ftls-model=global-dynamic -isystem $(WUT_ROOT)/include
# Defines to tell code about this environment
WUT_CFLAGS += -D__WIIU__ -D__WUT__ -D__WUT_MAKEFILE__
# keep relocations, use wut linker script
Expand Down
2 changes: 1 addition & 1 deletion share/wut.specs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
%rename link old_link

*link:
%(old_link) -T %:getenv(DEVKITPRO /wut/share/wut.ld) --gc-sections --emit-relocs -z nocopyreloc %(wut_entry)
%(old_link) -T %:getenv(DEVKITPRO /wut/share/wut.ld) --gc-sections --emit-relocs -z nocopyreloc --no-tls-optimize %(wut_entry)
2 changes: 1 addition & 1 deletion share/wut.toolchain.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ set(CMAKE_LINKER "${DEVKITPPC_LD}" CACHE PATH "")
set(CMAKE_AR "${DEVKITPPC_AR}" CACHE PATH "")
set(CMAKE_STRIP "${DEVKITPPC_STRIP}" CACHE PATH "")

set(WUT_C_FLAGS "-mcpu=750 -meabi -mhard-float -Wl,-q -D__WIIU__ -D__WUT__")
set(WUT_C_FLAGS "-mcpu=750 -meabi -mhard-float -ftls-model=global-dynamic -Wl,-q -D__WIIU__ -D__WUT__")
set(CMAKE_C_FLAGS "${WUT_C_FLAGS}" CACHE STRING "")
set(CMAKE_CXX_FLAGS "${WUT_C_FLAGS}" CACHE STRING "")
set(CMAKE_ASM_FLAGS "${WUT_C_FLAGS}" CACHE STRING "")
Expand Down