Re: ipa-visibility TLC 2/n

[Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE>]

Jan Hubicka <hubicka@ucw.cz> writes:

>> Honza,
>> 
>> I am not sure that the problem is caused only by aliases and thunks.
>> The large increase in AIX linker warnings about branches not followed
>> by nop also worry me.
>> 
>> Your patch was about visibility. How does the more aggressive
>
> ipa-visibility is a pass that basically bring external symbols local whenever
> it can (i.e. does privatizatoin), it is not that much about ELF visibilities.
>
>> algorithm behave on a platform that does not support visibility? Is it
>> defaulting to hidden? If the new algorithm is being too aggressive and
>> incorrectly converting calls from global to local, it could cause
>> serious problems for AIX because the GOT register will not be
>> restored.
>
> One of optimizations IPA visibility does is that it looks for global symbols
> that
>   1) are believed by target to be overwritable (interposed - I will probably
>      update name here) by linker to a different definition (we have
>      decl_binds_to_current_def_p that is target tweakable), and
>   2) the interposition may not change semantics of the symbol (i.e. function
>      body must be the same or variable initializer must match)
>
>      For some symbols (such as inline functions, virtual tables, readonly
>      variables with no address taken) we know it won't.
>   3) the symbol's definition can not be optimized away by linker
>      (by symtab_can_be_discarded)
> If all conditons match, it creates a static alias (not hidden, just local to
> the .o file) and redirects users of the symbol to the alias.  This should be
> always win: we know that the representation of symbol will survive to final
> binary (it is not discarded) and we replace expensive references through GOT by
> cheap references to local symbol.
>
> We did, for longer time, redirect calls.  The troublesome patch makes us to
> redirect also references in virtual tables and newly we also consider virtual
> tables themselves for aliases.
>
> This should be a win, since virtual tables tends to be startup time hogs and
> it is common to have virtual tables in one DSO to refer to comdats that are
> shared with other DSO, but because they must be the same, we can just ignore
> the sharing.
>
> On AIX we observed interesting series of events.
>
>  1) First the output machinery was not quite able to declare local static
>     alias for a symbol (this was about year ago when I introduced the first
>     change).
>  2) AIX assembler seems to issue warning when jump happens to the local
>     static alias confusing it with the global symbol it is aliasing.
>     I do not know if the warning is just bogus or we output something
>     incorrectly.  This is the reason for NOP warning as I understand.
>  3) Important difference we is that in AIX all COMDAT symbols are considered
>     non-discardable. This makes us to produce a lot more aliases than on ELF
>     system.
>
>     I am not sure if this is acurate and AIX linker really has no means
>     of removing duplicated bodies of COMDAT functions/initializers of variables.
>     If it has, we need to model it in symtab_can_be_discarded.
>     Currently we test whether symbol is in comdat group and in the case of
>     AIX it isn't. 
>
>     As disucssed earlier, I am thinking about making symtab_can_be_discarded
>     return true also for implicit sections (I have WIP patch for this to
>     commit today). Earlier version of the patch however did not solve
>     the warnings.
>
>     I also tested libstdc++.so sizes with current mainline, with the
>     local aliases disabled and with this change and current mainline
>     wins. Suggesting that perhaps there is really no way to discard
>     duplicated comdats or libstdc++ doesn't really have them.
>     Insight here would be welcome - I am sure it is easy to test if 
>     including and using inline __noinline__ function in two units
>     leads to two copies of that beast or not. It would be nice to know
>     how native toolchain handles it and if GCC does the same trick.
>  4) Before 4.9 we hit bug in inliner dealing with aliases that gave me
>     a headache. It reproduced on AIX only because of 3)
>  5) We hit problem with aliases to anchored sections, hopefully solved now
>  6) We hit the problem that AIX assembler silently accepts but miscompile
>     when alias is declared before its target.  THis is also hopefully
>     fixed now.  We hit it twice - one for normal symbols about year ago
>     and now again for thunks.
>  7) Given number of issues I ended up writting a verifier that checks
>     sanity of sections, aliases and comdat groups.  I check
>       a) all symbols in comdat group have the same section
>       b) alias and its target have same comdat group & section characteristics
>          (obivously one can not place alias out of comdat)
>       c) I check sanity of IMPLICIT_SECTION flag.
>     It turns out that C++ FE makes complete mess of those breaking all three
>     rules due to complex interactions in between comdat code, same body aliases
>     and the way thunks are produced.  I think this may confuse AIX output
>     macros since they are just given a contradictionary information.
>     So it may turn out that the AIX assembler warnings are correct.
>
>     Have patch in testing and it seems that warnings are gone, but I am not
>     at the end of bootstrap, yet, now resolving issues with libfortran.
>
>     I plan to add check that we do not have two comdat groups of the same
>     name, but I am not there, yet.  As usual, verifier improvements show
>     issues that are a lot of fun to resolve.
>
> Still, i would not think of the original optimization as aggressive and risky one.
> We just found that the way we deal with aliases is terribly broken at some targets.
> Pretty much all the bugs above are reproducable by a testcases using alias attribute.
> Yes, i understand it is frustrating (to me too) and that I should be faster dealing
> with the issue (teaching in Canada is time consuming). 
>
> I am now again testing similar changes on AIX before comitting.  I considered
> the original TLC patch as simple change given that we already debugged the
> local aliases for 4.9.  I am sorry for that.  I will try to be sure that I get libstdc++
> testsuite into a shape either by comitting the patch for 7) or reverting the relevant
> part of the TLC patch.

Unfortunately, AIX isn't the only target massively affected by your
recent patches.  This all started with r210597

2014-05-17  Jan Hubicka  <hubicka@ucw.cz>

	* tree-pass.h (make_pass_ipa_comdats): New pass.
        * timevar.def (TV_IPA_COMDATS): New timevar.
        * passes.def (pass_ipa_comdats): Add.
        * Makefile.in (OBJS): Add ipa-comdats.o
        * ipa-comdats.c: New file.

At that time, only Solaris 11 with gas/Solaris ld was affected: many Go
tests started failing like this:

runtime.SetFinalizer: cannot pass *     os      os.file to finalizer func(*     os      os.file) error
fatal error: runtime.SetFinalizer

Neither Solaris 10 (which doesn't enable comdat group due to missing ld
support for what gas emits) nor Solaris 11 with /bin/as (which has
different comdat syntax, thus emitting groups differently) were
affected.

I didn't file a PR at that time since I had started to work with the
Solaris linker engineers to see what could be done about this on the ld
side.  Unfortunately, even if this worked, it won't change the installed
base.

Since then, the number of Go testsuite failures seems to increase daily,
now Solaris 10 is also affected.  Given that sparc-sun-solaris2.10 is a
primary platform, we need to do something about this.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University