| Message ID | 32444455.vUGMIimiic@polaris |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR c++/85400 | expand |
On Thu, Apr 26, 2018 at 9:42 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: > Hi, > > for the attached testcase compiled/linked with -shared -fPIC -O, the Solaris > linker chokes on SPARC because the generated code uses the local-dynamic TLS > model for a global symbol: > > ld: fatal: relocation error: R_SPARC_TLS_LDM_HI22: file /var/tmp//ccKG6GQC.o: > symbol Test::blah(int)::mything: bound to: /var/tmp//ccKG6GQC.o: relocation > illegal when not bound to object being created > > The symbol is a local TLS symbol defined in an inline method of a class so it > is first local so decl_default_tls_model chooses TLS_MODEL_LOCAL_DYNAMIC. But > then it is "commonized" by the C++ front-end: > > /* If a local static variable is declared in an inline function, or if > we have a weak definition, we must endeavor to create only one > instance of the variable at link-time. */ > > which calls comdat_linkage, which in turn calls make_decl_one_only, which > makes it weak and public, so unsuitable for the local-dynamic TLS model. > > The proposed fix is to reset the TLS model after the commonization, but of > course only if no tls_model attribute was set on the variable, hence the fix > for handle_tls_model_attribute. Tested on x86-64/Linux, OK for mainline? > > > 2018-04-26 Eric Botcazou <ebotcazou@adacore.com> > > cp/ > PR c++/85400 > * decl.c (cp_finish_decl): Recompute the TLS model after commonizing > a TLS-local variable, unless it has got an explicit TLS model. It seems like we're likely to need the same thing when we get to make_decl_one_only by other paths; perhaps we should put this recalculation in a cxx_make_decl_one_only. Jason
> It seems like we're likely to need the same thing when we get to > make_decl_one_only by other paths; perhaps we should put this > recalculation in a cxx_make_decl_one_only. There are 4 calls to make_decl_one_only in the cp/ directory: the one at stake here in comdat_linkage, 1 in maybe_make_one_only, 1 in get_guard and 1 in get_tls_init_fn. The last 2 don't need the recalculation, especially the 3rd one which makes a copy of the TLS model. There are 3 calls to maybe_make_one_only in the cp/directory: 1 from start_preparsed_function, 1 from mark_decl_instantiated and 1 one from import_export_decl but guarded by DECL_FUNCTION_MEMBER_P. The 1st and 3rd don't need the recalculation. So it isn't clear to me if a cxx_make_decl_one_only is the way to go. Maybe doing the recalculation in comdat_linkage and maybe_make_one_only only would be sufficient.
> So it isn't clear to me if a cxx_make_decl_one_only is the way to go. Maybe > doing the recalculation in comdat_linkage and maybe_make_one_only only > would be sufficient. Patch to that effect attached, tested on x86-64/Linux, OK for mainline? 2018-05-09 Eric Botcazou <ebotcazou@adacore.com> cp/ PR c++/85400 * decl2.c (adjust_var_decl_tls_model): New static function. (comdat_linkage): Call it on a variable. (maybe_make_one_only): Likewise. c-family/ * c-attribs.c (handle_visibility_attribute): Do not set no_add_attrs.
OK. On Wed, May 9, 2018 at 6:05 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> So it isn't clear to me if a cxx_make_decl_one_only is the way to go. Maybe >> doing the recalculation in comdat_linkage and maybe_make_one_only only >> would be sufficient. > > Patch to that effect attached, tested on x86-64/Linux, OK for mainline? > > > 2018-05-09 Eric Botcazou <ebotcazou@adacore.com> > > cp/ > PR c++/85400 > * decl2.c (adjust_var_decl_tls_model): New static function. > (comdat_linkage): Call it on a variable. > (maybe_make_one_only): Likewise. > > c-family/ > * c-attribs.c (handle_visibility_attribute): Do not set no_add_attrs. > > -- > Eric Botcazou
On Wed, May 09, 2018 at 12:05:34PM +0200, Eric Botcazou wrote: > > So it isn't clear to me if a cxx_make_decl_one_only is the way to go. Maybe > > doing the recalculation in comdat_linkage and maybe_make_one_only only > > would be sufficient. > > Patch to that effect attached, tested on x86-64/Linux, OK for mainline? > > > 2018-05-09 Eric Botcazou <ebotcazou@adacore.com> > > cp/ > PR c++/85400 > * decl2.c (adjust_var_decl_tls_model): New static function. > (comdat_linkage): Call it on a variable. > (maybe_make_one_only): Likewise. > > c-family/ > * c-attribs.c (handle_visibility_attribute): Do not set no_add_attrs. Eric, do you plan to backport this to release branches? > Index: cp/decl2.c > =================================================================== > --- cp/decl2.c (revision 259642) > +++ cp/decl2.c (working copy) > @@ -1838,6 +1838,17 @@ mark_vtable_entries (tree decl) > } > } > > +/* Adjust the TLS model on variable DECL if need be, typically after > + the linkage of DECL has been modified. */ > + > +static void > +adjust_var_decl_tls_model (tree decl) > +{ > + if (CP_DECL_THREAD_LOCAL_P (decl) > + && !lookup_attribute ("tls_model", DECL_ATTRIBUTES (decl))) > + set_decl_tls_model (decl, decl_default_tls_model (decl)); > +} > + > /* Set DECL up to have the closest approximation of "initialized common" > linkage available. */ > > @@ -1888,6 +1899,9 @@ comdat_linkage (tree decl) > > if (TREE_PUBLIC (decl)) > DECL_COMDAT (decl) = 1; > + > + if (VAR_P (decl)) > + adjust_var_decl_tls_model (decl); > } > > /* For win32 we also want to put explicit instantiations in > @@ -1926,6 +1940,8 @@ maybe_make_one_only (tree decl) > /* Mark it needed so we don't forget to emit it. */ > node->forced_by_abi = true; > TREE_USED (decl) = 1; > + > + adjust_var_decl_tls_model (decl); > } > } > } > Index: c-family/c-attribs.c > =================================================================== > --- c-family/c-attribs.c (revision 259642) > +++ c-family/c-attribs.c (working copy) > @@ -2299,14 +2299,13 @@ handle_visibility_attribute (tree *node, > > static tree > handle_tls_model_attribute (tree *node, tree name, tree args, > - int ARG_UNUSED (flags), bool *no_add_attrs) > + int ARG_UNUSED (flags), > + bool *ARG_UNUSED (no_add_attrs)) > { > tree id; > tree decl = *node; > enum tls_model kind; > > - *no_add_attrs = true; > - > if (!VAR_P (decl) || !DECL_THREAD_LOCAL_P (decl)) > { > warning (OPT_Wattributes, "%qE attribute ignored", name); Jakub
> Eric, do you plan to backport this to release branches?
I don't think so, it isn't a regression and the workaround is trivial.
Index: cp/decl.c =================================================================== --- cp/decl.c (revision 259642) +++ cp/decl.c (working copy) @@ -7216,9 +7216,14 @@ cp_finish_decl (tree decl, tree init, bo { layout_var_decl (decl); maybe_commonize_var (decl); + + /* This needs to be adjusted after the linkage is set. */ + if (CP_DECL_THREAD_LOCAL_P (decl) + && !lookup_attribute ("tls_model", DECL_ATTRIBUTES (decl))) + set_decl_tls_model (decl, decl_default_tls_model (decl)); } - /* This needs to happen after the linkage is set. */ + /* This needs to happen after the linkage is set. */ determine_visibility (decl); if (var_definition_p && TREE_STATIC (decl)) Index: c-family/c-attribs.c =================================================================== --- c-family/c-attribs.c (revision 259642) +++ c-family/c-attribs.c (working copy) @@ -2299,14 +2299,13 @@ handle_visibility_attribute (tree *node, static tree handle_tls_model_attribute (tree *node, tree name, tree args, - int ARG_UNUSED (flags), bool *no_add_attrs) + int ARG_UNUSED (flags), + bool *ARG_UNUSED (no_add_attrs)) { tree id; tree decl = *node; enum tls_model kind; - *no_add_attrs = true; - if (!VAR_P (decl) || !DECL_THREAD_LOCAL_P (decl)) { warning (OPT_Wattributes, "%qE attribute ignored", name);