On Mon, 2 May 2022, Alexander Monakov wrote:
> > > --- a/gcc/ipa-visibility.cc
> > > +++ b/gcc/ipa-visibility.cc
> > > @@ -872,6 +872,22 @@ function_and_variable_visibility (bool whole_program)
> > > }
> > > }
> > > }
> > > + FOR_EACH_VARIABLE (vnode)
> > > + {
> > > + tree decl = vnode->decl;
> > > +
> > > + /* Optimize TLS model based on visibility (taking into account
> > > + optimizations done in the preceding loop), unless it was
> > > + specified explicitly. */
> > > +
> > > + if (DECL_THREAD_LOCAL_P (decl)
> > > + && !lookup_attribute ("tls_model", DECL_ATTRIBUTES (decl)))
> > > + {
> > > + enum tls_model new_model = decl_default_tls_model (decl);
> > > + gcc_checking_assert (new_model >= decl_tls_model (decl));
> > > + set_decl_tls_model (decl, new_model);
> > > + }
> > > + }
> > >
> >
> > decl_default_tls_model depends on the global optimize flag, which is
> > almost always problematic in IPA passes. I was able to make your patch
> > ICE using the vis-attr-hidden.c testcase from your patch with:
> >
> > mjambor@virgil:~/gcc/small/tests/tls$ ~/gcc/small/inst/bin/gcc -O2 -fPIC
> > -flto -c vis-attr-hidden.c
> > mjambor@virgil:~/gcc/small/tests/tls$ ~/gcc/small/inst/bin/gcc -fPIC -O0
> > -shared -flto vis-attr-hidden.o
> > during IPA pass: whole-program
> > lto1: internal compiler error: in function_and_variable_visibility, at
> > ipa-visibility.cc:888
> [snip]
> > Note the use of LTO, mismatching -O flags and the -shared flag in the
> > link step.
>
> Ah, right. The assert is checking that we don't accidentally downgrade decl's
> TLS access model, e.g. from local-dynamic to global-dynamic, and you've shown
> how to trigger that. I didn't realize this code can run twice, and with
> different 'optimize' levels.
>
> I would suggest to solve this by checking if the new TLS model is stronger,
> i.e. instead of this:
>
> gcc_checking_assert (new_model >= decl_tls_model (decl));
> set_decl_tls_model (decl, new_model);
>
> do this:
>
> if (new_model >= decl_tls_model (decl))
> set_decl_tls_model (decl, new_model);
>
> Does this look reasonable?
On second thought, it might be better to keep the assert, and place the loop
under 'if (optimize)'?
> > A simple but somewhat lame way to avoid the ICE would be to run your
> > loop over variables only from pass_ipa_function_and_variable_visibility
> > and not from pass_ipa_whole_program_visibility.
> >
> > I am afraid a real solution would involve copying relevant entries from
> > global_options to the symtab node representing the variable when it is
> > created/finalized, properly streaming them for LTO, and modifying
> > decl_default_tls_model to rely on those rather than global_options
> > itself.
>
> If we agree on the solution above, then this will not be necessary, after all
> this transformation looks at optimized whole-program visibility status,
> and so initial optimization level should not be relevant.
Alexander
