[PATCH] LoongArch: Avoid out-of-bounds access in loongarch_symbol_insns

[PATCH] LoongArch: Avoid out-of-bounds access in loongarch_symbol_insns

[Xi Ruoyao]

We call loongarch_symbol_insns with mode = MAX_MACHINE_MODE sometimes.
But in loongarch_symbol_insns:

    if (LSX_SUPPORTED_MODE_P (mode) || LASX_SUPPORTED_MODE_P (mode))
      return 0;

And LSX_SUPPORTED_MODE_P is defined as:

    #define LSX_SUPPORTED_MODE_P(MODE) \
      (ISA_HAS_LSX \
       && GET_MODE_SIZE (MODE) == UNITS_PER_LSX_REG ... ...

GET_MODE_SIZE is expanded to a call to mode_to_bytes, which is defined:

    ALWAYS_INLINE poly_uint16
    mode_to_bytes (machine_mode mode)
    {
    #if GCC_VERSION >= 4001
      return (__builtin_constant_p (mode)
	  ? mode_size_inline (mode) : mode_size[mode]);
    #else
      return mode_size[mode];
    #endif
    }

There is an assertion in mode_size_inline:

    gcc_assert (mode >= 0 && mode < NUM_MACHINE_MODES);

Note that NUM_MACHINE_MODES = MAX_MACHINE_MODE (emitted by genmodes.cc),
thus if __builtin_constant_p (mode) is evaluated true (it happens when
GCC is bootstrapped with LTO+PGO), the assertion will be triggered and
cause an ICE.  OTOH if __builtin_constant_p (mode) is evaluated false,
mode_size[mode] is still an out-of-bound array access (the length or the
mode_size array is NUM_MACHINE_MODES).

So we shouldn't call LSX_SUPPORTED_MODE_P or LASX_SUPPORTED_MODE_P with
MAX_MACHINE_MODE in loongarch_symbol_insns.  This is very similar to a
MIPS bug PR98491 fixed by me about 3 years ago.

gcc/ChangeLog:

	* config/loongarch/loongarch.cc (loongarch_symbol_insns): Do not
	use LSX_SUPPORTED_MODE_P or LASX_SUPPORTED_MODE_P if mode is
	MAX_MACHINE_MODE.
---

Bootstrapped and regtested on loongarch64-linux-gnu.  Ok for trunk?

 gcc/config/loongarch/loongarch.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/config/loongarch/loongarch.cc b/gcc/config/loongarch/loongarch.cc
index 963e86d61af..6badef45d62 100644
--- a/gcc/config/loongarch/loongarch.cc
+++ b/gcc/config/loongarch/loongarch.cc
@@ -2007,7 +2007,8 @@ loongarch_symbol_insns (enum loongarch_symbol_type type, machine_mode mode)
 {
   /* LSX LD.* and ST.* cannot support loading symbols via an immediate
      operand.  */
-  if (LSX_SUPPORTED_MODE_P (mode) || LASX_SUPPORTED_MODE_P (mode))
+  if (mode != MAX_MACHINE_MODE
+      && (LSX_SUPPORTED_MODE_P (mode) || LASX_SUPPORTED_MODE_P (mode)))
     return 0;
 
   switch (type)
-- 
2.43.0

Re: [PATCH] LoongArch: Avoid out-of-bounds access in loongarch_symbol_insns

[chenglulu]

在 2024/2/2 下午5:55, Xi Ruoyao 写道:
> We call loongarch_symbol_insns with mode = MAX_MACHINE_MODE sometimes.
> But in loongarch_symbol_insns:
>
>      if (LSX_SUPPORTED_MODE_P (mode) || LASX_SUPPORTED_MODE_P (mode))
>        return 0;
>
> And LSX_SUPPORTED_MODE_P is defined as:
>
>      #define LSX_SUPPORTED_MODE_P(MODE) \
>        (ISA_HAS_LSX \
>         && GET_MODE_SIZE (MODE) == UNITS_PER_LSX_REG ... ...
>
> GET_MODE_SIZE is expanded to a call to mode_to_bytes, which is defined:
>
>      ALWAYS_INLINE poly_uint16
>      mode_to_bytes (machine_mode mode)
>      {
>      #if GCC_VERSION >= 4001
>        return (__builtin_constant_p (mode)
> 	  ? mode_size_inline (mode) : mode_size[mode]);
>      #else
>        return mode_size[mode];
>      #endif
>      }
>
> There is an assertion in mode_size_inline:
>
>      gcc_assert (mode >= 0 && mode < NUM_MACHINE_MODES);
>
> Note that NUM_MACHINE_MODES = MAX_MACHINE_MODE (emitted by genmodes.cc),
> thus if __builtin_constant_p (mode) is evaluated true (it happens when
> GCC is bootstrapped with LTO+PGO), the assertion will be triggered and
> cause an ICE.  OTOH if __builtin_constant_p (mode) is evaluated false,
> mode_size[mode] is still an out-of-bound array access (the length or the
> mode_size array is NUM_MACHINE_MODES).
>
> So we shouldn't call LSX_SUPPORTED_MODE_P or LASX_SUPPORTED_MODE_P with
> MAX_MACHINE_MODE in loongarch_symbol_insns.  This is very similar to a
> MIPS bug PR98491 fixed by me about 3 years ago.
>
> gcc/ChangeLog:
>
> 	* config/loongarch/loongarch.cc (loongarch_symbol_insns): Do not
> 	use LSX_SUPPORTED_MODE_P or LASX_SUPPORTED_MODE_P if mode is
> 	MAX_MACHINE_MODE.
> ---
>
> Bootstrapped and regtested on loongarch64-linux-gnu.  Ok for trunk?

LGTM!

I have a question. I see that you often add compilation options in 
BOOT_CFLAGS.

I also want to test it. Do you have a recommended set of compilation 
options?

Thanks!

>
>   gcc/config/loongarch/loongarch.cc | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/loongarch/loongarch.cc b/gcc/config/loongarch/loongarch.cc
> index 963e86d61af..6badef45d62 100644
> --- a/gcc/config/loongarch/loongarch.cc
> +++ b/gcc/config/loongarch/loongarch.cc
> @@ -2007,7 +2007,8 @@ loongarch_symbol_insns (enum loongarch_symbol_type type, machine_mode mode)
>   {
>     /* LSX LD.* and ST.* cannot support loading symbols via an immediate
>        operand.  */
> -  if (LSX_SUPPORTED_MODE_P (mode) || LASX_SUPPORTED_MODE_P (mode))
> +  if (mode != MAX_MACHINE_MODE
> +      && (LSX_SUPPORTED_MODE_P (mode) || LASX_SUPPORTED_MODE_P (mode)))
>       return 0;
>   
>     switch (type)

Pushed: [PATCH] LoongArch: Avoid out-of-bounds access in loongarch_symbol_insns

[Xi Ruoyao]

On Sun, 2024-02-04 at 11:19 +0800, chenglulu wrote:
> 
> 在 2024/2/2 下午5:55, Xi Ruoyao 写道:
> > We call loongarch_symbol_insns with mode = MAX_MACHINE_MODE sometimes.
> > But in loongarch_symbol_insns:
> > 
> >      if (LSX_SUPPORTED_MODE_P (mode) || LASX_SUPPORTED_MODE_P (mode))
> >        return 0;
> > 
> > And LSX_SUPPORTED_MODE_P is defined as:
> > 
> >      #define LSX_SUPPORTED_MODE_P(MODE) \
> >        (ISA_HAS_LSX \
> >         && GET_MODE_SIZE (MODE) == UNITS_PER_LSX_REG ... ...
> > 
> > GET_MODE_SIZE is expanded to a call to mode_to_bytes, which is defined:
> > 
> >      ALWAYS_INLINE poly_uint16
> >      mode_to_bytes (machine_mode mode)
> >      {
> >      #if GCC_VERSION >= 4001
> >        return (__builtin_constant_p (mode)
> > 	  ? mode_size_inline (mode) : mode_size[mode]);
> >      #else
> >        return mode_size[mode];
> >      #endif
> >      }
> > 
> > There is an assertion in mode_size_inline:
> > 
> >      gcc_assert (mode >= 0 && mode < NUM_MACHINE_MODES);
> > 
> > Note that NUM_MACHINE_MODES = MAX_MACHINE_MODE (emitted by genmodes.cc),
> > thus if __builtin_constant_p (mode) is evaluated true (it happens when
> > GCC is bootstrapped with LTO+PGO), the assertion will be triggered and
> > cause an ICE.  OTOH if __builtin_constant_p (mode) is evaluated false,
> > mode_size[mode] is still an out-of-bound array access (the length or the
> > mode_size array is NUM_MACHINE_MODES).
> > 
> > So we shouldn't call LSX_SUPPORTED_MODE_P or LASX_SUPPORTED_MODE_P with
> > MAX_MACHINE_MODE in loongarch_symbol_insns.  This is very similar to a
> > MIPS bug PR98491 fixed by me about 3 years ago.
> > 
> > gcc/ChangeLog:
> > 
> > 	* config/loongarch/loongarch.cc (loongarch_symbol_insns): Do not
> > 	use LSX_SUPPORTED_MODE_P or LASX_SUPPORTED_MODE_P if mode is
> > 	MAX_MACHINE_MODE.
> > ---
> > 
> > Bootstrapped and regtested on loongarch64-linux-gnu.  Ok for trunk?
> 
> LGTM!

Pushed r14-8785.

> I have a question. I see that you often add compilation options in 
> BOOT_CFLAGS.
> 
> I also want to test it. Do you have a recommended set of compilation 
> options?

When I build a compiler for my system I use
{BOOT_{C,CXX,LD}FLAGS,{C,CXX,LD}FLAGS_FOR_TARGET}="-O3 -march=la664 -
mtune=la664 -pipe -fgraphite-identity -floop-nest-optimize -fipa-pta -
fdevirtualize-at-ltrans -fno-semantic-interposition -Wl,-O1 -Wl,--as-
needed"

and enable PGO (make profiledbootstrap) and LTO (--with-build-
config=bootstrap-lto).

All of them but GRAPHITE (-fgraphite-identity -floop-nest-optimize)
seems "pretty safe" on the architectures I have a hardware of.  GRAPHITE
is causing bootstrap failure on AArch64 with GCC 13 (PR109929) if
combined with PGO and the real cause is still not found yet.

But when I do a test build I normally only enable the flags which may
help to catch some issues, for example when a change only affects LTO I
add --with-build-config=bootstrap-lto, when changing something related
to LASX I use -O3 -mlasx (or -O3 -march=la664) as BOOT_CFLAGS.


-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: Pushed: [PATCH] LoongArch: Avoid out-of-bounds access in loongarch_symbol_insns

[chenglulu]

在 2024/2/5 上午1:01, Xi Ruoyao 写道:
> I have a question. I see that you often add compilation options in
>> BOOT_CFLAGS.
>>
>> I also want to test it. Do you have a recommended set of compilation
>> options?
> When I build a compiler for my system I use
> {BOOT_{C,CXX,LD}FLAGS,{C,CXX,LD}FLAGS_FOR_TARGET}="-O3 -march=la664 -
> mtune=la664 -pipe -fgraphite-identity -floop-nest-optimize -fipa-pta -
> fdevirtualize-at-ltrans -fno-semantic-interposition -Wl,-O1 -Wl,--as-
> needed"
>
> and enable PGO (make profiledbootstrap) and LTO (--with-build-
> config=bootstrap-lto).
>
> All of them but GRAPHITE (-fgraphite-identity -floop-nest-optimize)
> seems "pretty safe" on the architectures I have a hardware of.  GRAPHITE
> is causing bootstrap failure on AArch64 with GCC 13 (PR109929) if
> combined with PGO and the real cause is still not found yet.
>
> But when I do a test build I normally only enable the flags which may
> help to catch some issues, for example when a change only affects LTO I
> add --with-build-config=bootstrap-lto, when changing something related
> to LASX I use -O3 -mlasx (or -O3 -march=la664) as BOOT_CFLAGS.
>
>
Thank you so much. I will try to add optimization options.