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

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

[Alexandre Oliva]

g++.dg/ext/attr-ifunc-3.C and gcc.target/i386/mvc10.c, not changed,
have made it clear that there were problems in the optimizations to
use @GOTOFF to refer to locally-bound ifuncs.  GNU ld as recently as
May 2018 would reject such constructs, whereas later versions will
silently accept but generate incorrect PIE with them (attr-ifunc-3.C)
or still reject them if referenced through aliases (mvc10.c).  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.  This patch
modifies the local_symbolic_operand predicate to disable @GOTOFF for
locally-bound symbols that would require @PLT for calls, restoring
earlier behavior and disabling the optimization that has proven
problematic even on amd64.  Eventually we may reintroduce the
optimization, when the linker is fixed and we test for the fix before
enabling it, and we exclude symbols whose canonical addresses may be
preempted even when the symbol definition can't.  pr83782 tests have
been adjusted to expect @GOT instead of @GOTOFF.

Regstrapped on x86_64-linux-gnu; also tested, along with other patches
I'm posting today with "i386 PIE" in the subject, and compared
default-PIE and default-nonPIE results on it, and on i686-linux-gnu.  Ok
to install?


for  gcc/ChangeLog

	PR target/83782
	* config/i386/predicates.md (local_symbolic_operand): Disable
	GOTOFF even for locally-bound ifuncs.
	* config/i386/i386.cc (ix86_call_use_plt_p): Follow the alias
	chain looking for an ifunc, as in gcc.target/i386/mvc10.c.

for  gcc/testsuite/ChangeLog

	PR target/83782
	* gcc.target/i386/pr83782-1.c: Adjust to require GOT rather
	than GOTOFF on ia32.
	* gcc.target/i386/pr83782-2.c: Likewise.
---
 gcc/config/i386/i386.cc                   |   16 ++++++++++------
 gcc/config/i386/predicates.md             |    4 +++-
 gcc/testsuite/gcc.target/i386/pr83782-1.c |    4 ++--
 gcc/testsuite/gcc.target/i386/pr83782-2.c |    4 ++--
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
index aab28da4b5d4b..5c5dc8d2373ff 100644
--- a/gcc/config/i386/i386.cc
+++ b/gcc/config/i386/i386.cc
@@ -16058,13 +16058,17 @@ ix86_call_use_plt_p (rtx call_op)
     {
       if (SYMBOL_REF_DECL (call_op)
 	  && TREE_CODE (SYMBOL_REF_DECL (call_op)) == FUNCTION_DECL)
-	{
-	  /* NB: All ifunc functions must be called via PLT.  */
-	  cgraph_node *node
-	    = cgraph_node::get (SYMBOL_REF_DECL (call_op));
-	  if (node && node->ifunc_resolver)
+	/* NB: All ifunc functions must be called via PLT, and we have
+	   to explicitly iterate over an alias chain looking for a
+	   node marked as an ifunc(_resolver) to tell.  That node is
+	   itself aliased to the actual resolver function, so
+	   ultimate_alias_target would skip the marker, and the call
+	   may be to another declaration aliased to the ifunc.  */
+	for (cgraph_node *node
+	       = cgraph_node::get (SYMBOL_REF_DECL (call_op));
+	     node && node->alias; node = node->get_alias_target ())
+	  if (node->ifunc_resolver)
 	    return true;
-	}
       return false;
     }
   return true;
diff --git a/gcc/config/i386/predicates.md b/gcc/config/i386/predicates.md
index 42053ea7209f6..411c06e22e600 100644
--- a/gcc/config/i386/predicates.md
+++ b/gcc/config/i386/predicates.md
@@ -596,7 +596,9 @@ (define_predicate "local_symbolic_operand"
   if (TARGET_DLLIMPORT_DECL_ATTRIBUTES && SYMBOL_REF_DLLIMPORT_P (op))
     return false;
   if (SYMBOL_REF_LOCAL_P (op))
-    return true;
+    /* ifuncname@GOTOFF was rejected by the x86 linker before May
+       2018, and silently generated wrong code for PIE afterwards.  */
+    return !ix86_call_use_plt_p (op);
 
   /* There is, however, a not insubstantial body of code in the rest of
      the compiler that assumes it can just stick the results of
diff --git a/gcc/testsuite/gcc.target/i386/pr83782-1.c b/gcc/testsuite/gcc.target/i386/pr83782-1.c
index ce97b12e65d58..af52278ec4df2 100644
--- a/gcc/testsuite/gcc.target/i386/pr83782-1.c
+++ b/gcc/testsuite/gcc.target/i386/pr83782-1.c
@@ -20,7 +20,7 @@ bar(void)
   return foo;
 }
 
-/* { dg-final { scan-assembler {leal[ \t]foo@GOTOFF\(%[^,]*\),[ \t]%eax} { target ia32 } } } */
+/* { dg-final { scan-assembler-not {leal[ \t]foo@GOTOFF\(%[^,]*\),[ \t]%eax} { target ia32 } } } */
 /* { dg-final { scan-assembler {lea(?:l|q)[ \t]foo\(%rip\),[ \t]%(?:e|r)ax} { target { ! ia32 } } } } */
-/* { dg-final { scan-assembler-not "foo@GOT\\\(" { target ia32 } } } */
+/* { dg-final { scan-assembler "foo@GOT\\\(" { target ia32 } } } */
 /* { dg-final { scan-assembler-not "foo@GOTPCREL\\\(" { target { ! ia32 } } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr83782-2.c b/gcc/testsuite/gcc.target/i386/pr83782-2.c
index e25d258bbda43..15c7dc04e88c3 100644
--- a/gcc/testsuite/gcc.target/i386/pr83782-2.c
+++ b/gcc/testsuite/gcc.target/i386/pr83782-2.c
@@ -20,7 +20,7 @@ bar(void)
   return foo;
 }
 
-/* { dg-final { scan-assembler {leal[ \t]foo@GOTOFF\(%[^,]*\),[ \t]%eax} { target ia32 } } } */
+/* { dg-final { scan-assembler-not {leal[ \t]foo@GOTOFF\(%[^,]*\),[ \t]%eax} { target ia32 } } } */
 /* { dg-final { scan-assembler {lea(?:l|q)[ \t]foo\(%rip\),[ \t]%(?:e|r)ax} { target { ! ia32 } } } } */
-/* { dg-final { scan-assembler-not "foo@GOT\\\(" { target ia32 } } } */
+/* { dg-final { scan-assembler "foo@GOT\\\(" { target ia32 } } } */
 /* { dg-final { scan-assembler-not "foo@GOTPCREL\\\(" { target { ! ia32 } } } } */

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

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

[H.J. Lu]

On Tue, Jul 26, 2022 at 10:14 PM Alexandre Oliva <oliva@adacore.com> wrote:
>
>
> g++.dg/ext/attr-ifunc-3.C and gcc.target/i386/mvc10.c, not changed,
> have made it clear that there were problems in the optimizations to
> use @GOTOFF to refer to locally-bound ifuncs.  GNU ld as recently as
> May 2018 would reject such constructs, whereas later versions will
> silently accept but generate incorrect PIE with them (attr-ifunc-3.C)
> or still reject them if referenced through aliases (mvc10.c).  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.  This patch
> modifies the local_symbolic_operand predicate to disable @GOTOFF for
> locally-bound symbols that would require @PLT for calls, restoring
> earlier behavior and disabling the optimization that has proven
> problematic even on amd64.  Eventually we may reintroduce the
> optimization, when the linker is fixed and we test for the fix before
> enabling it, and we exclude symbols whose canonical addresses may be
> preempted even when the symbol definition can't.  pr83782 tests have
> been adjusted to expect @GOT instead of @GOTOFF.
>
> Regstrapped on x86_64-linux-gnu; also tested, along with other patches
> I'm posting today with "i386 PIE" in the subject, and compared
> default-PIE and default-nonPIE results on it, and on i686-linux-gnu.  Ok
> to install?

Here is a different fix:

https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598667.html

Use PLT doesn't mean that it can't be treated as local.  The problem
on ia32 is that PIC won't be set up properly for indirect call.    There is
no problem on x86-64 and non-PIC on ia32.

>
> for  gcc/ChangeLog
>
>         PR target/83782
>         * config/i386/predicates.md (local_symbolic_operand): Disable
>         GOTOFF even for locally-bound ifuncs.
>         * config/i386/i386.cc (ix86_call_use_plt_p): Follow the alias
>         chain looking for an ifunc, as in gcc.target/i386/mvc10.c.
>
> for  gcc/testsuite/ChangeLog
>
>         PR target/83782
>         * gcc.target/i386/pr83782-1.c: Adjust to require GOT rather
>         than GOTOFF on ia32.
>         * gcc.target/i386/pr83782-2.c: Likewise.
> ---
>  gcc/config/i386/i386.cc                   |   16 ++++++++++------
>  gcc/config/i386/predicates.md             |    4 +++-
>  gcc/testsuite/gcc.target/i386/pr83782-1.c |    4 ++--
>  gcc/testsuite/gcc.target/i386/pr83782-2.c |    4 ++--
>  4 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> index aab28da4b5d4b..5c5dc8d2373ff 100644
> --- a/gcc/config/i386/i386.cc
> +++ b/gcc/config/i386/i386.cc
> @@ -16058,13 +16058,17 @@ ix86_call_use_plt_p (rtx call_op)
>      {
>        if (SYMBOL_REF_DECL (call_op)
>           && TREE_CODE (SYMBOL_REF_DECL (call_op)) == FUNCTION_DECL)
> -       {
> -         /* NB: All ifunc functions must be called via PLT.  */
> -         cgraph_node *node
> -           = cgraph_node::get (SYMBOL_REF_DECL (call_op));
> -         if (node && node->ifunc_resolver)
> +       /* NB: All ifunc functions must be called via PLT, and we have
> +          to explicitly iterate over an alias chain looking for a
> +          node marked as an ifunc(_resolver) to tell.  That node is
> +          itself aliased to the actual resolver function, so
> +          ultimate_alias_target would skip the marker, and the call
> +          may be to another declaration aliased to the ifunc.  */
> +       for (cgraph_node *node
> +              = cgraph_node::get (SYMBOL_REF_DECL (call_op));
> +            node && node->alias; node = node->get_alias_target ())
> +         if (node->ifunc_resolver)
>             return true;
> -       }
>        return false;
>      }
>    return true;
> diff --git a/gcc/config/i386/predicates.md b/gcc/config/i386/predicates.md
> index 42053ea7209f6..411c06e22e600 100644
> --- a/gcc/config/i386/predicates.md
> +++ b/gcc/config/i386/predicates.md
> @@ -596,7 +596,9 @@ (define_predicate "local_symbolic_operand"
>    if (TARGET_DLLIMPORT_DECL_ATTRIBUTES && SYMBOL_REF_DLLIMPORT_P (op))
>      return false;
>    if (SYMBOL_REF_LOCAL_P (op))
> -    return true;
> +    /* ifuncname@GOTOFF was rejected by the x86 linker before May
> +       2018, and silently generated wrong code for PIE afterwards.  */
> +    return !ix86_call_use_plt_p (op);
>
>    /* There is, however, a not insubstantial body of code in the rest of
>       the compiler that assumes it can just stick the results of
> diff --git a/gcc/testsuite/gcc.target/i386/pr83782-1.c b/gcc/testsuite/gcc.target/i386/pr83782-1.c
> index ce97b12e65d58..af52278ec4df2 100644
> --- a/gcc/testsuite/gcc.target/i386/pr83782-1.c
> +++ b/gcc/testsuite/gcc.target/i386/pr83782-1.c
> @@ -20,7 +20,7 @@ bar(void)
>    return foo;
>  }
>
> -/* { dg-final { scan-assembler {leal[ \t]foo@GOTOFF\(%[^,]*\),[ \t]%eax} { target ia32 } } } */
> +/* { dg-final { scan-assembler-not {leal[ \t]foo@GOTOFF\(%[^,]*\),[ \t]%eax} { target ia32 } } } */
>  /* { dg-final { scan-assembler {lea(?:l|q)[ \t]foo\(%rip\),[ \t]%(?:e|r)ax} { target { ! ia32 } } } } */
> -/* { dg-final { scan-assembler-not "foo@GOT\\\(" { target ia32 } } } */
> +/* { dg-final { scan-assembler "foo@GOT\\\(" { target ia32 } } } */
>  /* { dg-final { scan-assembler-not "foo@GOTPCREL\\\(" { target { ! ia32 } } } } */
> diff --git a/gcc/testsuite/gcc.target/i386/pr83782-2.c b/gcc/testsuite/gcc.target/i386/pr83782-2.c
> index e25d258bbda43..15c7dc04e88c3 100644
> --- a/gcc/testsuite/gcc.target/i386/pr83782-2.c
> +++ b/gcc/testsuite/gcc.target/i386/pr83782-2.c
> @@ -20,7 +20,7 @@ bar(void)
>    return foo;
>  }
>
> -/* { dg-final { scan-assembler {leal[ \t]foo@GOTOFF\(%[^,]*\),[ \t]%eax} { target ia32 } } } */
> +/* { dg-final { scan-assembler-not {leal[ \t]foo@GOTOFF\(%[^,]*\),[ \t]%eax} { target ia32 } } } */
>  /* { dg-final { scan-assembler {lea(?:l|q)[ \t]foo\(%rip\),[ \t]%(?:e|r)ax} { target { ! ia32 } } } } */
> -/* { dg-final { scan-assembler-not "foo@GOT\\\(" { target ia32 } } } */
> +/* { dg-final { scan-assembler "foo@GOT\\\(" { target ia32 } } } */
>  /* { dg-final { scan-assembler-not "foo@GOTPCREL\\\(" { target { ! ia32 } } } } */
>
> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>



-- 
H.J.

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

[Alexandre Oliva]

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.

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

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

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

[H.J. Lu]

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.

>
> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>



-- 
H.J.

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

[H.J. Lu]

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.

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

[Fangrui Song]

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.

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

[Alexandre Oliva]

On Aug  1, 2022, "H.J. Lu" <hjl.tools@gmail.com> wrote:

> On Thu, Jul 28, 2022 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:

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

I was concerned that other tests might require such alias pointer
chasing, and figured we might be better off propagating the flag setting
to aliases.


[PR83782] ifunc: back-propagate ifunc_resolver to aliases

gcc.target/i386/mvc10.c fails with -fPIE on ia32 because we omit the
@PLT mark when calling an alias to an indirect function.  Such aliases
aren't marked as ifunc_resolvers in the cgraph, so the test that would
have forced the PLT call fails.

I've arranged for ifunc_resolver to be back-propagated to aliases, and
relaxed the test that required the ifunc attribute to be attached to
directly the decl, rather than taken from an aliased decl, when the
ifunc_resolver bit is set.

Regstrapped on x86_64-linux-gnu, also tested mvc10.c with -m32 -fPIE.
Ok to install?


for  gcc/ChangeLog

	PR target/83782
	* cgraph.h (symtab_node::set_ifunc_resolver): New, overloaded.
	Back-propagate flag to aliases.
	* cgraph.cc (cgraph_node::create): Use set_ifunc_resolver.
	(cgraph_node::create_alias): Likewise.
	* lto-cgraph.cc (input_node): Likewise.
	* multiple_target.cc (create_dispatcher_calls): Propagate to
	aliases when redirecting them.
	* symtab.cc (symtab_node::verify_base): Accept ifunc_resolver
	set in an alias to another ifunc_resolver nodes.
	(symtab_node::resolve_alias): Propagate ifunc_resolver from
	resolved target to alias.
	* varasm.cc (do_assemble_alias): Checking for the attribute.
---
 gcc/cgraph.cc          |    4 ++--
 gcc/cgraph.h           |   13 +++++++++++++
 gcc/lto-cgraph.cc      |    2 +-
 gcc/multiple_target.cc |    2 ++
 gcc/symtab.cc          |   15 ++++++++++++++-
 gcc/varasm.cc          |    5 ++++-
 6 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
index 8d6ed38efa25d..699f2c20defa4 100644
--- a/gcc/cgraph.cc
+++ b/gcc/cgraph.cc
@@ -518,7 +518,7 @@ cgraph_node::create (tree decl)
     }
 
   if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
-    node->ifunc_resolver = true;
+    node->set_ifunc_resolver ();
 
   node->register_symbol ();
   maybe_record_nested_function (node);
@@ -576,7 +576,7 @@ cgraph_node::create_alias (tree alias, tree target)
   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (alias)) != NULL)
     alias_node->transparent_alias = alias_node->weakref = true;
   if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias)))
-    alias_node->ifunc_resolver = true;
+    alias_node->set_ifunc_resolver ();
   return alias_node;
 }
 
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 4be67e3cea906..9468b8a4e3662 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -467,6 +467,19 @@ public:
     return decl->decl_with_vis.symtab_node;
   }
 
+  /* Worked for the nonstatic set_ifunc_resolver, to vback-propagate
+     ifunc_resolver in the alias chain.  */
+  static bool set_ifunc_resolver (symtab_node *n, void * = NULL)
+  {
+    n->ifunc_resolver = true;
+    return false;
+  }
+
+  /* Set the ifunc_resolver bit in this node and in any aliases thereof.  */
+  void set_ifunc_resolver () {
+    call_for_symbol_and_aliases (set_ifunc_resolver, NULL, true);
+  }
+
   /* Try to find a symtab node for declaration DECL and if it does not
      exist or if it corresponds to an inline clone, create a new one.  */
   static inline symtab_node * get_create (tree node);
diff --git a/gcc/lto-cgraph.cc b/gcc/lto-cgraph.cc
index 6d9c36ea8b67f..3d8ade1f9f042 100644
--- a/gcc/lto-cgraph.cc
+++ b/gcc/lto-cgraph.cc
@@ -1280,7 +1280,7 @@ input_node (struct lto_file_decl_data *file_data,
       node = symtab->create_empty ();
       node->decl = fn_decl;
       if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (fn_decl)))
-	node->ifunc_resolver = 1;
+	node->set_ifunc_resolver ();
       node->register_symbol ();
     }
 
diff --git a/gcc/multiple_target.cc b/gcc/multiple_target.cc
index 3e2d26882c8e9..97801a4cc9ea9 100644
--- a/gcc/multiple_target.cc
+++ b/gcc/multiple_target.cc
@@ -160,6 +160,8 @@ create_dispatcher_calls (struct cgraph_node *node)
 	      source->create_reference (inode, IPA_REF_ALIAS);
 	      if (inode->get_comdat_group ())
 		source->add_to_same_comdat_group (inode);
+	      if (!source->ifunc_resolver)
+		source->set_ifunc_resolver ();
 	    }
 	  else
 	    gcc_unreachable ();
diff --git a/gcc/symtab.cc b/gcc/symtab.cc
index f2d96c0268bf3..ea34afe07f5fc 100644
--- a/gcc/symtab.cc
+++ b/gcc/symtab.cc
@@ -1107,9 +1107,19 @@ symtab_node::verify_base (void)
           error ("function symbol is not function");
           error_found = true;
 	}
+      /* If the ifunc attribute is present, the node must be marked as
+	 ifunc_resolver, but it may also be marked on a node that
+	 doesn't have the attribute, if it's an alias to another
+	 marked node.  The resolver node itself is an alias to the
+	 function that performs the resolution proper, and that
+	 function is not marked, but here we test other kinds of
+	 aliases, that alias the indirect function.  */
       else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
 		!= NULL)
-	       != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
+	       ? !ifunc_resolver
+	       : ifunc_resolver
+	       ? !get_alias_target ()->ifunc_resolver
+	       : (alias && analyzed && get_alias_target ()->ifunc_resolver))
 	{
 	  error ("inconsistent %<ifunc%> attribute");
           error_found = true;
@@ -1877,6 +1887,9 @@ symtab_node::resolve_alias (symtab_node *target, bool transparent)
   if (target->implicit_section)
     call_for_symbol_and_aliases (set_implicit_section, NULL, true);
 
+  if (target->ifunc_resolver && !ifunc_resolver)
+    set_ifunc_resolver ();
+
   /* Alias targets become redundant after alias is resolved into an reference.
      We do not want to keep it around or we would have to mind updating them
      when renaming symbols.  */
diff --git a/gcc/varasm.cc b/gcc/varasm.cc
index 4db8506b106b6..56bf3ce6c23dc 100644
--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -6217,7 +6217,10 @@ do_assemble_alias (tree decl, tree target)
       maybe_assemble_visibility (decl);
     }
   if (TREE_CODE (decl) == FUNCTION_DECL
-      && cgraph_node::get (decl)->ifunc_resolver)
+      && cgraph_node::get (decl)->ifunc_resolver
+      /* Aliases to the ifunc decl will also have the ifunc_resolver
+	 bit set, so check that this is the ifunc declaration.  */
+      && lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
     {
 #if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
       if (targetm.has_ifunc_p ())


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>