On 2019-12-18 14:19 +0100, Jan Hubicka wrote:
> > ICE here.
> >
> > lto1: internal compiler error: tree check: expected identifier_node, have
> > function_decl in ultimate_transparent_alias_target, at varasm.c:1308
> > 0x6f9cfe tree_check_failed(tree_node const*, char const*, int, char const*,
> > ...)
> > ../../gcc/gcc/tree.c:9685
> > 0x714541 tree_check(tree_node*, char const*, int, char const*, tree_code)
> > ../../gcc/gcc/tree.h:3273
> > 0x714541 ultimate_transparent_alias_target
> > ../../gcc/gcc/varasm.c:1308
> > 0x714541 do_assemble_symver(tree_node*, tree_node*)
> > ../../gcc/gcc/varasm.c:5971
>
> Interesting that it works for me, but indeed we can remove that call
> since there is no way to do weakref of symbol version.
> > > #ifdef ASM_OUTPUT_SYMVER_DIRECTIVE
> > > - ASM_OUTPUT_SYMVER_DIRECTIVE (asm_out_file,
> > > - IDENTIFIER_POINTER (target),
> > > - IDENTIFIER_POINTER (id));
> > > + if (TREE_PUBLIC (target) && DECL_VISIBILITY (target) ==
> > > VISIBILITY_DEFAULT)
> > > + ASM_OUTPUT_SYMVER_DIRECTIVE (asm_out_file,
> > > + IDENTIFIER_POINTER
> > > + (DECL_ASSEMBLER_NAME (target)),
> > > + IDENTIFIER_POINTER (id));
> > > + else
> > > + {
> > > + int nameend;
> > > + for (nameend = 0; IDENTIFIER_POINTER (id)[nameend] != '@';
> > > nameend++)
> > > + ;
> > > + if (IDENTIFIER_POINTER (id)[nameend + 1] != '@'
> > > + || IDENTIFIER_POINTER (id)[nameend + 2] == '@')
> > > + {
> > > + sorry_at (DECL_SOURCE_LOCATION (target),
> > > + "can not produce %<symver%> of a symbol that is "
> > > + "not exported with default visibility");
> > > + return;
> >
> > I think this does not make sense. Some library authors may export "foo@VER_
> > 1"
> > but not "foo_v1" to ensure the programmers using the library upgrade their
> > code
> > to use new "correct" ABI, instead of an old one. This error makes it
> > impossible.
> >
> > (Try to comment out "foo_v1" in version.map, in the testcase.)
>
> The problem here is that we lie to the compiler (by pretending that
> foo_v2 is exported from DSO while it is not) and force user to do the
> same.
>
> We support two ways to hide symbol - either at compile time via
> attribute((visibility("hidden"))) or at link-time via map file. The
> first produces better code because compiler can do more optimizations
> knowing that the symbol can not be interposed.
>
> Generally we want users to use visiblity attribute or -fvisibility since
> it leads to better code. However now we tell users to use
> attribute((symver("..."))) to produce symbol version, but at the same
> time not use attribute((visibility("hidden"))).
Could a symver symbol be interposed? I'll do some test to see.
> > > + memcpy (buf, IDENTIFIER_POINTER (id), nameend + 2);
> > > + buf[nameend + 2] = '@';
> > > + strcpy (buf + nameend + 3, IDENTIFIER_POINTER (id) + nameend + 2);
> >
> > We can't replace a single "@" with "@@@". So I think producing .LSYMVERx is
> > not
> > an option for "old" versions like "foo@VER_1".
>
> I wonder why gas implements the .symver this way at first place. Does
> the linker really need the global symbol foo_v1 to produce the
> version (in addition to foo@VER_1 that is in symbol table as well)?
I don't think the global symbol foo_v1 is necessary. But I can't find a way
telling gas to make foo@VER_1 global and foo_v1 local.
> > > + /* Symbol versions are always used externally, but linker does not
> > > + report that correctly. */
> > > + else if (snode->symver && *res == LDPR_PREVAILING_DEF_IRONLY)
> > > + snode->resolution = LDPR_PREVAILING_DEF_IRONLY_EXP;
> >
> > This is absolutely correct.
>
> Good, I will go ahead with filling in binutils PR on the wrong LDPR
> value and apply the hack.
> > > else
> > > snode->resolution = *res;
> > > }
> >
> > I still believe we should consider symver targets to be externally visible
> > in
> > cgraph_externally_visible_p. There is a comment saying "if linker counts on
> > us,
> > we must preserve the function". That's true in our case.
> >
> > And, I think
> >
> > .globl .LSYMVER0
> > .set .LSYMVER0, foo_v2
> > .symver .LSYMVER0, foo@@VERS_2
> I produce
> .symver .LSYMVER0, foo@@@VERS_2
>
> > is exactly same as
> >
> > .globl foo_v2
> > .symver foo_v2, foo@@VERS_2
> >
> > except there is an unnecessary ".LSYMVER0". Adding ".globl foo_v2" or
> > ".globl
> > foo_v1" won't cause them to be "global" in the final DSO because the linker
> > will
> > hide them according to the version script.
>
> The difference is that in first case compiler can fully control foo_v2
> symbol (with LTO it will turn it into static symbol, it will inline
> calls to it and do other things), while in the second case we need to
> treat foo_v2 specially.
> > So if it's safe we can force a ".globl foo_v2" before ".symver foo_v2,
> > foo@@VERS_2". But I can't prove it's safe so I think it's better to
> > consider
> > this case in cgraph_externally_visible_p.
>
> It sort of makes things work, but for example it will prevent gcc from
> inlining calls to foo_v2. I think we will still need to do something
> about -fvisibility=hidden.
>
> It is sad that we do not have way to produce symbol version without a
> corresponding symbol of global visiblity. If we had we could support
> multiple symver aliases from one symbol and avoid the need to explicitly
> hide the unnecesary symbols in the map files...
Explicitly hiding the unnecessary symbols in map files is now how we handle this
[with __asm__(".symver foo_v2, foo@@VERS_2")]. We can continue to do in this
way and leave it as an enhancement.
--
Xi Ruoyao <xry...@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University
