On Thu, Mar 19, 2015 at 8:00 PM, Xinliang David Li <davi...@google.com> wrote: > ok -- then there is an over assertion in get_local_tls_init_fn. The > method can be called for aux module -- the decl created will also be > promoted.
No, it won't ever be called for an aux module. Both callsites are guarded (handle_tls_init has an early return at the top, and the callsite is guarded in get_tls_iniit_fn). > > Also can we rely on late promotion (special case of artificial > function __tls_init? This can avoid duplicated logic here. We don't need to promote __tls_init, but rather the init functions for each TLS variable (which are each aliased to __tls_init). I thought about both approaches, but promoting it as it was created seemed more straightforward. I can give that approach a try though if you prefer it (special casing DECL_ARTIFICIAL functions that start with _ZTH). Incidentally they are not marked static, so that would also need to be done to get them promoted. Teresa > > David > > On Thu, Mar 19, 2015 at 5:51 PM, Teresa Johnson <tejohn...@google.com> wrote: >> On Thu, Mar 19, 2015 at 4:45 PM, Xinliang David Li <davi...@google.com> >> wrote: >>> does generate_tls_wrapper also need to be suppressed for aux module? >> >> No, we generate the wrapper in the aux module and use it to access the >> TLS variable and invoke it's init function (which is defined in the >> variable's own module). Presumably we could generate that only in the >> original module and promote it if necessary, but generating it in the >> aux module does allow it to be inlined. This is essentially how the >> TLS accesses are handled for extern TLS variables defined elsewhere >> (wrapper generated in referring module). >> >> Teresa >> >>> >>> David >>> >>> On Thu, Mar 19, 2015 at 4:11 PM, Teresa Johnson <tejohn...@google.com> >>> wrote: >>>> New patch below. Passes regression tests plus internal application build. >>>> >>>> 2015-03-19 Teresa Johnson <tejohn...@google.com> >>>> >>>> gcc/ >>>> Google ref b/19618364. >>>> * cp/decl2.c (get_local_tls_init_fn): Assert on aux module. >>>> (get_tls_init_fn): Promote non-public init functions if necessary >>>> in LIPO mode, record new global at module scope. >>>> (get_tls_wrapper_fn): Record new global at module scope. >>>> (handle_tls_init): Skip in aux module, setup alias in exported >>>> primary module. >>>> >>>> testsuite/ >>>> Google ref b/19618364. >>>> * testsuite/g++.dg/tree-prof/lipo/tls.h: New test. >>>> * testsuite/g++.dg/tree-prof/lipo/tls2.h: Ditto. >>>> * testsuite/g++.dg/tree-prof/lipo/tls2_0.C: Ditto. >>>> * testsuite/g++.dg/tree-prof/lipo/tls2_1.C: Ditto. >>>> * testsuite/g++.dg/tree-prof/lipo/tls_0.C: Ditto. >>>> * testsuite/g++.dg/tree-prof/lipo/tls_1.C: Ditto. >>>> >>>> Index: cp/decl2.c >>>> =================================================================== >>>> --- cp/decl2.c (revision 221425) >>>> +++ cp/decl2.c (working copy) >>>> @@ -55,6 +55,7 @@ along with GCC; see the file COPYING3. If not see >>>> #include "langhooks.h" >>>> #include "c-family/c-ada-spec.h" >>>> #include "asan.h" >>>> +#include "gcov-io.h" >>>> >>>> extern cpp_reader *parse_in; >>>> >>>> @@ -3024,6 +3025,9 @@ var_needs_tls_wrapper (tree var) >>>> static tree >>>> get_local_tls_init_fn (void) >>>> { >>>> + /* In LIPO mode we should not generate local init functions for >>>> + the aux modules (see handle_tls_init). */ >>>> + gcc_assert (!L_IPO_IS_AUXILIARY_MODULE); >>>> tree sname = get_identifier ("__tls_init"); >>>> tree fn = IDENTIFIER_GLOBAL_VALUE (sname); >>>> if (!fn) >>>> @@ -3057,14 +3061,39 @@ get_tls_init_fn (tree var) >>>> if (!flag_extern_tls_init && DECL_EXTERNAL (var)) >>>> return NULL_TREE; >>>> >>>> + /* In LIPO mode we should not generate local init functions for >>>> + aux modules. The wrapper will reference the variable's init function >>>> + that is defined in its own primary module. Otherwise there is >>>> + a name conflict between the primary and aux __tls_init functions, >>>> + and difficulty ensuring each TLS variable is initialized exactly >>>> once. >>>> + Therefore, if this is an aux module or an exported primary module, we >>>> + need to ensure that the init function is available externally by >>>> + promoting it if it is not already public. This is similar to the >>>> + handling in promote_static_var_func, but we do it as the init >>>> function >>>> + is created to avoid the need to detect and properly promote this >>>> + artificial decl later on. */ >>>> + bool promote = L_IPO_IS_AUXILIARY_MODULE || >>>> + (L_IPO_COMP_MODE && PRIMARY_MODULE_EXPORTED); >>>> + >>>> #ifdef ASM_OUTPUT_DEF >>>> /* If the variable is internal, or if we can't generate aliases, >>>> - call the local init function directly. */ >>>> - if (!TREE_PUBLIC (var)) >>>> + call the local init function directly. Don't do this if we >>>> + are in LIPO mode an may need to export the init function. Note >>>> + that get_local_tls_init_fn will assert if it is called on an aux >>>> + module. */ >>>> + if (!TREE_PUBLIC (var) && !promote) >>>> #endif >>>> return get_local_tls_init_fn (); >>>> >>>> tree sname = mangle_tls_init_fn (var); >>>> + if (promote) >>>> + { >>>> + const char *name = IDENTIFIER_POINTER (sname); >>>> + char *assembler_name = (char*) alloca (strlen (name) + 30); >>>> + sprintf (assembler_name, "%s.cmo.%u", name, current_module_id); >>>> + sname = get_identifier (assembler_name); >>>> + } >>>> + >>>> tree fn = IDENTIFIER_GLOBAL_VALUE (sname); >>>> if (!fn) >>>> { >>>> @@ -3072,7 +3101,18 @@ get_tls_init_fn (tree var) >>>> build_function_type (void_type_node, >>>> void_list_node)); >>>> SET_DECL_LANGUAGE (fn, lang_c); >>>> - TREE_PUBLIC (fn) = TREE_PUBLIC (var); >>>> + if (promote) >>>> + { >>>> + TREE_PUBLIC (fn) = 1; >>>> + DECL_VISIBILITY (fn) = VISIBILITY_HIDDEN; >>>> + DECL_VISIBILITY_SPECIFIED (fn) = 1; >>>> + } >>>> + else >>>> + { >>>> + TREE_PUBLIC (fn) = TREE_PUBLIC (var); >>>> + DECL_VISIBILITY (fn) = DECL_VISIBILITY (var); >>>> + DECL_VISIBILITY_SPECIFIED (fn) = DECL_VISIBILITY_SPECIFIED >>>> (var); >>>> + } >>>> DECL_ARTIFICIAL (fn) = true; >>>> DECL_COMDAT (fn) = DECL_COMDAT (var); >>>> DECL_EXTERNAL (fn) = DECL_EXTERNAL (var); >>>> @@ -3091,8 +3131,6 @@ get_tls_init_fn (tree var) >>>> else >>>> DECL_WEAK (fn) = DECL_WEAK (var); >>>> } >>>> - DECL_VISIBILITY (fn) = DECL_VISIBILITY (var); >>>> - DECL_VISIBILITY_SPECIFIED (fn) = DECL_VISIBILITY_SPECIFIED (var); >>>> DECL_DLLIMPORT_P (fn) = DECL_DLLIMPORT_P (var); >>>> DECL_IGNORED_P (fn) = 1; >>>> mark_used (fn); >>>> @@ -3100,6 +3138,11 @@ get_tls_init_fn (tree var) >>>> DECL_BEFRIENDING_CLASSES (fn) = var; >>>> >>>> SET_IDENTIFIER_GLOBAL_VALUE (sname, fn); >>>> + /* In LIPO mode make sure we record the new global value so that it >>>> + is cleared before parsing the next aux module. */ >>>> + if (L_IPO_COMP_MODE && !is_parsing_done_p ()) >>>> + add_decl_to_current_module_scope (fn, >>>> + NAMESPACE_LEVEL >>>> (global_namespace)); >>>> } >>>> return fn; >>>> } >>>> @@ -3157,6 +3200,11 @@ get_tls_wrapper_fn (tree var) >>>> DECL_BEFRIENDING_CLASSES (fn) = var; >>>> >>>> SET_IDENTIFIER_GLOBAL_VALUE (sname, fn); >>>> + /* In LIPO mode make sure we record the new global value so that it >>>> + is cleared before parsing the next aux module. */ >>>> + if (L_IPO_COMP_MODE && !is_parsing_done_p ()) >>>> + add_decl_to_current_module_scope (fn, >>>> + NAMESPACE_LEVEL >>>> (global_namespace)); >>>> } >>>> return fn; >>>> } >>>> @@ -4179,6 +4227,14 @@ clear_decl_external (struct cgraph_node *node, voi >>>> static void >>>> handle_tls_init (void) >>>> { >>>> + /* In LIPO mode we should not generate local init functions for >>>> + aux modules. The wrapper will reference the variable's init function >>>> + that is defined in its own primary module. Otherwise there is >>>> + a name conflict between the primary and aux __tls_init functions, >>>> + and difficulty ensuring each TLS variable is initialized exactly >>>> once. */ >>>> + if (L_IPO_IS_AUXILIARY_MODULE) >>>> + return; >>>> + >>>> tree vars = prune_vars_needing_no_initialization (&tls_aggregates); >>>> if (vars == NULL_TREE) >>>> return; >>>> @@ -4213,8 +4269,12 @@ handle_tls_init (void) >>>> one_static_initialization_or_destruction (var, init, true); >>>> >>>> #ifdef ASM_OUTPUT_DEF >>>> - /* Output init aliases even with -fno-extern-tls-init. */ >>>> - if (TREE_PUBLIC (var)) >>>> + /* Output init aliases even with -fno-extern-tls-init. Also >>>> + export in LIPO mode if the primary module may be exported, >>>> + in which case the init function may be referenced externally >>>> + (see comments in get_tls_init_fn). */ >>>> + if (TREE_PUBLIC (var) || >>>> + (L_IPO_COMP_MODE && PRIMARY_MODULE_EXPORTED)) >>>> { >>>> tree single_init_fn = get_tls_init_fn (var); >>>> if (single_init_fn == NULL_TREE) >>>> Index: testsuite/g++.dg/tree-prof/lipo/tls.h >>>> =================================================================== >>>> --- testsuite/g++.dg/tree-prof/lipo/tls.h (revision 0) >>>> +++ testsuite/g++.dg/tree-prof/lipo/tls.h (working copy) >>>> @@ -0,0 +1,16 @@ >>>> +extern int NextId(); >>>> + >>>> +class TLSClass { >>>> + public: >>>> + TLSClass() { >>>> + id = NextId(); >>>> + bar = 1; >>>> + } >>>> + ~TLSClass() {} >>>> + int id; >>>> + int bar; >>>> +}; >>>> +extern TLSClass* NextTLSClass(); >>>> +extern void *SetTLSClass(TLSClass *a); >>>> +extern TLSClass *GetTLSClass(); >>>> +extern thread_local TLSClass* current_tls_; >>>> Index: testsuite/g++.dg/tree-prof/lipo/tls2.h >>>> =================================================================== >>>> --- testsuite/g++.dg/tree-prof/lipo/tls2.h (revision 0) >>>> +++ testsuite/g++.dg/tree-prof/lipo/tls2.h (working copy) >>>> @@ -0,0 +1,15 @@ >>>> +extern int NextId(); >>>> + >>>> +class TLSClass { >>>> + public: >>>> + TLSClass() { >>>> + id = NextId(); >>>> + bar = 1; >>>> + } >>>> + ~TLSClass() {} >>>> + int id; >>>> + int bar; >>>> +}; >>>> +extern TLSClass* NextTLSClass(); >>>> +extern void *SetTLSClass(TLSClass *a); >>>> +extern TLSClass *GetTLSClass(); >>>> Index: testsuite/g++.dg/tree-prof/lipo/tls2_0.C >>>> =================================================================== >>>> --- testsuite/g++.dg/tree-prof/lipo/tls2_0.C (revision 0) >>>> +++ testsuite/g++.dg/tree-prof/lipo/tls2_0.C (working copy) >>>> @@ -0,0 +1,10 @@ >>>> +// { dg-options "-std=c++11 -O2 --param=lipo-sampling-period=1" } >>>> +#include "tls2.h" >>>> + >>>> +static thread_local TLSClass* current_tls_ = NextTLSClass(); >>>> +void *SetTLSClass(TLSClass *a) { >>>> + current_tls_ = a; >>>> +} >>>> +TLSClass *GetTLSClass() { >>>> + return current_tls_; >>>> +} >>>> Index: testsuite/g++.dg/tree-prof/lipo/tls2_1.C >>>> =================================================================== >>>> --- testsuite/g++.dg/tree-prof/lipo/tls2_1.C (revision 0) >>>> +++ testsuite/g++.dg/tree-prof/lipo/tls2_1.C (working copy) >>>> @@ -0,0 +1,26 @@ >>>> +// { dg-options "-std=c++11 -O2 --param=lipo-sampling-period=1" } >>>> +#include <stdio.h> >>>> +#include <stdlib.h> >>>> +#include <new> >>>> +#include "tls2.h" >>>> +TLSClass* NextTLSClass() { >>>> + return new TLSClass(); >>>> +} >>>> +int NextId() { >>>> + static int id = 0; >>>> + return id++; >>>> +} >>>> +static thread_local TLSClass* current_tls2_ = NextTLSClass(); >>>> +void *SetTLSClass2(TLSClass *a) { >>>> + current_tls2_ = a; >>>> +} >>>> +int main() { >>>> + printf ("Id %d\n", GetTLSClass()->id); >>>> + TLSClass *A = NextTLSClass(); >>>> + SetTLSClass(A); >>>> + printf ("Id %d\n", GetTLSClass()->id); >>>> + printf ("Id %d\n", current_tls2_->id); >>>> + A = NextTLSClass(); >>>> + SetTLSClass2(A); >>>> + printf ("Id %d\n", current_tls2_->id); >>>> +} >>>> Index: testsuite/g++.dg/tree-prof/lipo/tls_0.C >>>> =================================================================== >>>> --- testsuite/g++.dg/tree-prof/lipo/tls_0.C (revision 0) >>>> +++ testsuite/g++.dg/tree-prof/lipo/tls_0.C (working copy) >>>> @@ -0,0 +1,10 @@ >>>> +// { dg-options "-std=c++11 -O2 --param=lipo-sampling-period=1" } >>>> +#include "tls.h" >>>> + >>>> +thread_local TLSClass* current_tls_ = NextTLSClass(); >>>> +void *SetTLSClass(TLSClass *a) { >>>> + current_tls_ = a; >>>> +} >>>> +TLSClass *GetTLSClass() { >>>> + return current_tls_; >>>> +} >>>> Index: testsuite/g++.dg/tree-prof/lipo/tls_1.C >>>> =================================================================== >>>> --- testsuite/g++.dg/tree-prof/lipo/tls_1.C (revision 0) >>>> +++ testsuite/g++.dg/tree-prof/lipo/tls_1.C (working copy) >>>> @@ -0,0 +1,32 @@ >>>> +// { dg-options "-std=c++11 -O2 --param=lipo-sampling-period=1" } >>>> +#include <stdio.h> >>>> +#include <stdlib.h> >>>> +#include <new> >>>> +#include "tls.h" >>>> +TLSClass* NextTLSClass() { >>>> + return new TLSClass(); >>>> +} >>>> +int NextId() { >>>> + static int id = 0; >>>> + return id++; >>>> +} >>>> +void *SetTLSClassHere(TLSClass *a) { >>>> + current_tls_ = a; >>>> +} >>>> +thread_local TLSClass* current_tls2_ = NextTLSClass(); >>>> +void *SetTLSClass2(TLSClass *a) { >>>> + current_tls2_ = a; >>>> +} >>>> +int main() { >>>> + printf ("Id %d\n", GetTLSClass()->id); >>>> + TLSClass *A = NextTLSClass(); >>>> + SetTLSClass(A); >>>> + printf ("Id %d\n", GetTLSClass()->id); >>>> + A = NextTLSClass(); >>>> + SetTLSClassHere(A); >>>> + printf ("Id %d\n", GetTLSClass()->id); >>>> + printf ("Id %d\n", current_tls2_->id); >>>> + A = NextTLSClass(); >>>> + SetTLSClass2(A); >>>> + printf ("Id %d\n", current_tls2_->id); >>>> +} >>>> >>>> On Thu, Mar 19, 2015 at 6:09 AM, Teresa Johnson <tejohn...@google.com> >>>> wrote: >>>>> On Wed, Mar 18, 2015 at 9:25 PM, Xinliang David Li <davi...@google.com> >>>>> wrote: >>>>>> get_local_tls_init_fn can be called from other contexts other than >>>>>> 'handle_tls_init' -- is the added new assertion safe ? >>>>> >>>>> In fact there is still an issue here, for file static TLS vars. Will >>>>> work on a fix and send the original test case (forgot to include in >>>>> first patch) and the new one with the fixed patch. >>>>> >>>>> Teresa >>>>> >>>>>> >>>>>> David >>>>>> >>>>>> On Wed, Mar 18, 2015 at 9:18 PM, Teresa Johnson <tejohn...@google.com> >>>>>> wrote: >>>>>>> >>>>>>> Ensure TLS variable wrapper and init functions are recorded at >>>>>>> the module scope upon creation so that they are cleared when >>>>>>> popping the module scope in between modules in LIPO mode. >>>>>>> >>>>>>> Also, do not generate a local TLS init function (__tls_init) >>>>>>> for aux functions, only in the primary modules. >>>>>>> >>>>>>> Passes regression tests. Ok for Google branches? >>>>>>> Teresa >>>>>>> >>>>>>> 2015-03-18 Teresa Johnson <tejohn...@google.com> >>>>>>> >>>>>>> Google ref b/19618364. >>>>>>> * cp/decl2.c (get_local_tls_init_fn): Assert on aux module. >>>>>>> (get_tls_init_fn): Record new global decl. >>>>>>> (get_tls_wrapper_fn): Ditto. >>>>>>> (handle_tls_init): Skip aux modules. >>>>>>> >>>>>>> Index: cp/decl2.c >>>>>>> =================================================================== >>>>>>> --- cp/decl2.c (revision 221425) >>>>>>> +++ cp/decl2.c (working copy) >>>>>>> @@ -3036,6 +3036,9 @@ get_local_tls_init_fn (void) >>>>>>> DECL_ARTIFICIAL (fn) = true; >>>>>>> mark_used (fn); >>>>>>> SET_IDENTIFIER_GLOBAL_VALUE (sname, fn); >>>>>>> + /* In LIPO mode we should not generate local init functions for >>>>>>> + the aux modules (see handle_tls_init). */ >>>>>>> + gcc_assert (!L_IPO_IS_AUXILIARY_MODULE); >>>>>>> } >>>>>>> return fn; >>>>>>> } >>>>>>> @@ -3100,6 +3103,11 @@ get_tls_init_fn (tree var) >>>>>>> DECL_BEFRIENDING_CLASSES (fn) = var; >>>>>>> >>>>>>> SET_IDENTIFIER_GLOBAL_VALUE (sname, fn); >>>>>>> + /* In LIPO mode make sure we record the new global value so that >>>>>>> it >>>>>>> + is cleared before parsing the next aux module. */ >>>>>>> + if (L_IPO_COMP_MODE && !is_parsing_done_p ()) >>>>>>> + add_decl_to_current_module_scope (fn, >>>>>>> + NAMESPACE_LEVEL >>>>>>> (global_namespace)); >>>>>>> } >>>>>>> return fn; >>>>>>> } >>>>>>> @@ -3157,6 +3165,11 @@ get_tls_wrapper_fn (tree var) >>>>>>> DECL_BEFRIENDING_CLASSES (fn) = var; >>>>>>> >>>>>>> SET_IDENTIFIER_GLOBAL_VALUE (sname, fn); >>>>>>> + /* In LIPO mode make sure we record the new global value so that >>>>>>> it >>>>>>> + is cleared before parsing the next aux module. */ >>>>>>> + if (L_IPO_COMP_MODE && !is_parsing_done_p ()) >>>>>>> + add_decl_to_current_module_scope (fn, >>>>>>> + NAMESPACE_LEVEL >>>>>>> (global_namespace)); >>>>>>> } >>>>>>> return fn; >>>>>>> } >>>>>>> @@ -4179,6 +4192,14 @@ clear_decl_external (struct cgraph_node *node, >>>>>>> voi >>>>>>> static void >>>>>>> handle_tls_init (void) >>>>>>> { >>>>>>> + /* In LIPO mode we should not generate local init functions for >>>>>>> + aux modules. The wrapper will reference the variable's init >>>>>>> function >>>>>>> + that is defined in its own primary module. Otherwise there is >>>>>>> + a name conflict between the primary and aux __tls_init functions, >>>>>>> + and difficulty ensuring each TLS variable is initialized exactly >>>>>>> once. */ >>>>>>> + if (L_IPO_IS_AUXILIARY_MODULE) >>>>>>> + return; >>>>>>> + >>>>>>> tree vars = prune_vars_needing_no_initialization (&tls_aggregates); >>>>>>> if (vars == NULL_TREE) >>>>>>> return; >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 >>>> >>>> >>>> >>>> -- >>>> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 >> >> >> >> -- >> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413