LoongArch: Avoid out-of-bounds access in loongarch_symbol_insns
Checks
Commit Message
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(-)
Comments
在 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)
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.
在 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.
@@ -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)