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

add support for Thread local Storage. #155

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

Conversation

aliaspider
Copy link

requires updated elf2rpl : devkitPro/wut-tools#6

Copy link
Contributor

@exjam exjam left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for looking at this.

I want to play around with this and check the codegen and stuff before I merge it but probably won't have time until the weekend to do that.

#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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants