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

[Xi Ruoyao <xry111@xry111.site>]

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