[v2,1/2] RISC-V: Fix ICE caused by early ggc_free on DECL for RVV intrinsics in LTO.

gcc/ChangeLog:

	* config/riscv/riscv-vector-builtins.cc (function_builder::add_function):
	Check the final DECl to make sure it is valid.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/rvv/base/bug-10.c: New test.
---
 gcc/config/riscv/riscv-vector-builtins.cc       |  9 +++++++--
 .../gcc.target/riscv/rvv/base/bug-10.c          | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-10.c

On Tue, Sep 10, 2024 at 7:56 AM Jin Ma <jinma@linux.alibaba.com> wrote:
>
> gcc/ChangeLog:
>
>         * config/riscv/riscv-vector-builtins.cc (function_builder::add_function):
>         Check the final DECl to make sure it is valid.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/riscv/rvv/base/bug-10.c: New test.
> ---
>  gcc/config/riscv/riscv-vector-builtins.cc       |  9 +++++++--
>  .../gcc.target/riscv/rvv/base/bug-10.c          | 17 +++++++++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/bug-10.c
>
> diff --git a/gcc/config/riscv/riscv-vector-builtins.cc b/gcc/config/riscv/riscv-vector-builtins.cc
> index 41730c483ee1..0176670fbdf2 100644
> --- a/gcc/config/riscv/riscv-vector-builtins.cc
> +++ b/gcc/config/riscv/riscv-vector-builtins.cc
> @@ -79,7 +79,7 @@ public:
>    function_instance GTY ((skip)) instance;
>
>    /* The decl itself.  */
> -  tree GTY ((skip)) decl;
> +  tree decl;

While this looks obvious, ...

>    /* The overload hash of non-overloaded intrinsic is determined by
>       the overload name and argument list. Adding the overload name to
> @@ -3771,7 +3771,6 @@ function_builder::add_function (const function_instance &instance,
>  {
>    unsigned int code = vec_safe_length (registered_functions);
>    code = (code << RISCV_BUILTIN_SHIFT) + RISCV_BUILTIN_VECTOR;
> -
>    /* We need to be able to generate placeholders to ensure that we have a
>       consistent numbering scheme for function codes between the C and C++
>       frontends, so that everything ties up in LTO.
> @@ -3790,6 +3789,12 @@ function_builder::add_function (const function_instance &instance,
>                 : simulate_builtin_function_decl (input_location, name, fntype,
>                                                   code, NULL, attrs);
>
> +  /* If the code of DECL is ERROR_MARK or invalid code, usually "ggc_freed", we
> +     use integer_zero_node instead of it. This will be very helpful for the
> +     ggc_free.  */
> +  if (TREE_CODE (decl) == ERROR_MARK || TREE_CODE (decl) >= MAX_TREE_CODES)
> +    decl = integer_zero_node;
> +

... this one looks like a hack (note ggc_free only poisons memory when
checking is enabled).
I'm curious why you ever get a ggc_freed decl here.

>    registered_function &rfn = *ggc_alloc<registered_function> ();
>    rfn.instance = instance;
>    rfn.decl = decl;
> diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/bug-10.c b/gcc/testsuite/gcc.target/riscv/rvv/base/bug-10.c
> new file mode 100644
> index 000000000000..f685792a2c65
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/bug-10.c
> @@ -0,0 +1,17 @@
> +/* Test that we do not have ice when compile */
> +/* { dg-do link } */
> +/* { dg-options "-march=rv64gcv_zvfh -mabi=lp64d -O2 -flto"  { target { rv64 } } } */
> +/* { dg-options "-march=rv32gcv_zvfh -mabi=ilp32d -O2 -flto"  { target { rv32 } } } */
> +
> + #include <riscv_vector.h>
> +
> +int
> +main ()
> +{
> +  size_t vl = 8;
> +  vint32m1_t vs1 = {};
> +  vint32m1_t vs2 = {};
> +  vint32m1_t vd = __riscv_vadd_vv_i32m1(vs1, vs2, vl);
> +
> +  return *(int*)&vd;
> +}
> --
> 2.17.1
>

> > +  /* If the code of DECL is ERROR_MARK or invalid code, usually "ggc_freed", we
> > +     use integer_zero_node instead of it. This will be very helpful for the
> > +     ggc_free.  */
> > +  if (TREE_CODE (decl) == ERROR_MARK || TREE_CODE (decl) >= MAX_TREE_CODES)
> > +    decl = integer_zero_node;
> > +
> 
> ... this one looks like a hack (note ggc_free only poisons memory when
> checking is enabled).

Hi, I just built the compiler with "--enable-multilib" from riscv-gnu-toolchain source,
and it seems to have "CHECKING_P"/"ENABLE_EXTRA_CHECKING" on by default, resulting in
"flag_checking=2".

I haven't located it in detail, so I'm not sure if it is.

> I'm curious why you ever get a ggc_freed decl here.

It seems that the overloaded interface of RVV has been registered repeatedly, resulting
in invalid registrations except for the first registration, and these invalid registrations
have been ggc_freed. But anyway, I think it is necessary to do a check here. I think using
"integer_zero_node" is to meet the needs, although direct return would be better.

BR
Jin

On Tue, Sep 10, 2024 at 9:38 AM Jin Ma <jinma@linux.alibaba.com> wrote:
>
> > > +  /* If the code of DECL is ERROR_MARK or invalid code, usually "ggc_freed", we
> > > +     use integer_zero_node instead of it. This will be very helpful for the
> > > +     ggc_free.  */
> > > +  if (TREE_CODE (decl) == ERROR_MARK || TREE_CODE (decl) >= MAX_TREE_CODES)
> > > +    decl = integer_zero_node;
> > > +
> >
> > ... this one looks like a hack (note ggc_free only poisons memory when
> > checking is enabled).
>
> Hi, I just built the compiler with "--enable-multilib" from riscv-gnu-toolchain source,
> and it seems to have "CHECKING_P"/"ENABLE_EXTRA_CHECKING" on by default, resulting in
> "flag_checking=2".
>
> I haven't located it in detail, so I'm not sure if it is.

During development checking is enabled by default
(--enable-checking=yes), but once
released the compiler defaults to --enable-checking=release.

> > I'm curious why you ever get a ggc_freed decl here.
>
> It seems that the overloaded interface of RVV has been registered repeatedly, resulting
> in invalid registrations except for the first registration, and these invalid registrations
> have been ggc_freed. But anyway, I think it is necessary to do a check here. I think using
> "integer_zero_node" is to meet the needs, although direct return would be better.

But there isn't any way to check whether 'decl' has been freed ...
just make sure it isn't - you
should not even have a reference to it.

Richard.

> BR
> Jin

> > > I'm curious why you ever get a ggc_freed decl here.
> >
> > It seems that the overloaded interface of RVV has been registered repeatedly, resulting
> > in invalid registrations except for the first registration, and these invalid registrations
> > have been ggc_freed. But anyway, I think it is necessary to do a check here. I think using
> > "integer_zero_node" is to meet the needs, although direct return would be better.
> 
> But there isn't any way to check whether 'decl' has been freed ...
> just make sure it isn't - you
> should not even have a reference to it.
> Richard.

I'm trying to understand what you mean. You mean that directly using "integer_zero_node" to
overwrite decl will not guarantee whether the memory of the original decl has been properly
cleaned up, right?

If so, then the current method is really not appropriate. Maybe I should check whether
the function has been registered before registering the current function. If it has
been registered, I will skip it directly. This will lead to a decrease in efficiency.
I am not sure whether this is appropriate. In fact, I see a similar patch on aarch64:

https://github.com/gcc-mirror/gcc/commit/685d822e524cc8b2726ad6c44c2ccaabe55a198c

Or any other comments?

BR
Jin

On Wed, Sep 11, 2024 at 9:27 AM Jin Ma <jinma@linux.alibaba.com> wrote:
>
> > > > I'm curious why you ever get a ggc_freed decl here.
> > >
> > > It seems that the overloaded interface of RVV has been registered repeatedly, resulting
> > > in invalid registrations except for the first registration, and these invalid registrations
> > > have been ggc_freed. But anyway, I think it is necessary to do a check here. I think using
> > > "integer_zero_node" is to meet the needs, although direct return would be better.
> >
> > But there isn't any way to check whether 'decl' has been freed ...
> > just make sure it isn't - you
> > should not even have a reference to it.
> > Richard.
>
> I'm trying to understand what you mean. You mean that directly using "integer_zero_node" to
> overwrite decl will not guarantee whether the memory of the original decl has been properly
> cleaned up, right?

I'm saying that if you ever get a ggc_free()d object as argument to
this function that's
the thing to fix - somewhere there is a stale reference to that object
that either shouldn't
be there or that should have prevented the object from being ggc_free()d.

Richard.

>
> If so, then the current method is really not appropriate. Maybe I should check whether
> the function has been registered before registering the current function. If it has
> been registered, I will skip it directly. This will lead to a decrease in efficiency.
> I am not sure whether this is appropriate. In fact, I see a similar patch on aarch64:
>
> https://github.com/gcc-mirror/gcc/commit/685d822e524cc8b2726ad6c44c2ccaabe55a198c
>
> Or any other comments?
>
> BR
> Jin