Re: [PATCH] [PR83782] i386 PIE: avoid @GOTOFF for ifuncs and their aliases

[Fangrui Song <maskray@google.com>]

On Mon, Aug 1, 2022 at 12:05 PM H.J. Lu via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Thu, Jul 28, 2022 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 1:26 AM Alexandre Oliva <oliva@adacore.com> wrote:
> > >
> > > On Jul 27, 2022, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> > >
> > > > On Tue, Jul 26, 2022 at 10:14 PM Alexandre Oliva <oliva@adacore.com> wrote:
> > >
> > > >> The use of @GOTOFF for locally-bound but externally-visible symbols
> > > >> (e.g. protected visibility) also breaks pointer identity if the
> > > >> canonical address ends up preempted by a PLT entry.
> > >
> > > > Here is a different fix:
> > >
> > > > https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598667.html
> > >
> > > Oh, thanks, I'd missed that.
> > >
> > > It doesn't seem to fix the part of the problem I quoted above, though.
> > > I think fixing that requires testing the visibility, to make sure the
> > > symbol's canonical address cannot be preempted, which may occur with
> > > local binding, if the symbol is protected and referenced in the main
> > > program, otherwise pointer identity is broken again, admittedly for a
> > > more obscure case, but pointer identity was the point of the PR.
> >
> > The protected symbol issue isn't IFUNC specific.   The protected
> > symbol handling is changed in glibc 2.36 and binutils 2.39.   Only
> > the address of the protected symbol definition should be used as
> > its address.
> >
> > > >> * config/i386/i386.cc (ix86_call_use_plt_p): Follow the alias
> > > >> chain looking for an ifunc, as in gcc.target/i386/mvc10.c.
> > >
> > > You may also need to do something like this bit for mvc10.c on ia32 PIE.
> > > Because the ifunc is called through an alias, AFAICT we don't even
> > > notice that the call target is (an alias to) an ifunc.  GCC's
> > > gotoff_operand predicate accepts it, but binutils (the linker, IIRC)
> > > then rejects that reference, because the named symbol is an alias to an
> > > ifunc.
> >
> > Yes, this change is needed.
>
> I think this fix should be applied to default_binds_local_p_3:
>
>   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp))
>       || (!targetm.ifunc_ref_local_ok ()
>           && TREE_CODE (exp) == FUNCTION_DECL
>           && cgraph_node::get (exp)
>           && cgraph_node::get (exp)->ifunc_resolver))
>     return false;
>
> since the ifunc_resolver check won't work on aliases.
>
> --
> H.J.

A note if people are going to refactor default_binds_local_p_3 or add
a new default_binds_local_p_* variant (unrelated to the ifunc
problem):
extern_protected_data == true is now legacy. For new variants we want
all extern_protected_data == false.