| Message ID | 20240202095628.3242-1-xry111@xry111.site |
|---|---|
| State | New |
| Headers | show |
| Series | LoongArch: Avoid out-of-bounds access in loongarch_symbol_insns | expand |
在 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.
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)
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(-)