On Mon, 2 May 2022, Martin Jambor wrote: > > Co-Authored-By: Alexander Monakov <amona...@gcc.gnu.org> > > (Minor nit and I don't care too much, but in GCC we traditionally > specify co-authors in the ChangeLog entry beginning by providing more > names, one per line. But perhaps we want to adapt more widely used > practices.)
I believe this is the recommended way to specify co-authors now (after Git migration) according to https://gcc.gnu.org/codingconventions.html (patch discussion below) > > --- 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? > 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. > But maybe Honza has some other clever idea. > > Also, please be careful not to unnecessarily commit trailing blank > spaces, the empty lines in your patch are not really empty. Yep, I can take care of whitespace issues. Thank you! Alexander
