Re: ipa-visibility TLC 2/n

[Jan Hubicka <hubicka at ucw dot cz>]

> 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.

Honza