* [PATCH v1] RISC-V: Fix out of range memory access when lto mode init @ 2023-06-19 8:07 pan2.li 2023-06-19 8:16 ` Li, Pan2 ` (2 more replies) 0 siblings, 3 replies; 33+ messages in thread From: pan2.li @ 2023-06-19 8:07 UTC (permalink / raw) To: gcc-patches Cc: juzhe.zhong, rdapp.gcc, jeffreyalaw, pan2.li, yanzhang.wang, kito.cheng From: Pan Li <pan2.li@intel.com> We extend the machine mode from 8 to 16 bits already. But there still one placing missing from the tree-streamer. It has one hard coded array for the machine code like size 256. In the lto pass, we memset the array by MAX_MACHINE_MODE count but the value of the MAX_MACHINE_MODE will grow as more and more modes are added. While the machine mode array in tree-streamer still leave 256 as is. Then, when the MAX_MACHINE_MODE is greater than 256, the memset of lto_output_init_mode_table will touch the memory out of range unexpected. This patch would like to take the MAX_MACHINE_MODE as the size of the array in tree-streamer, to make sure there is no potential unexpected memory access in future. Signed-off-by: Pan Li <pan2.li@intel.com> gcc/ChangeLog: * tree-streamer.cc (streamer_mode_table): Use MAX_MACHINE_MODE as array size. * tree-streamer.h (streamer_mode_table): Ditto. --- gcc/tree-streamer.cc | 2 +- gcc/tree-streamer.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc index ed65a7692e3..a28ef9c7920 100644 --- a/gcc/tree-streamer.cc +++ b/gcc/tree-streamer.cc @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see During streaming in, we translate the on the disk mode using this table. For normal LTO it is set to identity, for ACCEL_COMPILER depending on the mode_table content. */ -unsigned char streamer_mode_table[1 << 8]; +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; /* Check that all the TS_* structures handled by the streamer_write_* and streamer_read_* routines are exactly ALL the structures defined in diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index 170d61cf20b..51a292c8d80 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); void streamer_write_integer_cst (struct output_block *, tree); /* In tree-streamer.cc. */ -extern unsigned char streamer_mode_table[1 << 8]; +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; void streamer_check_handled_ts_structures (void); bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, hashval_t, unsigned *); -- 2.34.1 ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init 2023-06-19 8:07 [PATCH v1] RISC-V: Fix out of range memory access when lto mode init pan2.li @ 2023-06-19 8:16 ` Li, Pan2 2023-06-19 8:40 ` Richard Biener 2023-06-19 9:05 ` [PATCH] RISC-V: Fix out of range memory access of machine mode table pan2.li 2023-06-21 7:58 ` [PATCH v3] Streamer: Fix out of range memory access of machine mode pan2.li 2 siblings, 1 reply; 33+ messages in thread From: Li, Pan2 @ 2023-06-19 8:16 UTC (permalink / raw) To: gcc-patches Cc: juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, Richard Biener Add Richard Biener for reviewing, sorry for inconvenient. Pan -----Original Message----- From: Li, Pan2 <pan2.li@intel.com> Sent: Monday, June 19, 2023 4:07 PM To: gcc-patches@gcc.gnu.org Cc: juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Li, Pan2 <pan2.li@intel.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com Subject: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init From: Pan Li <pan2.li@intel.com> We extend the machine mode from 8 to 16 bits already. But there still one placing missing from the tree-streamer. It has one hard coded array for the machine code like size 256. In the lto pass, we memset the array by MAX_MACHINE_MODE count but the value of the MAX_MACHINE_MODE will grow as more and more modes are added. While the machine mode array in tree-streamer still leave 256 as is. Then, when the MAX_MACHINE_MODE is greater than 256, the memset of lto_output_init_mode_table will touch the memory out of range unexpected. This patch would like to take the MAX_MACHINE_MODE as the size of the array in tree-streamer, to make sure there is no potential unexpected memory access in future. Signed-off-by: Pan Li <pan2.li@intel.com> gcc/ChangeLog: * tree-streamer.cc (streamer_mode_table): Use MAX_MACHINE_MODE as array size. * tree-streamer.h (streamer_mode_table): Ditto. --- gcc/tree-streamer.cc | 2 +- gcc/tree-streamer.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc index ed65a7692e3..a28ef9c7920 100644 --- a/gcc/tree-streamer.cc +++ b/gcc/tree-streamer.cc @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see During streaming in, we translate the on the disk mode using this table. For normal LTO it is set to identity, for ACCEL_COMPILER depending on the mode_table content. */ -unsigned char streamer_mode_table[1 << 8]; +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; /* Check that all the TS_* structures handled by the streamer_write_* and streamer_read_* routines are exactly ALL the structures defined in diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index 170d61cf20b..51a292c8d80 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); void streamer_write_integer_cst (struct output_block *, tree); /* In tree-streamer.cc. */ -extern unsigned char streamer_mode_table[1 << 8]; +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; void streamer_check_handled_ts_structures (void); bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, hashval_t, unsigned *); -- 2.34.1 ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init 2023-06-19 8:16 ` Li, Pan2 @ 2023-06-19 8:40 ` Richard Biener 2023-06-19 9:08 ` Li, Pan2 2023-06-19 9:10 ` Jakub Jelinek 0 siblings, 2 replies; 33+ messages in thread From: Richard Biener @ 2023-06-19 8:40 UTC (permalink / raw) To: Li, Pan2 Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, Jakub Jelinek On Mon, 19 Jun 2023, Li, Pan2 wrote: > Add Richard Biener for reviewing, sorry for inconvenient. > > Pan > > -----Original Message----- > From: Li, Pan2 <pan2.li@intel.com> > Sent: Monday, June 19, 2023 4:07 PM > To: gcc-patches@gcc.gnu.org > Cc: juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Li, Pan2 <pan2.li@intel.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com > Subject: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init > > From: Pan Li <pan2.li@intel.com> > > We extend the machine mode from 8 to 16 bits already. But there still > one placing missing from the tree-streamer. It has one hard coded array > for the machine code like size 256. > > In the lto pass, we memset the array by MAX_MACHINE_MODE count but the > value of the MAX_MACHINE_MODE will grow as more and more modes are added. > While the machine mode array in tree-streamer still leave 256 as is. > > Then, when the MAX_MACHINE_MODE is greater than 256, the memset of > lto_output_init_mode_table will touch the memory out of range unexpected. > > This patch would like to take the MAX_MACHINE_MODE as the size of the > array in tree-streamer, to make sure there is no potential unexpected > memory access in future. You also have to fix bp_pack_machine_mode/bp_unpack_machine_mode which streams exactly values in [0, 1<<8 - 1]. CCing Jakub who invented this code. Richard. > Signed-off-by: Pan Li <pan2.li@intel.com> > > gcc/ChangeLog: > > * tree-streamer.cc (streamer_mode_table): Use MAX_MACHINE_MODE > as array size. > * tree-streamer.h (streamer_mode_table): Ditto. > --- > gcc/tree-streamer.cc | 2 +- > gcc/tree-streamer.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc > index ed65a7692e3..a28ef9c7920 100644 > --- a/gcc/tree-streamer.cc > +++ b/gcc/tree-streamer.cc > @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see > During streaming in, we translate the on the disk mode using this > table. For normal LTO it is set to identity, for ACCEL_COMPILER > depending on the mode_table content. */ > -unsigned char streamer_mode_table[1 << 8]; > +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > /* Check that all the TS_* structures handled by the streamer_write_* and > streamer_read_* routines are exactly ALL the structures defined in > diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h > index 170d61cf20b..51a292c8d80 100644 > --- a/gcc/tree-streamer.h > +++ b/gcc/tree-streamer.h > @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); > void streamer_write_integer_cst (struct output_block *, tree); > > /* In tree-streamer.cc. */ > -extern unsigned char streamer_mode_table[1 << 8]; > +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > void streamer_check_handled_ts_structures (void); > bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, > hashval_t, unsigned *); > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init 2023-06-19 8:40 ` Richard Biener @ 2023-06-19 9:08 ` Li, Pan2 2023-06-19 9:10 ` Jakub Jelinek 1 sibling, 0 replies; 33+ messages in thread From: Li, Pan2 @ 2023-06-19 9:08 UTC (permalink / raw) To: Richard Biener Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, Jakub Jelinek Thanks Richard for the review, just go thru the word (1 << 8) and found another one besides bp. Update the PATCH v2 as below. https://gcc.gnu.org/pipermail/gcc-patches/2023-June/622151.html Pan -----Original Message----- From: Richard Biener <rguenther@suse.de> Sent: Monday, June 19, 2023 4:41 PM To: Li, Pan2 <pan2.li@intel.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; Jakub Jelinek <jakub@redhat.com> Subject: RE: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init On Mon, 19 Jun 2023, Li, Pan2 wrote: > Add Richard Biener for reviewing, sorry for inconvenient. > > Pan > > -----Original Message----- > From: Li, Pan2 <pan2.li@intel.com> > Sent: Monday, June 19, 2023 4:07 PM > To: gcc-patches@gcc.gnu.org > Cc: juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Li, Pan2 <pan2.li@intel.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com > Subject: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init > > From: Pan Li <pan2.li@intel.com> > > We extend the machine mode from 8 to 16 bits already. But there still > one placing missing from the tree-streamer. It has one hard coded array > for the machine code like size 256. > > In the lto pass, we memset the array by MAX_MACHINE_MODE count but the > value of the MAX_MACHINE_MODE will grow as more and more modes are added. > While the machine mode array in tree-streamer still leave 256 as is. > > Then, when the MAX_MACHINE_MODE is greater than 256, the memset of > lto_output_init_mode_table will touch the memory out of range unexpected. > > This patch would like to take the MAX_MACHINE_MODE as the size of the > array in tree-streamer, to make sure there is no potential unexpected > memory access in future. You also have to fix bp_pack_machine_mode/bp_unpack_machine_mode which streams exactly values in [0, 1<<8 - 1]. CCing Jakub who invented this code. Richard. > Signed-off-by: Pan Li <pan2.li@intel.com> > > gcc/ChangeLog: > > * tree-streamer.cc (streamer_mode_table): Use MAX_MACHINE_MODE > as array size. > * tree-streamer.h (streamer_mode_table): Ditto. > --- > gcc/tree-streamer.cc | 2 +- > gcc/tree-streamer.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc > index ed65a7692e3..a28ef9c7920 100644 > --- a/gcc/tree-streamer.cc > +++ b/gcc/tree-streamer.cc > @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see > During streaming in, we translate the on the disk mode using this > table. For normal LTO it is set to identity, for ACCEL_COMPILER > depending on the mode_table content. */ > -unsigned char streamer_mode_table[1 << 8]; > +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > /* Check that all the TS_* structures handled by the streamer_write_* and > streamer_read_* routines are exactly ALL the structures defined in > diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h > index 170d61cf20b..51a292c8d80 100644 > --- a/gcc/tree-streamer.h > +++ b/gcc/tree-streamer.h > @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); > void streamer_write_integer_cst (struct output_block *, tree); > > /* In tree-streamer.cc. */ > -extern unsigned char streamer_mode_table[1 << 8]; > +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > void streamer_check_handled_ts_structures (void); > bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, > hashval_t, unsigned *); > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init 2023-06-19 8:40 ` Richard Biener 2023-06-19 9:08 ` Li, Pan2 @ 2023-06-19 9:10 ` Jakub Jelinek 1 sibling, 0 replies; 33+ messages in thread From: Jakub Jelinek @ 2023-06-19 9:10 UTC (permalink / raw) To: Richard Biener Cc: Li, Pan2, gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng On Mon, Jun 19, 2023 at 08:40:58AM +0000, Richard Biener wrote: > You also have to fix bp_pack_machine_mode/bp_unpack_machine_mode which > streams exactly values in [0, 1<<8 - 1]. > > CCing Jakub who invented this code. For stream-out, all it stores is a bool flag whether the mode is streamed out, and on stream in it contains a mapping table between host and offloading modes. For stream in, it actually isn't used despite the comment maybe suggesting it is, so I guess using MAX_MACHINE_MODE for it is ok. As you said, inline void bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) { streamer_mode_table[mode] = 1; bp_pack_enum (bp, machine_mode, 1 << 8, mode); } inline machine_mode bp_unpack_machine_mode (struct bitpack_d *bp) { return (machine_mode) ((class lto_input_block *) bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; } needs changing for the case when MAX_MACHINE_MODE > 256, but far more places make similar assumptions: E.g. lto_write_mode_table has bp_pack_value (&bp, m, 8); (if MAX_MACHINE_MODE > 256, this can't encode all modes), bp_pack_value (&bp, GET_MODE_INNER (m), 8); (ditto). lto_input_mode_table has e.g. unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); file_data->mode_table = table; Here we need to decide if we keep requiring that offloading architectures still have MAX_MACHINE_MODE <= 256 or not. Currently the offloading arches are nvptx and amdgcn, intelmic support has been removed. If yes, table can have unsigned char elements, but its size actually depends on the number of modes on the host side, so lto_write_mode_table would need to stream out the host MAX_MACHINE_MODE value and lto_input_mode_table stream it in and use instead of the 1 << 8 size above. If not, mode_table and unsigned char * would need to change to unsigned short *, or just conditionally depending on if MAX_MACHINE_MODE <= 256 or not. Then while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) { ... machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); again hardcode 8 bits, that needs to match how many bits packs the host compiler in lto_write_mode_table. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-19 8:07 [PATCH v1] RISC-V: Fix out of range memory access when lto mode init pan2.li 2023-06-19 8:16 ` Li, Pan2 @ 2023-06-19 9:05 ` pan2.li 2023-06-19 9:15 ` Richard Biener 2023-06-19 9:16 ` Jakub Jelinek 2023-06-21 7:58 ` [PATCH v3] Streamer: Fix out of range memory access of machine mode pan2.li 2 siblings, 2 replies; 33+ messages in thread From: pan2.li @ 2023-06-19 9:05 UTC (permalink / raw) To: gcc-patches Cc: juzhe.zhong, rdapp.gcc, jeffreyalaw, pan2.li, yanzhang.wang, kito.cheng, rguenther, jakub From: Pan Li <pan2.li@intel.com> We extend the machine mode from 8 to 16 bits already. But there still one placing missing from the tree-streamer. It has one hard coded array for the machine code like size 256. In the lto pass, we memset the array by MAX_MACHINE_MODE count but the value of the MAX_MACHINE_MODE will grow as more and more modes are added. While the machine mode array in tree-streamer still leave 256 as is. Then, when the MAX_MACHINE_MODE is greater than 256, the memset of lto_output_init_mode_table will touch the memory out of range unexpected. This patch would like to take the MAX_MACHINE_MODE as the size of the array in tree-streamer, to make sure there is no potential unexpected memory access in future. Signed-off-by: Pan Li <pan2.li@intel.com> gcc/ChangeLog: * lto-streamer-in.cc (lto_input_mode_table): Use MAX_MACHINE_MODE for memory allocation. * tree-streamer.cc: Use MAX_MACHINE_MODE for array size. * tree-streamer.h (streamer_mode_table): Ditto. (bp_pack_machine_mode): Ditto. (bp_unpack_machine_mode): Ditto. --- gcc/lto-streamer-in.cc | 3 ++- gcc/tree-streamer.cc | 2 +- gcc/tree-streamer.h | 7 ++++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc index 2cb83406db5..102b7e18526 100644 --- a/gcc/lto-streamer-in.cc +++ b/gcc/lto-streamer-in.cc @@ -1985,7 +1985,8 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) internal_error ("cannot read LTO mode table from %s", file_data->file_name); - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> ( + MAX_MACHINE_MODE); file_data->mode_table = table; const struct lto_simple_header_with_strings *header = (const struct lto_simple_header_with_strings *) data; diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc index ed65a7692e3..a28ef9c7920 100644 --- a/gcc/tree-streamer.cc +++ b/gcc/tree-streamer.cc @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see During streaming in, we translate the on the disk mode using this table. For normal LTO it is set to identity, for ACCEL_COMPILER depending on the mode_table content. */ -unsigned char streamer_mode_table[1 << 8]; +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; /* Check that all the TS_* structures handled by the streamer_write_* and streamer_read_* routines are exactly ALL the structures defined in diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index 170d61cf20b..be3a1938e76 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); void streamer_write_integer_cst (struct output_block *, tree); /* In tree-streamer.cc. */ -extern unsigned char streamer_mode_table[1 << 8]; +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; void streamer_check_handled_ts_structures (void); bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, hashval_t, unsigned *); @@ -108,7 +108,7 @@ inline void bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) { streamer_mode_table[mode] = 1; - bp_pack_enum (bp, machine_mode, 1 << 8, mode); + bp_pack_enum (bp, machine_mode, MAX_MACHINE_MODE, mode); } inline machine_mode @@ -116,7 +116,8 @@ bp_unpack_machine_mode (struct bitpack_d *bp) { return (machine_mode) ((class lto_input_block *) - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; + bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, + MAX_MACHINE_MODE)]; } #endif /* GCC_TREE_STREAMER_H */ -- 2.34.1 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-19 9:05 ` [PATCH] RISC-V: Fix out of range memory access of machine mode table pan2.li @ 2023-06-19 9:15 ` Richard Biener 2023-06-19 9:16 ` Jakub Jelinek 1 sibling, 0 replies; 33+ messages in thread From: Richard Biener @ 2023-06-19 9:15 UTC (permalink / raw) To: pan2.li Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, yanzhang.wang, kito.cheng, jakub On Mon, 19 Jun 2023, pan2.li@intel.com wrote: > From: Pan Li <pan2.li@intel.com> > > We extend the machine mode from 8 to 16 bits already. But there still > one placing missing from the tree-streamer. It has one hard coded array > for the machine code like size 256. > > In the lto pass, we memset the array by MAX_MACHINE_MODE count but the > value of the MAX_MACHINE_MODE will grow as more and more modes are added. > While the machine mode array in tree-streamer still leave 256 as is. > > Then, when the MAX_MACHINE_MODE is greater than 256, the memset of > lto_output_init_mode_table will touch the memory out of range unexpected. > > This patch would like to take the MAX_MACHINE_MODE as the size of the > array in tree-streamer, to make sure there is no potential unexpected > memory access in future. Please review more careful: void lto_input_mode_table (struct lto_file_decl_data *file_data) { ... while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) reads 8 bits again. ibit = bp_unpack_value (&bp, 8); fbit = bp_unpack_value (&bp, 8); likewise. Also file_data->mode_table is indexed by the _host_ mode, so you have to allocate enough space to fill in all streamed modes but you are using the targets MAX_MACHINE_MODE here. I think we need to stream the hosts MAX_MACHINE_MODE. Richard. > Signed-off-by: Pan Li <pan2.li@intel.com> > > gcc/ChangeLog: > > * lto-streamer-in.cc (lto_input_mode_table): Use > MAX_MACHINE_MODE for memory allocation. > * tree-streamer.cc: Use MAX_MACHINE_MODE for array size. > * tree-streamer.h (streamer_mode_table): Ditto. > (bp_pack_machine_mode): Ditto. > (bp_unpack_machine_mode): Ditto. > --- > gcc/lto-streamer-in.cc | 3 ++- > gcc/tree-streamer.cc | 2 +- > gcc/tree-streamer.h | 7 ++++--- > 3 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc > index 2cb83406db5..102b7e18526 100644 > --- a/gcc/lto-streamer-in.cc > +++ b/gcc/lto-streamer-in.cc > @@ -1985,7 +1985,8 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > internal_error ("cannot read LTO mode table from %s", > file_data->file_name); > > - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); > + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> ( > + MAX_MACHINE_MODE); > file_data->mode_table = table; > const struct lto_simple_header_with_strings *header > = (const struct lto_simple_header_with_strings *) data; > diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc > index ed65a7692e3..a28ef9c7920 100644 > --- a/gcc/tree-streamer.cc > +++ b/gcc/tree-streamer.cc > @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see > During streaming in, we translate the on the disk mode using this > table. For normal LTO it is set to identity, for ACCEL_COMPILER > depending on the mode_table content. */ > -unsigned char streamer_mode_table[1 << 8]; > +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > /* Check that all the TS_* structures handled by the streamer_write_* and > streamer_read_* routines are exactly ALL the structures defined in > diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h > index 170d61cf20b..be3a1938e76 100644 > --- a/gcc/tree-streamer.h > +++ b/gcc/tree-streamer.h > @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); > void streamer_write_integer_cst (struct output_block *, tree); > > /* In tree-streamer.cc. */ > -extern unsigned char streamer_mode_table[1 << 8]; > +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > void streamer_check_handled_ts_structures (void); > bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, > hashval_t, unsigned *); > @@ -108,7 +108,7 @@ inline void > bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) > { > streamer_mode_table[mode] = 1; > - bp_pack_enum (bp, machine_mode, 1 << 8, mode); > + bp_pack_enum (bp, machine_mode, MAX_MACHINE_MODE, mode); > } > > inline machine_mode > @@ -116,7 +116,8 @@ bp_unpack_machine_mode (struct bitpack_d *bp) > { > return (machine_mode) > ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, > + MAX_MACHINE_MODE)]; > } > > #endif /* GCC_TREE_STREAMER_H */ > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-19 9:05 ` [PATCH] RISC-V: Fix out of range memory access of machine mode table pan2.li 2023-06-19 9:15 ` Richard Biener @ 2023-06-19 9:16 ` Jakub Jelinek 2023-06-19 13:35 ` Li, Pan2 1 sibling, 1 reply; 33+ messages in thread From: Jakub Jelinek @ 2023-06-19 9:16 UTC (permalink / raw) To: pan2.li Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, yanzhang.wang, kito.cheng, rguenther On Mon, Jun 19, 2023 at 05:05:48PM +0800, pan2.li@intel.com wrote: > --- a/gcc/lto-streamer-in.cc > +++ b/gcc/lto-streamer-in.cc > @@ -1985,7 +1985,8 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > internal_error ("cannot read LTO mode table from %s", > file_data->file_name); > > - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); > + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> ( > + MAX_MACHINE_MODE); Incorrect formatting. And, see my other mail, this is wrong. > @@ -108,7 +108,7 @@ inline void > bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) > { > streamer_mode_table[mode] = 1; > - bp_pack_enum (bp, machine_mode, 1 << 8, mode); > + bp_pack_enum (bp, machine_mode, MAX_MACHINE_MODE, mode); > } > > inline machine_mode > @@ -116,7 +116,8 @@ bp_unpack_machine_mode (struct bitpack_d *bp) > { > return (machine_mode) > ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, > + MAX_MACHINE_MODE)]; > } And these two are wrong as well. The value passed to bp_pack_enum has to match the one used on bp_unpack_enum. But that is not the case after your changes. You stream out with the host MAX_MACHINE_MODE, and stream in for normal LTO with the same value (ok), but for offloading targets (nvptx, amdgcn) with a different MAX_MACHINE_MODE. That will immediate result in LTO streaming being out of sync and ICEs all around. The reason for using 1 << 8 there was exactly to make it interoperable for offloading. What could be perhaps done is that you stream out the host MAX_MACHINE_MODE value somewhere and stream it in inside of lto_input_mode_table before you allocate the table. But, that streamed in host max_machine_mdoe has to be remembered somewhere and used e.g. in bp_unpack_machine_mode instead of MAX_MACHINE_MODE. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-19 9:16 ` Jakub Jelinek @ 2023-06-19 13:35 ` Li, Pan2 2023-06-20 7:50 ` Li, Pan2 0 siblings, 1 reply; 33+ messages in thread From: Li, Pan2 @ 2023-06-19 13:35 UTC (permalink / raw) To: Jakub Jelinek Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, rguenther Thanks Jakub for reviewing, sorry for misleading and will have a try for PATCH v3. Pan -----Original Message----- From: Jakub Jelinek <jakub@redhat.com> Sent: Monday, June 19, 2023 5:17 PM To: Li, Pan2 <pan2.li@intel.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; rguenther@suse.de Subject: Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table On Mon, Jun 19, 2023 at 05:05:48PM +0800, pan2.li@intel.com wrote: > --- a/gcc/lto-streamer-in.cc > +++ b/gcc/lto-streamer-in.cc > @@ -1985,7 +1985,8 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > internal_error ("cannot read LTO mode table from %s", > file_data->file_name); > > - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); > + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> ( > + MAX_MACHINE_MODE); Incorrect formatting. And, see my other mail, this is wrong. > @@ -108,7 +108,7 @@ inline void > bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) > { > streamer_mode_table[mode] = 1; > - bp_pack_enum (bp, machine_mode, 1 << 8, mode); > + bp_pack_enum (bp, machine_mode, MAX_MACHINE_MODE, mode); > } > > inline machine_mode > @@ -116,7 +116,8 @@ bp_unpack_machine_mode (struct bitpack_d *bp) > { > return (machine_mode) > ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, > + MAX_MACHINE_MODE)]; > } And these two are wrong as well. The value passed to bp_pack_enum has to match the one used on bp_unpack_enum. But that is not the case after your changes. You stream out with the host MAX_MACHINE_MODE, and stream in for normal LTO with the same value (ok), but for offloading targets (nvptx, amdgcn) with a different MAX_MACHINE_MODE. That will immediate result in LTO streaming being out of sync and ICEs all around. The reason for using 1 << 8 there was exactly to make it interoperable for offloading. What could be perhaps done is that you stream out the host MAX_MACHINE_MODE value somewhere and stream it in inside of lto_input_mode_table before you allocate the table. But, that streamed in host max_machine_mdoe has to be remembered somewhere and used e.g. in bp_unpack_machine_mode instead of MAX_MACHINE_MODE. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-19 13:35 ` Li, Pan2 @ 2023-06-20 7:50 ` Li, Pan2 2023-06-20 8:03 ` Jakub Jelinek 0 siblings, 1 reply; 33+ messages in thread From: Li, Pan2 @ 2023-06-20 7:50 UTC (permalink / raw) To: Jakub Jelinek Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, rguenther Hi Jakub, Thanks for reviewing but I am not quite sure if I fully understand how to fix this issue. Could you please help to enlighten me more about this ? Currently for RISC-V, the memset has touched out of range memory already due to MAX_MACHINE_MODE > 256. And we may have below parts require adjusting. 1. streamer_mode_table. 2. bp_unpack_machine_mode/bp_pack_machine_mode 3. bp_pack_value/bp_unpack_value in lto_write_mode_table. 4. unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8) in lto_input_mode_table. For 1. is safe to extend the size to MAX_MACHINE_MODE as the array only used as Boolean, aka streamer_mode_table[XXXmode] = 1. For 2 & 3. Keep 1 << 8 as is, or stream out the host MAX_MACHINE_MODE value somewhere for underlying consuming? For 4, one possible approach is that extend unsigned char to unsigned short, as well as 256 to MAX_MACHINE_MODE. Because it stores the actually machine mode in array. Pan -----Original Message----- From: Li, Pan2 Sent: Monday, June 19, 2023 9:36 PM To: Jakub Jelinek <jakub@redhat.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; rguenther@suse.de Subject: RE: [PATCH] RISC-V: Fix out of range memory access of machine mode table Thanks Jakub for reviewing, sorry for misleading and will have a try for PATCH v3. Pan -----Original Message----- From: Jakub Jelinek <jakub@redhat.com> Sent: Monday, June 19, 2023 5:17 PM To: Li, Pan2 <pan2.li@intel.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; rguenther@suse.de Subject: Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table On Mon, Jun 19, 2023 at 05:05:48PM +0800, pan2.li@intel.com wrote: > --- a/gcc/lto-streamer-in.cc > +++ b/gcc/lto-streamer-in.cc > @@ -1985,7 +1985,8 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > internal_error ("cannot read LTO mode table from %s", > file_data->file_name); > > - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); > + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> ( > + MAX_MACHINE_MODE); Incorrect formatting. And, see my other mail, this is wrong. > @@ -108,7 +108,7 @@ inline void > bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) > { > streamer_mode_table[mode] = 1; > - bp_pack_enum (bp, machine_mode, 1 << 8, mode); > + bp_pack_enum (bp, machine_mode, MAX_MACHINE_MODE, mode); > } > > inline machine_mode > @@ -116,7 +116,8 @@ bp_unpack_machine_mode (struct bitpack_d *bp) > { > return (machine_mode) > ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, > + MAX_MACHINE_MODE)]; > } And these two are wrong as well. The value passed to bp_pack_enum has to match the one used on bp_unpack_enum. But that is not the case after your changes. You stream out with the host MAX_MACHINE_MODE, and stream in for normal LTO with the same value (ok), but for offloading targets (nvptx, amdgcn) with a different MAX_MACHINE_MODE. That will immediate result in LTO streaming being out of sync and ICEs all around. The reason for using 1 << 8 there was exactly to make it interoperable for offloading. What could be perhaps done is that you stream out the host MAX_MACHINE_MODE value somewhere and stream it in inside of lto_input_mode_table before you allocate the table. But, that streamed in host max_machine_mdoe has to be remembered somewhere and used e.g. in bp_unpack_machine_mode instead of MAX_MACHINE_MODE. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-20 7:50 ` Li, Pan2 @ 2023-06-20 8:03 ` Jakub Jelinek 2023-06-20 14:08 ` Li, Pan2 0 siblings, 1 reply; 33+ messages in thread From: Jakub Jelinek @ 2023-06-20 8:03 UTC (permalink / raw) To: Li, Pan2 Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, rguenther On Tue, Jun 20, 2023 at 07:50:00AM +0000, Li, Pan2 wrote: > Hi Jakub, > > Thanks for reviewing but I am not quite sure if I fully understand how to fix this issue. Could you please help to enlighten me more about this ? > > Currently for RISC-V, the memset has touched out of range memory already due to MAX_MACHINE_MODE > 256. And we may have below parts require adjusting. > > 1. streamer_mode_table. > 2. bp_unpack_machine_mode/bp_pack_machine_mode > 3. bp_pack_value/bp_unpack_value in lto_write_mode_table. > 4. unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8) in lto_input_mode_table. > > For 1. is safe to extend the size to MAX_MACHINE_MODE as the array only used as Boolean, aka streamer_mode_table[XXXmode] = 1. Because the array is used only during stream out, that is safe. > For 2 & 3. Keep 1 << 8 as is, or stream out the host MAX_MACHINE_MODE value somewhere for underlying consuming? You can't keep 1 << 8, otherwise you won't stream all the bits. I think you want to use 1 << ceil_log2 (MAX_MACHINE_MODE) on the stream out side, stream that ceil_log2 (MAX_MACHINE_MODE) value somewhere at the start of the mode table, add some field next to mode_table in lto_input_block which will contain that value (and make sure to initialize it to ceil_log2 (MAX_MACHINE_MODE) in case mode table isn't streamed in and use 1 << ...->mode_bits in e.g. bp_unpack_machine_mode Or for cases where 8 was used before use ceil_log2 (MAX_MACHINE_MODE) or mode_bits. > For 4, one possible approach is that extend unsigned char to unsigned short, as well as 256 to MAX_MACHINE_MODE. Because it stores the actually machine mode in array. The 1 << 8 needs to be similarly 1 << ...->mode_bits or ...->num_modes (that is also streamed out and in), it is sized by the host number of modes. Whether it is unsigned char or unsigned short array depends on if we want to support offloading targets with > 256 modes. If yes, it needs to be unsigned short, if not, we should add an assertion (e.g. on streaming in the LTO table) that MAX_MACHINE_MODE <= 256. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-20 8:03 ` Jakub Jelinek @ 2023-06-20 14:08 ` Li, Pan2 2023-06-20 15:25 ` Jakub Jelinek 0 siblings, 1 reply; 33+ messages in thread From: Li, Pan2 @ 2023-06-20 14:08 UTC (permalink / raw) To: Jakub Jelinek Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, rguenther Thanks Jakub for the explanation, I have a try like below patch but I am not quite sure it is expected, and where should I put the assertion. > If yes, it needs to > be unsigned short, if not, we should add an assertion (e.g. on streaming > in the LTO table) that MAX_MACHINE_MODE <= 256. diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc index 2cb83406db5..93ef97ec5d3 100644 --- a/gcc/lto-streamer-in.cc +++ b/gcc/lto-streamer-in.cc @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) internal_error ("cannot read LTO mode table from %s", file_data->file_name); - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); - file_data->mode_table = table; const struct lto_simple_header_with_strings *header = (const struct lto_simple_header_with_strings *) data; int string_offset; @@ -1994,6 +1992,9 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) string_offset = sizeof (*header) + header->main_size; lto_input_block ib (data + sizeof (*header), header->main_size, NULL); + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> ( + 1 << ib.mode_bits); + file_data->mode_table = table; data_in = lto_data_in_create (file_data, data + string_offset, header->string_size, vNULL); bitpack_d bp = streamer_read_bitpack (&ib); @@ -2001,13 +2002,13 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) table[VOIDmode] = VOIDmode; table[BLKmode] = BLKmode; unsigned int m; - while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) + while ((m = bp_unpack_value (&bp, ib.mode_bits)) != VOIDmode) { enum mode_class mclass = bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS); poly_uint16 size = bp_unpack_poly_value (&bp, 16); poly_uint16 prec = bp_unpack_poly_value (&bp, 16); - machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); + machine_mode inner = (machine_mode) bp_unpack_value (&bp, ib.mode_bits); poly_uint16 nunits = bp_unpack_poly_value (&bp, 16); unsigned int ibit = 0, fbit = 0; unsigned int real_fmt_len = 0; @@ -2018,8 +2019,8 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) case MODE_UFRACT: case MODE_ACCUM: case MODE_UACCUM: - ibit = bp_unpack_value (&bp, 8); - fbit = bp_unpack_value (&bp, 8); + ibit = bp_unpack_value (&bp, ib.mode_bits); + fbit = bp_unpack_value (&bp, ib.mode_bits); break; case MODE_FLOAT: case MODE_DECIMAL_FLOAT: diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index fc7133d07ba..f1d826d59e4 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -352,6 +352,8 @@ public: const char *data; const unsigned char *mode_table; + /* Indicates how many bits of one machine mode will have. */ + const unsigned int mode_bits = ceil_log2 (MAX_MACHINE_MODE) ; unsigned int p; unsigned int len; }; diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc index ed65a7692e3..a28ef9c7920 100644 --- a/gcc/tree-streamer.cc +++ b/gcc/tree-streamer.cc @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see During streaming in, we translate the on the disk mode using this table. For normal LTO it is set to identity, for ACCEL_COMPILER depending on the mode_table content. */ -unsigned char streamer_mode_table[1 << 8]; +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; /* Check that all the TS_* structures handled by the streamer_write_* and streamer_read_* routines are exactly ALL the structures defined in diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index 170d61cf20b..9aa248cd2f5 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); void streamer_write_integer_cst (struct output_block *, tree); /* In tree-streamer.cc. */ -extern unsigned char streamer_mode_table[1 << 8]; +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; void streamer_check_handled_ts_structures (void); bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, hashval_t, unsigned *); @@ -108,15 +108,18 @@ inline void bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) { streamer_mode_table[mode] = 1; - bp_pack_enum (bp, machine_mode, 1 << 8, mode); + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); + + bp_pack_enum (bp, machine_mode, last, mode); } inline machine_mode bp_unpack_machine_mode (struct bitpack_d *bp) { - return (machine_mode) - ((class lto_input_block *) - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; + lto_input_block *input_block = (class lto_input_block *)bp->stream; + int index = bp_unpack_enum (bp, machine_mode, input_block->mode_bits); + + return (machine_mode)input_block->mode_table[index]; } #endif /* GCC_TREE_STREAMER_H */ Pan -----Original Message----- From: Jakub Jelinek <jakub@redhat.com> Sent: Tuesday, June 20, 2023 4:04 PM To: Li, Pan2 <pan2.li@intel.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; rguenther@suse.de Subject: Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table On Tue, Jun 20, 2023 at 07:50:00AM +0000, Li, Pan2 wrote: > Hi Jakub, > > Thanks for reviewing but I am not quite sure if I fully understand how to fix this issue. Could you please help to enlighten me more about this ? > > Currently for RISC-V, the memset has touched out of range memory already due to MAX_MACHINE_MODE > 256. And we may have below parts require adjusting. > > 1. streamer_mode_table. > 2. bp_unpack_machine_mode/bp_pack_machine_mode > 3. bp_pack_value/bp_unpack_value in lto_write_mode_table. > 4. unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8) in lto_input_mode_table. > > For 1. is safe to extend the size to MAX_MACHINE_MODE as the array only used as Boolean, aka streamer_mode_table[XXXmode] = 1. Because the array is used only during stream out, that is safe. > For 2 & 3. Keep 1 << 8 as is, or stream out the host MAX_MACHINE_MODE value somewhere for underlying consuming? You can't keep 1 << 8, otherwise you won't stream all the bits. I think you want to use 1 << ceil_log2 (MAX_MACHINE_MODE) on the stream out side, stream that ceil_log2 (MAX_MACHINE_MODE) value somewhere at the start of the mode table, add some field next to mode_table in lto_input_block which will contain that value (and make sure to initialize it to ceil_log2 (MAX_MACHINE_MODE) in case mode table isn't streamed in and use 1 << ...->mode_bits in e.g. bp_unpack_machine_mode Or for cases where 8 was used before use ceil_log2 (MAX_MACHINE_MODE) or mode_bits. > For 4, one possible approach is that extend unsigned char to unsigned short, as well as 256 to MAX_MACHINE_MODE. Because it stores the actually machine mode in array. The 1 << 8 needs to be similarly 1 << ...->mode_bits or ...->num_modes (that is also streamed out and in), it is sized by the host number of modes. Whether it is unsigned char or unsigned short array depends on if we want to support offloading targets with > 256 modes. If yes, it needs to be unsigned short, if not, we should add an assertion (e.g. on streaming in the LTO table) that MAX_MACHINE_MODE <= 256. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-20 14:08 ` Li, Pan2 @ 2023-06-20 15:25 ` Jakub Jelinek 2023-06-21 6:59 ` Li, Pan2 0 siblings, 1 reply; 33+ messages in thread From: Jakub Jelinek @ 2023-06-20 15:25 UTC (permalink / raw) To: Li, Pan2 Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, rguenther On Tue, Jun 20, 2023 at 02:08:07PM +0000, Li, Pan2 via Gcc-patches wrote: > Thanks Jakub for the explanation, I have a try like below patch but I am not quite sure it is expected, and where should I put the assertion. > > > If yes, it needs to > > be unsigned short, if not, we should add an assertion (e.g. on streaming > > in the LTO table) that MAX_MACHINE_MODE <= 256. > > diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc > index 2cb83406db5..93ef97ec5d3 100644 > --- a/gcc/lto-streamer-in.cc > +++ b/gcc/lto-streamer-in.cc > @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > internal_error ("cannot read LTO mode table from %s", > file_data->file_name); > > - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); > - file_data->mode_table = table; > const struct lto_simple_header_with_strings *header > = (const struct lto_simple_header_with_strings *) data; > int string_offset; > @@ -1994,6 +1992,9 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > string_offset = sizeof (*header) + header->main_size; > > lto_input_block ib (data + sizeof (*header), header->main_size, NULL); > + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> ( > + 1 << ib.mode_bits); > + file_data->mode_table = table; > data_in = lto_data_in_create (file_data, data + string_offset, > header->string_size, vNULL); > bitpack_d bp = streamer_read_bitpack (&ib); Your ib.mode_bits is again the same ceil_log2 (MAX_MACHINE_MODE) value. You need to stream that value out in lto-streamer-out.cc as perhaps the first thing in the bitpack and stream it back here, so some mode_bits = bp_unpack_value (&bp, 5); or so (perhaps 4 would be enough if we only support up to 15 bits for mode). I.e. tell the offloading compiler what value had the host compiler when streaming LTO out. Then move those 3 lines from the above after that. I'd put it next to mode_table, so file_data->mode_bits. The unsigned char *table = ggc_cleared_vec_alloc<unsigned char> ( 1 << ib.mode_bits); formatting is wrong, ( shouldn't if at all possible be the last character on a line. In this case, > --- a/gcc/lto-streamer.h > +++ b/gcc/lto-streamer.h > @@ -352,6 +352,8 @@ public: > > const char *data; > const unsigned char *mode_table; > + /* Indicates how many bits of one machine mode will have. */ > + const unsigned int mode_bits = ceil_log2 (MAX_MACHINE_MODE) ; As I said earlier, I'd put it elsewhere. The formatting is wrong (no space before semicolon) and please don't add NSDMIs in structures which don't have them already. > inline machine_mode > bp_unpack_machine_mode (struct bitpack_d *bp) > { > - return (machine_mode) > - ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + lto_input_block *input_block = (class lto_input_block *)bp->stream; Wrong formatting again, there shouldn't be two consecutive spaces in there. On the other hand, there should be a space between *) and bp. > + int index = bp_unpack_enum (bp, machine_mode, input_block->mode_bits); > + > + return (machine_mode)input_block->mode_table[index]; And here similarly. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-20 15:25 ` Jakub Jelinek @ 2023-06-21 6:59 ` Li, Pan2 2023-06-21 7:16 ` Jakub Jelinek 0 siblings, 1 reply; 33+ messages in thread From: Li, Pan2 @ 2023-06-21 6:59 UTC (permalink / raw) To: Jakub Jelinek Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, rguenther Thanks Jakub for the useful comments, go thru the mail list and have a refinement version as below. But I not sure if I understand correct about adding new field named mode_bits in struct lto_file_decl_data, looks unnecessary up to a point. Thanks again for your coaching with patient. diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc index 2cb83406db5..2a0720b4e6f 100644 --- a/gcc/lto-streamer-in.cc +++ b/gcc/lto-streamer-in.cc @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) internal_error ("cannot read LTO mode table from %s", file_data->file_name); - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); - file_data->mode_table = table; const struct lto_simple_header_with_strings *header = (const struct lto_simple_header_with_strings *) data; int string_offset; @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) header->string_size, vNULL); bitpack_d bp = streamer_read_bitpack (&ib); + unsigned mode_bits = bp_unpack_value (&bp, 5); + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); + + file_data->mode_table = table; + file_data->mode_bits = mode_bits; + table[VOIDmode] = VOIDmode; table[BLKmode] = BLKmode; unsigned int m; - while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) + while ((m = bp_unpack_value (&bp, mode_bits)) != VOIDmode) { enum mode_class mclass = bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS); poly_uint16 size = bp_unpack_poly_value (&bp, 16); poly_uint16 prec = bp_unpack_poly_value (&bp, 16); - machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); + machine_mode inner = (machine_mode) bp_unpack_value (&bp, mode_bits); poly_uint16 nunits = bp_unpack_poly_value (&bp, 16); unsigned int ibit = 0, fbit = 0; unsigned int real_fmt_len = 0; diff --git a/gcc/lto-streamer-out.cc b/gcc/lto-streamer-out.cc index 5ab2eb4301e..77250ee2385 100644 --- a/gcc/lto-streamer-out.cc +++ b/gcc/lto-streamer-out.cc @@ -3196,6 +3196,11 @@ lto_write_mode_table (void) if (inner_m != m) streamer_mode_table[(int) inner_m] = 1; } + + /* Pack the mode_bits value within 5 bits (up to 31) of the beginning. */ + unsigned mode_bits = ceil_log2 (MAX_MACHINE_MODE); + bp_pack_value (&bp, mode_bits, 5); + /* First stream modes that have GET_MODE_INNER (m) == m, so that we can refer to them afterwards. */ for (int pass = 0; pass < 2; pass++) @@ -3205,11 +3210,11 @@ lto_write_mode_table (void) machine_mode m = (machine_mode) i; if ((GET_MODE_INNER (m) == m) ^ (pass == 0)) continue; - bp_pack_value (&bp, m, 8); + bp_pack_value (&bp, m, mode_bits); bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m)); bp_pack_poly_value (&bp, GET_MODE_SIZE (m), 16); bp_pack_poly_value (&bp, GET_MODE_PRECISION (m), 16); - bp_pack_value (&bp, GET_MODE_INNER (m), 8); + bp_pack_value (&bp, GET_MODE_INNER (m), mode_bits); bp_pack_poly_value (&bp, GET_MODE_NUNITS (m), 16); switch (GET_MODE_CLASS (m)) { @@ -3229,7 +3234,7 @@ lto_write_mode_table (void) } bp_pack_string (ob, &bp, GET_MODE_NAME (m), true); } - bp_pack_value (&bp, VOIDmode, 8); + bp_pack_value (&bp, VOIDmode, mode_bits); streamer_write_bitpack (&bp); diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index fc7133d07ba..443f0cd616e 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data int order_base; int unit_base; + + unsigned mode_bits; }; typedef struct lto_file_decl_data *lto_file_decl_data_ptr; diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc index ed65a7692e3..a28ef9c7920 100644 --- a/gcc/tree-streamer.cc +++ b/gcc/tree-streamer.cc @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see During streaming in, we translate the on the disk mode using this table. For normal LTO it is set to identity, for ACCEL_COMPILER depending on the mode_table content. */ -unsigned char streamer_mode_table[1 << 8]; +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; /* Check that all the TS_* structures handled by the streamer_write_* and streamer_read_* routines are exactly ALL the structures defined in diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index 170d61cf20b..10718b03640 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); void streamer_write_integer_cst (struct output_block *, tree); /* In tree-streamer.cc. */ -extern unsigned char streamer_mode_table[1 << 8]; +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; void streamer_check_handled_ts_structures (void); bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, hashval_t, unsigned *); @@ -108,15 +108,19 @@ inline void bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) { streamer_mode_table[mode] = 1; - bp_pack_enum (bp, machine_mode, 1 << 8, mode); + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); + + bp_pack_enum (bp, machine_mode, last, mode); } inline machine_mode bp_unpack_machine_mode (struct bitpack_d *bp) { - return (machine_mode) - ((class lto_input_block *) - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); + lto_input_block *input_block = (class lto_input_block *) bp->stream; + int index = bp_unpack_enum (bp, machine_mode, last); + + return (machine_mode) input_block->mode_table[index]; } #endif /* GCC_TREE_STREAMER_H */ Pan -----Original Message----- From: Jakub Jelinek <jakub@redhat.com> Sent: Tuesday, June 20, 2023 11:26 PM To: Li, Pan2 <pan2.li@intel.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; rguenther@suse.de Subject: Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table On Tue, Jun 20, 2023 at 02:08:07PM +0000, Li, Pan2 via Gcc-patches wrote: > Thanks Jakub for the explanation, I have a try like below patch but I am not quite sure it is expected, and where should I put the assertion. > > > If yes, it needs to > > be unsigned short, if not, we should add an assertion (e.g. on streaming > > in the LTO table) that MAX_MACHINE_MODE <= 256. > > diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc > index 2cb83406db5..93ef97ec5d3 100644 > --- a/gcc/lto-streamer-in.cc > +++ b/gcc/lto-streamer-in.cc > @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > internal_error ("cannot read LTO mode table from %s", > file_data->file_name); > > - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); > - file_data->mode_table = table; > const struct lto_simple_header_with_strings *header > = (const struct lto_simple_header_with_strings *) data; > int string_offset; > @@ -1994,6 +1992,9 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > string_offset = sizeof (*header) + header->main_size; > > lto_input_block ib (data + sizeof (*header), header->main_size, NULL); > + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> ( > + 1 << ib.mode_bits); > + file_data->mode_table = table; > data_in = lto_data_in_create (file_data, data + string_offset, > header->string_size, vNULL); > bitpack_d bp = streamer_read_bitpack (&ib); Your ib.mode_bits is again the same ceil_log2 (MAX_MACHINE_MODE) value. You need to stream that value out in lto-streamer-out.cc as perhaps the first thing in the bitpack and stream it back here, so some mode_bits = bp_unpack_value (&bp, 5); or so (perhaps 4 would be enough if we only support up to 15 bits for mode). I.e. tell the offloading compiler what value had the host compiler when streaming LTO out. Then move those 3 lines from the above after that. I'd put it next to mode_table, so file_data->mode_bits. The unsigned char *table = ggc_cleared_vec_alloc<unsigned char> ( 1 << ib.mode_bits); formatting is wrong, ( shouldn't if at all possible be the last character on a line. In this case, > --- a/gcc/lto-streamer.h > +++ b/gcc/lto-streamer.h > @@ -352,6 +352,8 @@ public: > > const char *data; > const unsigned char *mode_table; > + /* Indicates how many bits of one machine mode will have. */ > + const unsigned int mode_bits = ceil_log2 (MAX_MACHINE_MODE) ; As I said earlier, I'd put it elsewhere. The formatting is wrong (no space before semicolon) and please don't add NSDMIs in structures which don't have them already. > inline machine_mode > bp_unpack_machine_mode (struct bitpack_d *bp) > { > - return (machine_mode) > - ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + lto_input_block *input_block = (class lto_input_block *)bp->stream; Wrong formatting again, there shouldn't be two consecutive spaces in there. On the other hand, there should be a space between *) and bp. > + int index = bp_unpack_enum (bp, machine_mode, input_block->mode_bits); > + > + return (machine_mode)input_block->mode_table[index]; And here similarly. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-21 6:59 ` Li, Pan2 @ 2023-06-21 7:16 ` Jakub Jelinek 2023-06-21 7:23 ` Li, Pan2 0 siblings, 1 reply; 33+ messages in thread From: Jakub Jelinek @ 2023-06-21 7:16 UTC (permalink / raw) To: Li, Pan2 Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, rguenther On Wed, Jun 21, 2023 at 06:59:08AM +0000, Li, Pan2 wrote: > inline machine_mode > bp_unpack_machine_mode (struct bitpack_d *bp) > { > - return (machine_mode) > - ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > + lto_input_block *input_block = (class lto_input_block *) bp->stream; Still 2 spaces instead of 1 here, otherwise it LGTM, but the important question is if you have actually tested it with offloading, because only that will verify it works correctly. See https://gcc.gnu.org/wiki/Offloading for details. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-21 7:16 ` Jakub Jelinek @ 2023-06-21 7:23 ` Li, Pan2 2023-06-22 0:19 ` Li, Pan2 0 siblings, 1 reply; 33+ messages in thread From: Li, Pan2 @ 2023-06-21 7:23 UTC (permalink / raw) To: Jakub Jelinek Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, rguenther Thanks Jakub, will fix the format issue and send the V3 patch, as well as try to validate it for offloading. Pan -----Original Message----- From: Jakub Jelinek <jakub@redhat.com> Sent: Wednesday, June 21, 2023 3:16 PM To: Li, Pan2 <pan2.li@intel.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; rguenther@suse.de Subject: Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table On Wed, Jun 21, 2023 at 06:59:08AM +0000, Li, Pan2 wrote: > inline machine_mode > bp_unpack_machine_mode (struct bitpack_d *bp) > { > - return (machine_mode) > - ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > + lto_input_block *input_block = (class lto_input_block *) bp->stream; Still 2 spaces instead of 1 here, otherwise it LGTM, but the important question is if you have actually tested it with offloading, because only that will verify it works correctly. See https://gcc.gnu.org/wiki/Offloading for details. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-21 7:23 ` Li, Pan2 @ 2023-06-22 0:19 ` Li, Pan2 2023-06-28 18:37 ` Jeff Law 0 siblings, 1 reply; 33+ messages in thread From: Li, Pan2 @ 2023-06-22 0:19 UTC (permalink / raw) To: Jakub Jelinek Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, rguenther Hi there, I try to verify the offloading following below doc. https://gcc.gnu.org/wiki/Offloading#How_to_build_an_offloading-enabled_GCC with some steps: 1. Build nvptx-tools. 2. Symbol link nvptx-newlib to gcc source code. 3. Build the Nividia PTX accel compiler. 4. Build the host compiler with nvptx as offload target, but I don't have the GPU, then drop the --with-cuda-driver=xxx option. 5. Run command for building, aka ./nvptx-tools/usr/local/bin/gcc -O0 -fopenmp test.c -o test.elf. The building complete successfully, but looks I cannot run it without GPU, and I am not very sure this is good enough for validation or not. Pan -----Original Message----- From: Li, Pan2 Sent: Wednesday, June 21, 2023 3:23 PM To: Jakub Jelinek <jakub@redhat.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; rguenther@suse.de Subject: RE: [PATCH] RISC-V: Fix out of range memory access of machine mode table Thanks Jakub, will fix the format issue and send the V3 patch, as well as try to validate it for offloading. Pan -----Original Message----- From: Jakub Jelinek <jakub@redhat.com> Sent: Wednesday, June 21, 2023 3:16 PM To: Li, Pan2 <pan2.li@intel.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; rguenther@suse.de Subject: Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table On Wed, Jun 21, 2023 at 06:59:08AM +0000, Li, Pan2 wrote: > inline machine_mode > bp_unpack_machine_mode (struct bitpack_d *bp) > { > - return (machine_mode) > - ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > + lto_input_block *input_block = (class lto_input_block *) bp->stream; Still 2 spaces instead of 1 here, otherwise it LGTM, but the important question is if you have actually tested it with offloading, because only that will verify it works correctly. See https://gcc.gnu.org/wiki/Offloading for details. Jakub ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table 2023-06-22 0:19 ` Li, Pan2 @ 2023-06-28 18:37 ` Jeff Law 0 siblings, 0 replies; 33+ messages in thread From: Jeff Law @ 2023-06-28 18:37 UTC (permalink / raw) To: Li, Pan2, Jakub Jelinek Cc: gcc-patches, juzhe.zhong, rdapp.gcc, Wang, Yanzhang, kito.cheng, rguenther On 6/21/23 18:19, Li, Pan2 wrote: > Hi there, > > I try to verify the offloading following below doc. > > https://gcc.gnu.org/wiki/Offloading#How_to_build_an_offloading-enabled_GCC > > with some steps: > > 1. Build nvptx-tools. > 2. Symbol link nvptx-newlib to gcc source code. > 3. Build the Nividia PTX accel compiler. > 4. Build the host compiler with nvptx as offload target, but I don't have the GPU, then drop the --with-cuda-driver=xxx option. > 5. Run command for building, aka ./nvptx-tools/usr/local/bin/gcc -O0 -fopenmp test.c -o test.elf. > > The building complete successfully, but looks I cannot run it without GPU, and I am not very sure this is good enough for validation or not. If you don't have a suitable GPU for offloading, you could instead just compare the offloaded binary before/after your change. I would expect them to be 100% identical. If we take that route for verification, I think the question turns into how to do that for the testsuite. ie, I think Jakub wants to verify that check-target-libgomp still passes when offloading is enabled. I don't think there's an easy way to capture the resulting binaries for comparison purposes. But that's what I'd suggest given the lack of a suitable GPU for testing. So you might need to hack up the libgomp testsuite's .exp files to capture the binaries. Before going to those extremes, I would suggest verifying that you do in fact get identical binaries before/after your change on a simple offloading test. jeff ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3] Streamer: Fix out of range memory access of machine mode 2023-06-19 8:07 [PATCH v1] RISC-V: Fix out of range memory access when lto mode init pan2.li 2023-06-19 8:16 ` Li, Pan2 2023-06-19 9:05 ` [PATCH] RISC-V: Fix out of range memory access of machine mode table pan2.li @ 2023-06-21 7:58 ` pan2.li 2023-06-22 15:26 ` Li, Pan2 2023-06-29 9:29 ` Thomas Schwinge 2 siblings, 2 replies; 33+ messages in thread From: pan2.li @ 2023-06-21 7:58 UTC (permalink / raw) To: gcc-patches Cc: juzhe.zhong, rdapp.gcc, jeffreyalaw, pan2.li, yanzhang.wang, kito.cheng, rguenther, jakub From: Pan Li <pan2.li@intel.com> We extend the machine mode from 8 to 16 bits already. But there still one placing missing from the streamer. It has one hard coded array for the machine code like size 256. In the lto pass, we memset the array by MAX_MACHINE_MODE count but the value of the MAX_MACHINE_MODE will grow as more and more modes are added. While the machine mode array in tree-streamer still leave 256 as is. Then, when the MAX_MACHINE_MODE is greater than 256, the memset of lto_output_init_mode_table will touch the memory out of range unexpected. This patch would like to take the MAX_MACHINE_MODE as the size of the array in streamer, to make sure there is no potential unexpected memory access in future. Meanwhile, this patch also adjust some place which has MAX_MACHINE_MODE <= 256 assumption. Signed-off-by: Pan Li <pan2.li@intel.com> gcc/ChangeLog: * lto-streamer-in.cc (lto_input_mode_table): Stream in the mode bits for machine mode table. * lto-streamer-out.cc (lto_write_mode_table): Stream out the HOST machine mode bits. * lto-streamer.h (struct lto_file_decl_data): New fields mode_bits. * tree-streamer.cc (streamer_mode_table): Take MAX_MACHINE_MODE as the table size. * tree-streamer.h (streamer_mode_table): Ditto. (bp_pack_machine_mode): Take 1 << ceil_log2 (MAX_MACHINE_MODE) as the packing limit. (bp_unpack_machine_mode): Ditto. --- gcc/lto-streamer-in.cc | 12 ++++++++---- gcc/lto-streamer-out.cc | 11 ++++++++--- gcc/lto-streamer.h | 2 ++ gcc/tree-streamer.cc | 2 +- gcc/tree-streamer.h | 14 +++++++++----- 5 files changed, 28 insertions(+), 13 deletions(-) diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc index 2cb83406db5..2a0720b4e6f 100644 --- a/gcc/lto-streamer-in.cc +++ b/gcc/lto-streamer-in.cc @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) internal_error ("cannot read LTO mode table from %s", file_data->file_name); - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); - file_data->mode_table = table; const struct lto_simple_header_with_strings *header = (const struct lto_simple_header_with_strings *) data; int string_offset; @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) header->string_size, vNULL); bitpack_d bp = streamer_read_bitpack (&ib); + unsigned mode_bits = bp_unpack_value (&bp, 5); + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); + + file_data->mode_table = table; + file_data->mode_bits = mode_bits; + table[VOIDmode] = VOIDmode; table[BLKmode] = BLKmode; unsigned int m; - while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) + while ((m = bp_unpack_value (&bp, mode_bits)) != VOIDmode) { enum mode_class mclass = bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS); poly_uint16 size = bp_unpack_poly_value (&bp, 16); poly_uint16 prec = bp_unpack_poly_value (&bp, 16); - machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); + machine_mode inner = (machine_mode) bp_unpack_value (&bp, mode_bits); poly_uint16 nunits = bp_unpack_poly_value (&bp, 16); unsigned int ibit = 0, fbit = 0; unsigned int real_fmt_len = 0; diff --git a/gcc/lto-streamer-out.cc b/gcc/lto-streamer-out.cc index 5ab2eb4301e..36899283ded 100644 --- a/gcc/lto-streamer-out.cc +++ b/gcc/lto-streamer-out.cc @@ -3196,6 +3196,11 @@ lto_write_mode_table (void) if (inner_m != m) streamer_mode_table[(int) inner_m] = 1; } + + /* Pack the mode_bits value within 5 bits (up to 31) in the beginning. */ + unsigned mode_bits = ceil_log2 (MAX_MACHINE_MODE); + bp_pack_value (&bp, mode_bits, 5); + /* First stream modes that have GET_MODE_INNER (m) == m, so that we can refer to them afterwards. */ for (int pass = 0; pass < 2; pass++) @@ -3205,11 +3210,11 @@ lto_write_mode_table (void) machine_mode m = (machine_mode) i; if ((GET_MODE_INNER (m) == m) ^ (pass == 0)) continue; - bp_pack_value (&bp, m, 8); + bp_pack_value (&bp, m, mode_bits); bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m)); bp_pack_poly_value (&bp, GET_MODE_SIZE (m), 16); bp_pack_poly_value (&bp, GET_MODE_PRECISION (m), 16); - bp_pack_value (&bp, GET_MODE_INNER (m), 8); + bp_pack_value (&bp, GET_MODE_INNER (m), mode_bits); bp_pack_poly_value (&bp, GET_MODE_NUNITS (m), 16); switch (GET_MODE_CLASS (m)) { @@ -3229,7 +3234,7 @@ lto_write_mode_table (void) } bp_pack_string (ob, &bp, GET_MODE_NAME (m), true); } - bp_pack_value (&bp, VOIDmode, 8); + bp_pack_value (&bp, VOIDmode, mode_bits); streamer_write_bitpack (&bp); diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index fc7133d07ba..443f0cd616e 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data int order_base; int unit_base; + + unsigned mode_bits; }; typedef struct lto_file_decl_data *lto_file_decl_data_ptr; diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc index ed65a7692e3..a28ef9c7920 100644 --- a/gcc/tree-streamer.cc +++ b/gcc/tree-streamer.cc @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see During streaming in, we translate the on the disk mode using this table. For normal LTO it is set to identity, for ACCEL_COMPILER depending on the mode_table content. */ -unsigned char streamer_mode_table[1 << 8]; +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; /* Check that all the TS_* structures handled by the streamer_write_* and streamer_read_* routines are exactly ALL the structures defined in diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index 170d61cf20b..ff8bccf901a 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); void streamer_write_integer_cst (struct output_block *, tree); /* In tree-streamer.cc. */ -extern unsigned char streamer_mode_table[1 << 8]; +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; void streamer_check_handled_ts_structures (void); bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, hashval_t, unsigned *); @@ -108,15 +108,19 @@ inline void bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) { streamer_mode_table[mode] = 1; - bp_pack_enum (bp, machine_mode, 1 << 8, mode); + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); + + bp_pack_enum (bp, machine_mode, last, mode); } inline machine_mode bp_unpack_machine_mode (struct bitpack_d *bp) { - return (machine_mode) - ((class lto_input_block *) - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); + lto_input_block *input_block = (class lto_input_block *) bp->stream; + int index = bp_unpack_enum (bp, machine_mode, last); + + return (machine_mode) input_block->mode_table[index]; } #endif /* GCC_TREE_STREAMER_H */ -- 2.34.1 ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH v3] Streamer: Fix out of range memory access of machine mode 2023-06-21 7:58 ` [PATCH v3] Streamer: Fix out of range memory access of machine mode pan2.li @ 2023-06-22 15:26 ` Li, Pan2 2023-06-29 9:29 ` Thomas Schwinge 1 sibling, 0 replies; 33+ messages in thread From: Li, Pan2 @ 2023-06-22 15:26 UTC (permalink / raw) To: gcc-patches Cc: juzhe.zhong, rdapp.gcc, jeffreyalaw, Wang, Yanzhang, kito.cheng, rguenther, jakub, Jivan Hakobyan Looks Jivan notices this issue too, cc Jivan for awareness. > Re: LTO: buffer overflow in lto_output_init_mode_table > Hi Robbin. > Thank you for responding. > I will defer my thread. > On Thu, Jun 22, 2023 at 3:42 PM Robin Dapp <rdapp.gcc@gmail.com> wrote: >> Hi Jivan, >> >> I think Pan is already on this problem. Please see this thread: >> https://gcc.gnu.org/pipermail/gcc-patches/2023-June/622129.html >> >> Regards >> Robin Pan -----Original Message----- From: Li, Pan2 <pan2.li@intel.com> Sent: Wednesday, June 21, 2023 3:58 PM To: gcc-patches@gcc.gnu.org Cc: juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Li, Pan2 <pan2.li@intel.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; rguenther@suse.de; jakub@redhat.com Subject: [PATCH v3] Streamer: Fix out of range memory access of machine mode From: Pan Li <pan2.li@intel.com> We extend the machine mode from 8 to 16 bits already. But there still one placing missing from the streamer. It has one hard coded array for the machine code like size 256. In the lto pass, we memset the array by MAX_MACHINE_MODE count but the value of the MAX_MACHINE_MODE will grow as more and more modes are added. While the machine mode array in tree-streamer still leave 256 as is. Then, when the MAX_MACHINE_MODE is greater than 256, the memset of lto_output_init_mode_table will touch the memory out of range unexpected. This patch would like to take the MAX_MACHINE_MODE as the size of the array in streamer, to make sure there is no potential unexpected memory access in future. Meanwhile, this patch also adjust some place which has MAX_MACHINE_MODE <= 256 assumption. Signed-off-by: Pan Li <pan2.li@intel.com> gcc/ChangeLog: * lto-streamer-in.cc (lto_input_mode_table): Stream in the mode bits for machine mode table. * lto-streamer-out.cc (lto_write_mode_table): Stream out the HOST machine mode bits. * lto-streamer.h (struct lto_file_decl_data): New fields mode_bits. * tree-streamer.cc (streamer_mode_table): Take MAX_MACHINE_MODE as the table size. * tree-streamer.h (streamer_mode_table): Ditto. (bp_pack_machine_mode): Take 1 << ceil_log2 (MAX_MACHINE_MODE) as the packing limit. (bp_unpack_machine_mode): Ditto. --- gcc/lto-streamer-in.cc | 12 ++++++++---- gcc/lto-streamer-out.cc | 11 ++++++++--- gcc/lto-streamer.h | 2 ++ gcc/tree-streamer.cc | 2 +- gcc/tree-streamer.h | 14 +++++++++----- 5 files changed, 28 insertions(+), 13 deletions(-) diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc index 2cb83406db5..2a0720b4e6f 100644 --- a/gcc/lto-streamer-in.cc +++ b/gcc/lto-streamer-in.cc @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) internal_error ("cannot read LTO mode table from %s", file_data->file_name); - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); - file_data->mode_table = table; const struct lto_simple_header_with_strings *header = (const struct lto_simple_header_with_strings *) data; int string_offset; @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) header->string_size, vNULL); bitpack_d bp = streamer_read_bitpack (&ib); + unsigned mode_bits = bp_unpack_value (&bp, 5); + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); + + file_data->mode_table = table; + file_data->mode_bits = mode_bits; + table[VOIDmode] = VOIDmode; table[BLKmode] = BLKmode; unsigned int m; - while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) + while ((m = bp_unpack_value (&bp, mode_bits)) != VOIDmode) { enum mode_class mclass = bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS); poly_uint16 size = bp_unpack_poly_value (&bp, 16); poly_uint16 prec = bp_unpack_poly_value (&bp, 16); - machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); + machine_mode inner = (machine_mode) bp_unpack_value (&bp, mode_bits); poly_uint16 nunits = bp_unpack_poly_value (&bp, 16); unsigned int ibit = 0, fbit = 0; unsigned int real_fmt_len = 0; diff --git a/gcc/lto-streamer-out.cc b/gcc/lto-streamer-out.cc index 5ab2eb4301e..36899283ded 100644 --- a/gcc/lto-streamer-out.cc +++ b/gcc/lto-streamer-out.cc @@ -3196,6 +3196,11 @@ lto_write_mode_table (void) if (inner_m != m) streamer_mode_table[(int) inner_m] = 1; } + + /* Pack the mode_bits value within 5 bits (up to 31) in the beginning. */ + unsigned mode_bits = ceil_log2 (MAX_MACHINE_MODE); + bp_pack_value (&bp, mode_bits, 5); + /* First stream modes that have GET_MODE_INNER (m) == m, so that we can refer to them afterwards. */ for (int pass = 0; pass < 2; pass++) @@ -3205,11 +3210,11 @@ lto_write_mode_table (void) machine_mode m = (machine_mode) i; if ((GET_MODE_INNER (m) == m) ^ (pass == 0)) continue; - bp_pack_value (&bp, m, 8); + bp_pack_value (&bp, m, mode_bits); bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m)); bp_pack_poly_value (&bp, GET_MODE_SIZE (m), 16); bp_pack_poly_value (&bp, GET_MODE_PRECISION (m), 16); - bp_pack_value (&bp, GET_MODE_INNER (m), 8); + bp_pack_value (&bp, GET_MODE_INNER (m), mode_bits); bp_pack_poly_value (&bp, GET_MODE_NUNITS (m), 16); switch (GET_MODE_CLASS (m)) { @@ -3229,7 +3234,7 @@ lto_write_mode_table (void) } bp_pack_string (ob, &bp, GET_MODE_NAME (m), true); } - bp_pack_value (&bp, VOIDmode, 8); + bp_pack_value (&bp, VOIDmode, mode_bits); streamer_write_bitpack (&bp); diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index fc7133d07ba..443f0cd616e 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data int order_base; int unit_base; + + unsigned mode_bits; }; typedef struct lto_file_decl_data *lto_file_decl_data_ptr; diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc index ed65a7692e3..a28ef9c7920 100644 --- a/gcc/tree-streamer.cc +++ b/gcc/tree-streamer.cc @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see During streaming in, we translate the on the disk mode using this table. For normal LTO it is set to identity, for ACCEL_COMPILER depending on the mode_table content. */ -unsigned char streamer_mode_table[1 << 8]; +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; /* Check that all the TS_* structures handled by the streamer_write_* and streamer_read_* routines are exactly ALL the structures defined in diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index 170d61cf20b..ff8bccf901a 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); void streamer_write_integer_cst (struct output_block *, tree); /* In tree-streamer.cc. */ -extern unsigned char streamer_mode_table[1 << 8]; +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; void streamer_check_handled_ts_structures (void); bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, hashval_t, unsigned *); @@ -108,15 +108,19 @@ inline void bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) { streamer_mode_table[mode] = 1; - bp_pack_enum (bp, machine_mode, 1 << 8, mode); + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); + + bp_pack_enum (bp, machine_mode, last, mode); } inline machine_mode bp_unpack_machine_mode (struct bitpack_d *bp) { - return (machine_mode) - ((class lto_input_block *) - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); + lto_input_block *input_block = (class lto_input_block *) bp->stream; + int index = bp_unpack_enum (bp, machine_mode, last); + + return (machine_mode) input_block->mode_table[index]; } #endif /* GCC_TREE_STREAMER_H */ -- 2.34.1 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode 2023-06-21 7:58 ` [PATCH v3] Streamer: Fix out of range memory access of machine mode pan2.li 2023-06-22 15:26 ` Li, Pan2 @ 2023-06-29 9:29 ` Thomas Schwinge 2023-06-29 9:33 ` juzhe.zhong 2023-06-29 20:14 ` Thomas Schwinge 1 sibling, 2 replies; 33+ messages in thread From: Thomas Schwinge @ 2023-06-29 9:29 UTC (permalink / raw) To: Pan Li Cc: gcc-patches, juzhe.zhong, rdapp.gcc, jeffreyalaw, yanzhang.wang, kito.cheng, rguenther, jakub, Tobias Burnus Hi! On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > We extend the machine mode from 8 to 16 bits already. But there still > one placing missing from the streamer. It has one hard coded array > for the machine code like size 256. > > In the lto pass, we memset the array by MAX_MACHINE_MODE count but the > value of the MAX_MACHINE_MODE will grow as more and more modes are > added. While the machine mode array in tree-streamer still leave 256 as is. > > Then, when the MAX_MACHINE_MODE is greater than 256, the memset of > lto_output_init_mode_table will touch the memory out of range unexpected. Uh. :-O > This patch would like to take the MAX_MACHINE_MODE as the size of the > array in streamer, to make sure there is no potential unexpected > memory access in future. Meanwhile, this patch also adjust some place > which has MAX_MACHINE_MODE <= 256 assumption. Thanks to Jakub and Richard for guidance re the offloading compilation case, where we've got different 'MAX_MACHINE_MODE's between stream-out and stream-in, and a modes mapping table. However, with this patch, there are ICEs all over the place... I'm having a look. Grüße Thomas > gcc/ChangeLog: > > * lto-streamer-in.cc (lto_input_mode_table): Stream in the mode > bits for machine mode table. > * lto-streamer-out.cc (lto_write_mode_table): Stream out the > HOST machine mode bits. > * lto-streamer.h (struct lto_file_decl_data): New fields mode_bits. > * tree-streamer.cc (streamer_mode_table): Take MAX_MACHINE_MODE > as the table size. > * tree-streamer.h (streamer_mode_table): Ditto. > (bp_pack_machine_mode): Take 1 << ceil_log2 (MAX_MACHINE_MODE) > as the packing limit. > (bp_unpack_machine_mode): Ditto. > --- > gcc/lto-streamer-in.cc | 12 ++++++++---- > gcc/lto-streamer-out.cc | 11 ++++++++--- > gcc/lto-streamer.h | 2 ++ > gcc/tree-streamer.cc | 2 +- > gcc/tree-streamer.h | 14 +++++++++----- > 5 files changed, 28 insertions(+), 13 deletions(-) > > diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc > index 2cb83406db5..2a0720b4e6f 100644 > --- a/gcc/lto-streamer-in.cc > +++ b/gcc/lto-streamer-in.cc > @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > internal_error ("cannot read LTO mode table from %s", > file_data->file_name); > > - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); > - file_data->mode_table = table; > const struct lto_simple_header_with_strings *header > = (const struct lto_simple_header_with_strings *) data; > int string_offset; > @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > header->string_size, vNULL); > bitpack_d bp = streamer_read_bitpack (&ib); > > + unsigned mode_bits = bp_unpack_value (&bp, 5); > + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); > + > + file_data->mode_table = table; > + file_data->mode_bits = mode_bits; > + > table[VOIDmode] = VOIDmode; > table[BLKmode] = BLKmode; > unsigned int m; > - while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) > + while ((m = bp_unpack_value (&bp, mode_bits)) != VOIDmode) > { > enum mode_class mclass > = bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS); > poly_uint16 size = bp_unpack_poly_value (&bp, 16); > poly_uint16 prec = bp_unpack_poly_value (&bp, 16); > - machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); > + machine_mode inner = (machine_mode) bp_unpack_value (&bp, mode_bits); > poly_uint16 nunits = bp_unpack_poly_value (&bp, 16); > unsigned int ibit = 0, fbit = 0; > unsigned int real_fmt_len = 0; > diff --git a/gcc/lto-streamer-out.cc b/gcc/lto-streamer-out.cc > index 5ab2eb4301e..36899283ded 100644 > --- a/gcc/lto-streamer-out.cc > +++ b/gcc/lto-streamer-out.cc > @@ -3196,6 +3196,11 @@ lto_write_mode_table (void) > if (inner_m != m) > streamer_mode_table[(int) inner_m] = 1; > } > + > + /* Pack the mode_bits value within 5 bits (up to 31) in the beginning. */ > + unsigned mode_bits = ceil_log2 (MAX_MACHINE_MODE); > + bp_pack_value (&bp, mode_bits, 5); > + > /* First stream modes that have GET_MODE_INNER (m) == m, > so that we can refer to them afterwards. */ > for (int pass = 0; pass < 2; pass++) > @@ -3205,11 +3210,11 @@ lto_write_mode_table (void) > machine_mode m = (machine_mode) i; > if ((GET_MODE_INNER (m) == m) ^ (pass == 0)) > continue; > - bp_pack_value (&bp, m, 8); > + bp_pack_value (&bp, m, mode_bits); > bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m)); > bp_pack_poly_value (&bp, GET_MODE_SIZE (m), 16); > bp_pack_poly_value (&bp, GET_MODE_PRECISION (m), 16); > - bp_pack_value (&bp, GET_MODE_INNER (m), 8); > + bp_pack_value (&bp, GET_MODE_INNER (m), mode_bits); > bp_pack_poly_value (&bp, GET_MODE_NUNITS (m), 16); > switch (GET_MODE_CLASS (m)) > { > @@ -3229,7 +3234,7 @@ lto_write_mode_table (void) > } > bp_pack_string (ob, &bp, GET_MODE_NAME (m), true); > } > - bp_pack_value (&bp, VOIDmode, 8); > + bp_pack_value (&bp, VOIDmode, mode_bits); > > streamer_write_bitpack (&bp); > > diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h > index fc7133d07ba..443f0cd616e 100644 > --- a/gcc/lto-streamer.h > +++ b/gcc/lto-streamer.h > @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data > int order_base; > > int unit_base; > + > + unsigned mode_bits; > }; > > typedef struct lto_file_decl_data *lto_file_decl_data_ptr; > diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc > index ed65a7692e3..a28ef9c7920 100644 > --- a/gcc/tree-streamer.cc > +++ b/gcc/tree-streamer.cc > @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see > During streaming in, we translate the on the disk mode using this > table. For normal LTO it is set to identity, for ACCEL_COMPILER > depending on the mode_table content. */ > -unsigned char streamer_mode_table[1 << 8]; > +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > /* Check that all the TS_* structures handled by the streamer_write_* and > streamer_read_* routines are exactly ALL the structures defined in > diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h > index 170d61cf20b..ff8bccf901a 100644 > --- a/gcc/tree-streamer.h > +++ b/gcc/tree-streamer.h > @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); > void streamer_write_integer_cst (struct output_block *, tree); > > /* In tree-streamer.cc. */ > -extern unsigned char streamer_mode_table[1 << 8]; > +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > void streamer_check_handled_ts_structures (void); > bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, > hashval_t, unsigned *); > @@ -108,15 +108,19 @@ inline void > bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) > { > streamer_mode_table[mode] = 1; > - bp_pack_enum (bp, machine_mode, 1 << 8, mode); > + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > + > + bp_pack_enum (bp, machine_mode, last, mode); > } > > inline machine_mode > bp_unpack_machine_mode (struct bitpack_d *bp) > { > - return (machine_mode) > - ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > + lto_input_block *input_block = (class lto_input_block *) bp->stream; > + int index = bp_unpack_enum (bp, machine_mode, last); > + > + return (machine_mode) input_block->mode_table[index]; > } > > #endif /* GCC_TREE_STREAMER_H */ > -- > 2.34.1 ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode 2023-06-29 9:29 ` Thomas Schwinge @ 2023-06-29 9:33 ` juzhe.zhong 2023-06-29 9:47 ` Thomas Schwinge 2023-06-29 20:14 ` Thomas Schwinge 1 sibling, 1 reply; 33+ messages in thread From: juzhe.zhong @ 2023-06-29 9:33 UTC (permalink / raw) To: Thomas Schwinge, pan2.li Cc: gcc-patches, Robin Dapp, jeffreyalaw, yanzhang.wang, kito.cheng, rguenther, jakub, Tobias Burnus [-- Attachment #1: Type: text/plain, Size: 11162 bytes --] Not sure what happens you said ICEs all over the place... Actually, even without this patch, current upstream GCC in RISCV port already ICE all over the place: FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup-3.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup-3.c (test for excess errors) FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-1.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-1.c (test for excess errors) FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-17.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-17.c (test for excess errors) FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_run-3.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_run-3.c (test for excess errors) FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-2.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-2.c (test for excess errors) FAIL: gcc.target/riscv/rvv/autovec/binop/narrow_run-1.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) FAIL: gcc.target/riscv/rvv/autovec/binop/narrow_run-1.c (test for excess errors) FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-16.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-16.c (test for excess errors) FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-3.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-3.c (test for excess errors) juzhe.zhong@rivai.ai From: Thomas Schwinge Date: 2023-06-29 17:29 To: Pan Li CC: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; yanzhang.wang@intel.com; kito.cheng@gmail.com; rguenther@suse.de; jakub@redhat.com; Tobias Burnus Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode Hi! On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > We extend the machine mode from 8 to 16 bits already. But there still > one placing missing from the streamer. It has one hard coded array > for the machine code like size 256. > > In the lto pass, we memset the array by MAX_MACHINE_MODE count but the > value of the MAX_MACHINE_MODE will grow as more and more modes are > added. While the machine mode array in tree-streamer still leave 256 as is. > > Then, when the MAX_MACHINE_MODE is greater than 256, the memset of > lto_output_init_mode_table will touch the memory out of range unexpected. Uh. :-O > This patch would like to take the MAX_MACHINE_MODE as the size of the > array in streamer, to make sure there is no potential unexpected > memory access in future. Meanwhile, this patch also adjust some place > which has MAX_MACHINE_MODE <= 256 assumption. Thanks to Jakub and Richard for guidance re the offloading compilation case, where we've got different 'MAX_MACHINE_MODE's between stream-out and stream-in, and a modes mapping table. However, with this patch, there are ICEs all over the place... I'm having a look. Grüße Thomas > gcc/ChangeLog: > > * lto-streamer-in.cc (lto_input_mode_table): Stream in the mode > bits for machine mode table. > * lto-streamer-out.cc (lto_write_mode_table): Stream out the > HOST machine mode bits. > * lto-streamer.h (struct lto_file_decl_data): New fields mode_bits. > * tree-streamer.cc (streamer_mode_table): Take MAX_MACHINE_MODE > as the table size. > * tree-streamer.h (streamer_mode_table): Ditto. > (bp_pack_machine_mode): Take 1 << ceil_log2 (MAX_MACHINE_MODE) > as the packing limit. > (bp_unpack_machine_mode): Ditto. > --- > gcc/lto-streamer-in.cc | 12 ++++++++---- > gcc/lto-streamer-out.cc | 11 ++++++++--- > gcc/lto-streamer.h | 2 ++ > gcc/tree-streamer.cc | 2 +- > gcc/tree-streamer.h | 14 +++++++++----- > 5 files changed, 28 insertions(+), 13 deletions(-) > > diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc > index 2cb83406db5..2a0720b4e6f 100644 > --- a/gcc/lto-streamer-in.cc > +++ b/gcc/lto-streamer-in.cc > @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > internal_error ("cannot read LTO mode table from %s", > file_data->file_name); > > - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); > - file_data->mode_table = table; > const struct lto_simple_header_with_strings *header > = (const struct lto_simple_header_with_strings *) data; > int string_offset; > @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > header->string_size, vNULL); > bitpack_d bp = streamer_read_bitpack (&ib); > > + unsigned mode_bits = bp_unpack_value (&bp, 5); > + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); > + > + file_data->mode_table = table; > + file_data->mode_bits = mode_bits; > + > table[VOIDmode] = VOIDmode; > table[BLKmode] = BLKmode; > unsigned int m; > - while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) > + while ((m = bp_unpack_value (&bp, mode_bits)) != VOIDmode) > { > enum mode_class mclass > = bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS); > poly_uint16 size = bp_unpack_poly_value (&bp, 16); > poly_uint16 prec = bp_unpack_poly_value (&bp, 16); > - machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); > + machine_mode inner = (machine_mode) bp_unpack_value (&bp, mode_bits); > poly_uint16 nunits = bp_unpack_poly_value (&bp, 16); > unsigned int ibit = 0, fbit = 0; > unsigned int real_fmt_len = 0; > diff --git a/gcc/lto-streamer-out.cc b/gcc/lto-streamer-out.cc > index 5ab2eb4301e..36899283ded 100644 > --- a/gcc/lto-streamer-out.cc > +++ b/gcc/lto-streamer-out.cc > @@ -3196,6 +3196,11 @@ lto_write_mode_table (void) > if (inner_m != m) > streamer_mode_table[(int) inner_m] = 1; > } > + > + /* Pack the mode_bits value within 5 bits (up to 31) in the beginning. */ > + unsigned mode_bits = ceil_log2 (MAX_MACHINE_MODE); > + bp_pack_value (&bp, mode_bits, 5); > + > /* First stream modes that have GET_MODE_INNER (m) == m, > so that we can refer to them afterwards. */ > for (int pass = 0; pass < 2; pass++) > @@ -3205,11 +3210,11 @@ lto_write_mode_table (void) > machine_mode m = (machine_mode) i; > if ((GET_MODE_INNER (m) == m) ^ (pass == 0)) > continue; > - bp_pack_value (&bp, m, 8); > + bp_pack_value (&bp, m, mode_bits); > bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m)); > bp_pack_poly_value (&bp, GET_MODE_SIZE (m), 16); > bp_pack_poly_value (&bp, GET_MODE_PRECISION (m), 16); > - bp_pack_value (&bp, GET_MODE_INNER (m), 8); > + bp_pack_value (&bp, GET_MODE_INNER (m), mode_bits); > bp_pack_poly_value (&bp, GET_MODE_NUNITS (m), 16); > switch (GET_MODE_CLASS (m)) > { > @@ -3229,7 +3234,7 @@ lto_write_mode_table (void) > } > bp_pack_string (ob, &bp, GET_MODE_NAME (m), true); > } > - bp_pack_value (&bp, VOIDmode, 8); > + bp_pack_value (&bp, VOIDmode, mode_bits); > > streamer_write_bitpack (&bp); > > diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h > index fc7133d07ba..443f0cd616e 100644 > --- a/gcc/lto-streamer.h > +++ b/gcc/lto-streamer.h > @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data > int order_base; > > int unit_base; > + > + unsigned mode_bits; > }; > > typedef struct lto_file_decl_data *lto_file_decl_data_ptr; > diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc > index ed65a7692e3..a28ef9c7920 100644 > --- a/gcc/tree-streamer.cc > +++ b/gcc/tree-streamer.cc > @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see > During streaming in, we translate the on the disk mode using this > table. For normal LTO it is set to identity, for ACCEL_COMPILER > depending on the mode_table content. */ > -unsigned char streamer_mode_table[1 << 8]; > +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > /* Check that all the TS_* structures handled by the streamer_write_* and > streamer_read_* routines are exactly ALL the structures defined in > diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h > index 170d61cf20b..ff8bccf901a 100644 > --- a/gcc/tree-streamer.h > +++ b/gcc/tree-streamer.h > @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); > void streamer_write_integer_cst (struct output_block *, tree); > > /* In tree-streamer.cc. */ > -extern unsigned char streamer_mode_table[1 << 8]; > +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > void streamer_check_handled_ts_structures (void); > bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, > hashval_t, unsigned *); > @@ -108,15 +108,19 @@ inline void > bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) > { > streamer_mode_table[mode] = 1; > - bp_pack_enum (bp, machine_mode, 1 << 8, mode); > + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > + > + bp_pack_enum (bp, machine_mode, last, mode); > } > > inline machine_mode > bp_unpack_machine_mode (struct bitpack_d *bp) > { > - return (machine_mode) > - ((class lto_input_block *) > - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > + lto_input_block *input_block = (class lto_input_block *) bp->stream; > + int index = bp_unpack_enum (bp, machine_mode, last); > + > + return (machine_mode) input_block->mode_table[index]; > } > > #endif /* GCC_TREE_STREAMER_H */ > -- > 2.34.1 ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode 2023-06-29 9:33 ` juzhe.zhong @ 2023-06-29 9:47 ` Thomas Schwinge 2023-06-29 9:52 ` juzhe.zhong 0 siblings, 1 reply; 33+ messages in thread From: Thomas Schwinge @ 2023-06-29 9:47 UTC (permalink / raw) To: juzhe.zhong Cc: pan2.li, gcc-patches, Robin Dapp, jeffreyalaw, yanzhang.wang, kito.cheng, rguenther, jakub, Tobias Burnus Hi! On 2023-06-29T17:33:14+0800, "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> wrote: > Not sure what happens you said ICEs all over the place... Ah, sorry for not providing proper context here. My comment was about heterogeneous GCC configurations involving code offloading, like: x86_64 host with GCN, nvptx offload targets, which I'd been asked to test this "Streamer: Fix out of range memory access of machine mode" for (in private email) -- for good reasons, as we've now found. ;-| > Actually, even without this patch, current upstream GCC in RISCV port already ICE all over the place: > > FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup-3.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup-3.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-1.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-1.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-17.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-17.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_run-3.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_run-3.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-2.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-2.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/binop/narrow_run-1.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/binop/narrow_run-1.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-16.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-16.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-3.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-3.c (test for excess errors) That looks like a different issue, though? Grüße Thomas > From: Thomas Schwinge > Date: 2023-06-29 17:29 > To: Pan Li > CC: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; yanzhang.wang@intel.com; kito.cheng@gmail.com; rguenther@suse.de; jakub@redhat.com; Tobias Burnus > Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode > Hi! > > On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> We extend the machine mode from 8 to 16 bits already. But there still >> one placing missing from the streamer. It has one hard coded array >> for the machine code like size 256. >> >> In the lto pass, we memset the array by MAX_MACHINE_MODE count but the >> value of the MAX_MACHINE_MODE will grow as more and more modes are >> added. While the machine mode array in tree-streamer still leave 256 as is. >> >> Then, when the MAX_MACHINE_MODE is greater than 256, the memset of >> lto_output_init_mode_table will touch the memory out of range unexpected. > > Uh. :-O > >> This patch would like to take the MAX_MACHINE_MODE as the size of the >> array in streamer, to make sure there is no potential unexpected >> memory access in future. Meanwhile, this patch also adjust some place >> which has MAX_MACHINE_MODE <= 256 assumption. > > Thanks to Jakub and Richard for guidance re the offloading compilation > case, where we've got different 'MAX_MACHINE_MODE's between stream-out > and stream-in, and a modes mapping table. > > However, with this patch, there are ICEs all over the place... I'm > having a look. > > > Grüße > Thomas > > >> gcc/ChangeLog: >> >> * lto-streamer-in.cc (lto_input_mode_table): Stream in the mode >> bits for machine mode table. >> * lto-streamer-out.cc (lto_write_mode_table): Stream out the >> HOST machine mode bits. >> * lto-streamer.h (struct lto_file_decl_data): New fields mode_bits. >> * tree-streamer.cc (streamer_mode_table): Take MAX_MACHINE_MODE >> as the table size. >> * tree-streamer.h (streamer_mode_table): Ditto. >> (bp_pack_machine_mode): Take 1 << ceil_log2 (MAX_MACHINE_MODE) >> as the packing limit. >> (bp_unpack_machine_mode): Ditto. >> --- >> gcc/lto-streamer-in.cc | 12 ++++++++---- >> gcc/lto-streamer-out.cc | 11 ++++++++--- >> gcc/lto-streamer.h | 2 ++ >> gcc/tree-streamer.cc | 2 +- >> gcc/tree-streamer.h | 14 +++++++++----- >> 5 files changed, 28 insertions(+), 13 deletions(-) >> >> diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc >> index 2cb83406db5..2a0720b4e6f 100644 >> --- a/gcc/lto-streamer-in.cc >> +++ b/gcc/lto-streamer-in.cc >> @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >> internal_error ("cannot read LTO mode table from %s", >> file_data->file_name); >> >> - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); >> - file_data->mode_table = table; >> const struct lto_simple_header_with_strings *header >> = (const struct lto_simple_header_with_strings *) data; >> int string_offset; >> @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >> header->string_size, vNULL); >> bitpack_d bp = streamer_read_bitpack (&ib); >> >> + unsigned mode_bits = bp_unpack_value (&bp, 5); >> + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); >> + >> + file_data->mode_table = table; >> + file_data->mode_bits = mode_bits; >> + >> table[VOIDmode] = VOIDmode; >> table[BLKmode] = BLKmode; >> unsigned int m; >> - while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) >> + while ((m = bp_unpack_value (&bp, mode_bits)) != VOIDmode) >> { >> enum mode_class mclass >> = bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS); >> poly_uint16 size = bp_unpack_poly_value (&bp, 16); >> poly_uint16 prec = bp_unpack_poly_value (&bp, 16); >> - machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); >> + machine_mode inner = (machine_mode) bp_unpack_value (&bp, mode_bits); >> poly_uint16 nunits = bp_unpack_poly_value (&bp, 16); >> unsigned int ibit = 0, fbit = 0; >> unsigned int real_fmt_len = 0; >> diff --git a/gcc/lto-streamer-out.cc b/gcc/lto-streamer-out.cc >> index 5ab2eb4301e..36899283ded 100644 >> --- a/gcc/lto-streamer-out.cc >> +++ b/gcc/lto-streamer-out.cc >> @@ -3196,6 +3196,11 @@ lto_write_mode_table (void) >> if (inner_m != m) >> streamer_mode_table[(int) inner_m] = 1; >> } >> + >> + /* Pack the mode_bits value within 5 bits (up to 31) in the beginning. */ >> + unsigned mode_bits = ceil_log2 (MAX_MACHINE_MODE); >> + bp_pack_value (&bp, mode_bits, 5); >> + >> /* First stream modes that have GET_MODE_INNER (m) == m, >> so that we can refer to them afterwards. */ >> for (int pass = 0; pass < 2; pass++) >> @@ -3205,11 +3210,11 @@ lto_write_mode_table (void) >> machine_mode m = (machine_mode) i; >> if ((GET_MODE_INNER (m) == m) ^ (pass == 0)) >> continue; >> - bp_pack_value (&bp, m, 8); >> + bp_pack_value (&bp, m, mode_bits); >> bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m)); >> bp_pack_poly_value (&bp, GET_MODE_SIZE (m), 16); >> bp_pack_poly_value (&bp, GET_MODE_PRECISION (m), 16); >> - bp_pack_value (&bp, GET_MODE_INNER (m), 8); >> + bp_pack_value (&bp, GET_MODE_INNER (m), mode_bits); >> bp_pack_poly_value (&bp, GET_MODE_NUNITS (m), 16); >> switch (GET_MODE_CLASS (m)) >> { >> @@ -3229,7 +3234,7 @@ lto_write_mode_table (void) >> } >> bp_pack_string (ob, &bp, GET_MODE_NAME (m), true); >> } >> - bp_pack_value (&bp, VOIDmode, 8); >> + bp_pack_value (&bp, VOIDmode, mode_bits); >> >> streamer_write_bitpack (&bp); >> >> diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h >> index fc7133d07ba..443f0cd616e 100644 >> --- a/gcc/lto-streamer.h >> +++ b/gcc/lto-streamer.h >> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data >> int order_base; >> >> int unit_base; >> + >> + unsigned mode_bits; >> }; >> >> typedef struct lto_file_decl_data *lto_file_decl_data_ptr; >> diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc >> index ed65a7692e3..a28ef9c7920 100644 >> --- a/gcc/tree-streamer.cc >> +++ b/gcc/tree-streamer.cc >> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see >> During streaming in, we translate the on the disk mode using this >> table. For normal LTO it is set to identity, for ACCEL_COMPILER >> depending on the mode_table content. */ >> -unsigned char streamer_mode_table[1 << 8]; >> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; >> >> /* Check that all the TS_* structures handled by the streamer_write_* and >> streamer_read_* routines are exactly ALL the structures defined in >> diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h >> index 170d61cf20b..ff8bccf901a 100644 >> --- a/gcc/tree-streamer.h >> +++ b/gcc/tree-streamer.h >> @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); >> void streamer_write_integer_cst (struct output_block *, tree); >> >> /* In tree-streamer.cc. */ >> -extern unsigned char streamer_mode_table[1 << 8]; >> +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; >> void streamer_check_handled_ts_structures (void); >> bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, >> hashval_t, unsigned *); >> @@ -108,15 +108,19 @@ inline void >> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) >> { >> streamer_mode_table[mode] = 1; >> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); >> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >> + >> + bp_pack_enum (bp, machine_mode, last, mode); >> } >> >> inline machine_mode >> bp_unpack_machine_mode (struct bitpack_d *bp) >> { >> - return (machine_mode) >> - ((class lto_input_block *) >> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; >> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >> + lto_input_block *input_block = (class lto_input_block *) bp->stream; >> + int index = bp_unpack_enum (bp, machine_mode, last); >> + >> + return (machine_mode) input_block->mode_table[index]; >> } >> >> #endif /* GCC_TREE_STREAMER_H */ >> -- >> 2.34.1 ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode 2023-06-29 9:47 ` Thomas Schwinge @ 2023-06-29 9:52 ` juzhe.zhong 0 siblings, 0 replies; 33+ messages in thread From: juzhe.zhong @ 2023-06-29 9:52 UTC (permalink / raw) To: Thomas Schwinge Cc: pan2.li, gcc-patches, Robin Dapp, jeffreyalaw, yanzhang.wang, kito.cheng, rguenther, jakub, Tobias Burnus [-- Attachment #1: Type: text/plain, Size: 12441 bytes --] Ok. Thanks for taking care of it! >> That looks like a different issue, though? Yes, it's different issue and I am trying to fix it in RISC-V backend. Thanks a lot. juzhe.zhong@rivai.ai From: Thomas Schwinge Date: 2023-06-29 17:47 To: juzhe.zhong@rivai.ai CC: pan2.li@intel.com; gcc-patches@gcc.gnu.org; Robin Dapp; jeffreyalaw@gmail.com; yanzhang.wang@intel.com; kito.cheng@gmail.com; rguenther@suse.de; jakub@redhat.com; Tobias Burnus Subject: Re: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode Hi! On 2023-06-29T17:33:14+0800, "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai> wrote: > Not sure what happens you said ICEs all over the place... Ah, sorry for not providing proper context here. My comment was about heterogeneous GCC configurations involving code offloading, like: x86_64 host with GCN, nvptx offload targets, which I'd been asked to test this "Streamer: Fix out of range memory access of machine mode" for (in private email) -- for good reasons, as we've now found. ;-| > Actually, even without this patch, current upstream GCC in RISCV port already ICE all over the place: > > FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup-3.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup-3.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-1.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-1.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-17.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-17.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_run-3.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/multiple_rgroup_run-3.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-2.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-2.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/binop/narrow_run-1.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/binop/narrow_run-1.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-16.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-16.c (test for excess errors) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-3.c (internal compiler error: tree check: expected none of vector_type, have vector_type in divmod_candidate_p, at tree-ssa-math-opts.cc:4998) > FAIL: gcc.target/riscv/rvv/autovec/partial/slp_run-3.c (test for excess errors) That looks like a different issue, though? Grüße Thomas > From: Thomas Schwinge > Date: 2023-06-29 17:29 > To: Pan Li > CC: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; yanzhang.wang@intel.com; kito.cheng@gmail.com; rguenther@suse.de; jakub@redhat.com; Tobias Burnus > Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode > Hi! > > On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> We extend the machine mode from 8 to 16 bits already. But there still >> one placing missing from the streamer. It has one hard coded array >> for the machine code like size 256. >> >> In the lto pass, we memset the array by MAX_MACHINE_MODE count but the >> value of the MAX_MACHINE_MODE will grow as more and more modes are >> added. While the machine mode array in tree-streamer still leave 256 as is. >> >> Then, when the MAX_MACHINE_MODE is greater than 256, the memset of >> lto_output_init_mode_table will touch the memory out of range unexpected. > > Uh. :-O > >> This patch would like to take the MAX_MACHINE_MODE as the size of the >> array in streamer, to make sure there is no potential unexpected >> memory access in future. Meanwhile, this patch also adjust some place >> which has MAX_MACHINE_MODE <= 256 assumption. > > Thanks to Jakub and Richard for guidance re the offloading compilation > case, where we've got different 'MAX_MACHINE_MODE's between stream-out > and stream-in, and a modes mapping table. > > However, with this patch, there are ICEs all over the place... I'm > having a look. > > > Grüße > Thomas > > >> gcc/ChangeLog: >> >> * lto-streamer-in.cc (lto_input_mode_table): Stream in the mode >> bits for machine mode table. >> * lto-streamer-out.cc (lto_write_mode_table): Stream out the >> HOST machine mode bits. >> * lto-streamer.h (struct lto_file_decl_data): New fields mode_bits. >> * tree-streamer.cc (streamer_mode_table): Take MAX_MACHINE_MODE >> as the table size. >> * tree-streamer.h (streamer_mode_table): Ditto. >> (bp_pack_machine_mode): Take 1 << ceil_log2 (MAX_MACHINE_MODE) >> as the packing limit. >> (bp_unpack_machine_mode): Ditto. >> --- >> gcc/lto-streamer-in.cc | 12 ++++++++---- >> gcc/lto-streamer-out.cc | 11 ++++++++--- >> gcc/lto-streamer.h | 2 ++ >> gcc/tree-streamer.cc | 2 +- >> gcc/tree-streamer.h | 14 +++++++++----- >> 5 files changed, 28 insertions(+), 13 deletions(-) >> >> diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc >> index 2cb83406db5..2a0720b4e6f 100644 >> --- a/gcc/lto-streamer-in.cc >> +++ b/gcc/lto-streamer-in.cc >> @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >> internal_error ("cannot read LTO mode table from %s", >> file_data->file_name); >> >> - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); >> - file_data->mode_table = table; >> const struct lto_simple_header_with_strings *header >> = (const struct lto_simple_header_with_strings *) data; >> int string_offset; >> @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >> header->string_size, vNULL); >> bitpack_d bp = streamer_read_bitpack (&ib); >> >> + unsigned mode_bits = bp_unpack_value (&bp, 5); >> + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); >> + >> + file_data->mode_table = table; >> + file_data->mode_bits = mode_bits; >> + >> table[VOIDmode] = VOIDmode; >> table[BLKmode] = BLKmode; >> unsigned int m; >> - while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) >> + while ((m = bp_unpack_value (&bp, mode_bits)) != VOIDmode) >> { >> enum mode_class mclass >> = bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS); >> poly_uint16 size = bp_unpack_poly_value (&bp, 16); >> poly_uint16 prec = bp_unpack_poly_value (&bp, 16); >> - machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); >> + machine_mode inner = (machine_mode) bp_unpack_value (&bp, mode_bits); >> poly_uint16 nunits = bp_unpack_poly_value (&bp, 16); >> unsigned int ibit = 0, fbit = 0; >> unsigned int real_fmt_len = 0; >> diff --git a/gcc/lto-streamer-out.cc b/gcc/lto-streamer-out.cc >> index 5ab2eb4301e..36899283ded 100644 >> --- a/gcc/lto-streamer-out.cc >> +++ b/gcc/lto-streamer-out.cc >> @@ -3196,6 +3196,11 @@ lto_write_mode_table (void) >> if (inner_m != m) >> streamer_mode_table[(int) inner_m] = 1; >> } >> + >> + /* Pack the mode_bits value within 5 bits (up to 31) in the beginning. */ >> + unsigned mode_bits = ceil_log2 (MAX_MACHINE_MODE); >> + bp_pack_value (&bp, mode_bits, 5); >> + >> /* First stream modes that have GET_MODE_INNER (m) == m, >> so that we can refer to them afterwards. */ >> for (int pass = 0; pass < 2; pass++) >> @@ -3205,11 +3210,11 @@ lto_write_mode_table (void) >> machine_mode m = (machine_mode) i; >> if ((GET_MODE_INNER (m) == m) ^ (pass == 0)) >> continue; >> - bp_pack_value (&bp, m, 8); >> + bp_pack_value (&bp, m, mode_bits); >> bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m)); >> bp_pack_poly_value (&bp, GET_MODE_SIZE (m), 16); >> bp_pack_poly_value (&bp, GET_MODE_PRECISION (m), 16); >> - bp_pack_value (&bp, GET_MODE_INNER (m), 8); >> + bp_pack_value (&bp, GET_MODE_INNER (m), mode_bits); >> bp_pack_poly_value (&bp, GET_MODE_NUNITS (m), 16); >> switch (GET_MODE_CLASS (m)) >> { >> @@ -3229,7 +3234,7 @@ lto_write_mode_table (void) >> } >> bp_pack_string (ob, &bp, GET_MODE_NAME (m), true); >> } >> - bp_pack_value (&bp, VOIDmode, 8); >> + bp_pack_value (&bp, VOIDmode, mode_bits); >> >> streamer_write_bitpack (&bp); >> >> diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h >> index fc7133d07ba..443f0cd616e 100644 >> --- a/gcc/lto-streamer.h >> +++ b/gcc/lto-streamer.h >> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data >> int order_base; >> >> int unit_base; >> + >> + unsigned mode_bits; >> }; >> >> typedef struct lto_file_decl_data *lto_file_decl_data_ptr; >> diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc >> index ed65a7692e3..a28ef9c7920 100644 >> --- a/gcc/tree-streamer.cc >> +++ b/gcc/tree-streamer.cc >> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see >> During streaming in, we translate the on the disk mode using this >> table. For normal LTO it is set to identity, for ACCEL_COMPILER >> depending on the mode_table content. */ >> -unsigned char streamer_mode_table[1 << 8]; >> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; >> >> /* Check that all the TS_* structures handled by the streamer_write_* and >> streamer_read_* routines are exactly ALL the structures defined in >> diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h >> index 170d61cf20b..ff8bccf901a 100644 >> --- a/gcc/tree-streamer.h >> +++ b/gcc/tree-streamer.h >> @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); >> void streamer_write_integer_cst (struct output_block *, tree); >> >> /* In tree-streamer.cc. */ >> -extern unsigned char streamer_mode_table[1 << 8]; >> +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; >> void streamer_check_handled_ts_structures (void); >> bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, >> hashval_t, unsigned *); >> @@ -108,15 +108,19 @@ inline void >> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) >> { >> streamer_mode_table[mode] = 1; >> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); >> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >> + >> + bp_pack_enum (bp, machine_mode, last, mode); >> } >> >> inline machine_mode >> bp_unpack_machine_mode (struct bitpack_d *bp) >> { >> - return (machine_mode) >> - ((class lto_input_block *) >> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; >> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >> + lto_input_block *input_block = (class lto_input_block *) bp->stream; >> + int index = bp_unpack_enum (bp, machine_mode, last); >> + >> + return (machine_mode) input_block->mode_table[index]; >> } >> >> #endif /* GCC_TREE_STREAMER_H */ >> -- >> 2.34.1 ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode 2023-06-29 9:29 ` Thomas Schwinge 2023-06-29 9:33 ` juzhe.zhong @ 2023-06-29 20:14 ` Thomas Schwinge 2023-06-30 1:26 ` juzhe.zhong 2023-06-30 8:23 ` LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block' (was: [PATCH v3] Streamer: Fix out of range memory access of machine mode) Thomas Schwinge 1 sibling, 2 replies; 33+ messages in thread From: Thomas Schwinge @ 2023-06-29 20:14 UTC (permalink / raw) To: Pan Li, gcc-patches, Richard Biener, Jakub Jelinek Cc: juzhe.zhong, rdapp.gcc, jeffreyalaw, yanzhang.wang, kito.cheng, Tobias Burnus [-- Attachment #1: Type: text/plain, Size: 5477 bytes --] Hi! On 2023-06-29T11:29:57+0200, I wrote: > On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> We extend the machine mode from 8 to 16 bits already. But there still >> one placing missing from the streamer. It has one hard coded array >> for the machine code like size 256. >> >> In the lto pass, we memset the array by MAX_MACHINE_MODE count but the >> value of the MAX_MACHINE_MODE will grow as more and more modes are >> added. While the machine mode array in tree-streamer still leave 256 as is. >> >> Then, when the MAX_MACHINE_MODE is greater than 256, the memset of >> lto_output_init_mode_table will touch the memory out of range unexpected. > > Uh. :-O > >> This patch would like to take the MAX_MACHINE_MODE as the size of the >> array in streamer, to make sure there is no potential unexpected >> memory access in future. Meanwhile, this patch also adjust some place >> which has MAX_MACHINE_MODE <= 256 assumption. > > Thanks to Jakub and Richard for guidance re the offloading compilation > case, where we've got different 'MAX_MACHINE_MODE's between stream-out > and stream-in, and a modes mapping table. > > However, with this patch, there are ICEs all over the place... I'm > having a look. Your patch has all the right ideas, there are just a few additional changes necessary. Please merge in the attached "f into Streamer: Fix out of range memory access of machine mode", with 'Co-authored-by: Thomas Schwinge <thomas@codesourcery.com>'. This has already survived compiler-side 'lto.exp' testing and 'check-target-libgomp' with Nvidia GPU offloading; AMD GPU testing is now running (not expecting any bad surprises). Will let you know by (my) tomorrow morning in case there are any more problems. Explanation: >> --- a/gcc/lto-streamer-in.cc >> +++ b/gcc/lto-streamer-in.cc >> @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >> internal_error ("cannot read LTO mode table from %s", >> file_data->file_name); >> >> - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); >> - file_data->mode_table = table; >> const struct lto_simple_header_with_strings *header >> = (const struct lto_simple_header_with_strings *) data; >> int string_offset; >> @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >> header->string_size, vNULL); >> bitpack_d bp = streamer_read_bitpack (&ib); >> >> + unsigned mode_bits = bp_unpack_value (&bp, 5); >> + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); >> + >> + file_data->mode_table = table; >> + file_data->mode_bits = mode_bits; Here, we set 'file_data->mode_bits' for the offloading case (where 'lto_input_mode_table' is called) -- but it's not set for the non-offloading case (where 'lto_input_mode_table' isn't called). (See my 'gcc/lto/lto-common.cc:lto_read_decls' change.) That's "not currently a problem", as 'file_data->mode_bits' isn't used anywhere... >> --- a/gcc/lto-streamer.h >> +++ b/gcc/lto-streamer.h >> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data >> int order_base; >> >> int unit_base; >> + >> + unsigned mode_bits; >> }; >> inline machine_mode >> bp_unpack_machine_mode (struct bitpack_d *bp) >> { >> - return (machine_mode) >> - ((class lto_input_block *) >> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; >> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >> + lto_input_block *input_block = (class lto_input_block *) bp->stream; >> + int index = bp_unpack_enum (bp, machine_mode, last); >> + >> + return (machine_mode) input_block->mode_table[index]; >> } ..., but 'file_data->mode_bits' needs to be considered here, in the stream-in for offloading, where 'file_data->mode_bits' -- that is, the host 'MAX_MACHINE_MODE' -- very likely is different from the offload device 'MAX_MACHINE_MODE'. Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture 'lto_file_decl_data *file_data' instead of just 'unsigned char *mode_table', and adjust all users. That's it. :-) >> --- a/gcc/tree-streamer.h >> +++ b/gcc/tree-streamer.h >> @@ -108,15 +108,19 @@ inline void >> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) >> { >> streamer_mode_table[mode] = 1; >> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); >> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >> + >> + bp_pack_enum (bp, machine_mode, last, mode); >> } That use of 'MAX_MACHINE_MODE' is safe, as that only concerns the stream-out phase. >> --- a/gcc/tree-streamer.cc >> +++ b/gcc/tree-streamer.cc >> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see >> During streaming in, we translate the on the disk mode using this >> table. For normal LTO it is set to identity, for ACCEL_COMPILER >> depending on the mode_table content. */ >> -unsigned char streamer_mode_table[1 << 8]; >> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; Likewise. Grüße Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-f-into-Streamer-Fix-out-of-range-memory-access-of-ma.patch --] [-- Type: text/x-diff, Size: 8995 bytes --] From 88ff74f043235735701f71cdb51a83315f8a3895 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge <thomas@codesourcery.com> Date: Thu, 29 Jun 2023 21:33:06 +0200 Subject: [PATCH] f into Streamer: Fix out of range memory access of machine mode gcc/ * lto-streamer.h (class lto_input_block): Capture 'lto_file_decl_data *file_data' instead of just 'unsigned char *mode_table'. Adjust all users. * tree-streamer.h (bp_unpack_machine_mode): Use 'file_data->mode_bits'. gcc/lto/ * lto-common.cc (lto_read_decls) [!ACCEL_COMPILER]: Initialize 'file_data->mode_bits'. --- gcc/ipa-devirt.cc | 2 +- gcc/ipa-fnsummary.cc | 2 +- gcc/ipa-icf.cc | 2 +- gcc/ipa-modref.cc | 2 +- gcc/ipa-prop.cc | 4 ++-- gcc/ipa-sra.cc | 2 +- gcc/lto-cgraph.cc | 2 +- gcc/lto-section-in.cc | 2 +- gcc/lto-streamer-in.cc | 6 +++--- gcc/lto-streamer.h | 10 +++++----- gcc/lto/lto-common.cc | 3 ++- gcc/tree-streamer.h | 6 +++--- 12 files changed, 22 insertions(+), 21 deletions(-) diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc index 2c61a497cee..87529be4515 100644 --- a/gcc/ipa-devirt.cc +++ b/gcc/ipa-devirt.cc @@ -4147,7 +4147,7 @@ ipa_odr_read_section (struct lto_file_decl_data *file_data, const char *data, class data_in *data_in; lto_input_block ib ((const char *) data + main_offset, header->main_size, - file_data->mode_table); + file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc index a5f5a50c8a5..37c1edc2f3a 100644 --- a/gcc/ipa-fnsummary.cc +++ b/gcc/ipa-fnsummary.cc @@ -4528,7 +4528,7 @@ inline_read_section (struct lto_file_decl_data *file_data, const char *data, unsigned int f_count; lto_input_block ib ((const char *) data + main_offset, header->main_size, - file_data->mode_table); + file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc index cb9f768d85d..836d0914ded 100644 --- a/gcc/ipa-icf.cc +++ b/gcc/ipa-icf.cc @@ -2204,7 +2204,7 @@ sem_item_optimizer::read_section (lto_file_decl_data *file_data, unsigned int count; lto_input_block ib_main ((const char *) data + main_offset, 0, - header->main_size, file_data->mode_table); + header->main_size, file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/ipa-modref.cc b/gcc/ipa-modref.cc index e3196df8aa9..278b2dbd828 100644 --- a/gcc/ipa-modref.cc +++ b/gcc/ipa-modref.cc @@ -3816,7 +3816,7 @@ read_section (struct lto_file_decl_data *file_data, const char *data, unsigned int f_count; lto_input_block ib ((const char *) data + main_offset, header->main_size, - file_data->mode_table); + file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc index 704fe01b02c..8f2119b72e3 100644 --- a/gcc/ipa-prop.cc +++ b/gcc/ipa-prop.cc @@ -5337,7 +5337,7 @@ ipa_prop_read_section (struct lto_file_decl_data *file_data, const char *data, unsigned int count; lto_input_block ib_main ((const char *) data + main_offset, - header->main_size, file_data->mode_table); + header->main_size, file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, @@ -5561,7 +5561,7 @@ read_replacements_section (struct lto_file_decl_data *file_data, unsigned int count; lto_input_block ib_main ((const char *) data + main_offset, - header->main_size, file_data->mode_table); + header->main_size, file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, header->string_size, vNULL); diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc index 21d281a9756..c35e03b7abd 100644 --- a/gcc/ipa-sra.cc +++ b/gcc/ipa-sra.cc @@ -2944,7 +2944,7 @@ isra_read_summary_section (struct lto_file_decl_data *file_data, unsigned int count; lto_input_block ib_main ((const char *) data + main_offset, - header->main_size, file_data->mode_table); + header->main_size, file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/lto-cgraph.cc b/gcc/lto-cgraph.cc index aed5e9ddb18..32c0f5ac6db 100644 --- a/gcc/lto-cgraph.cc +++ b/gcc/lto-cgraph.cc @@ -2174,7 +2174,7 @@ input_cgraph_opt_section (struct lto_file_decl_data *file_data, unsigned int count; lto_input_block ib_main ((const char *) data + main_offset, - header->main_size, file_data->mode_table); + header->main_size, file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/lto-section-in.cc b/gcc/lto-section-in.cc index 07cf7326582..5ff00a3c130 100644 --- a/gcc/lto-section-in.cc +++ b/gcc/lto-section-in.cc @@ -262,7 +262,7 @@ lto_create_simple_input_block (struct lto_file_decl_data *file_data, *datar = data; return new lto_input_block (data + main_offset, header->main_size, - file_data->mode_table); + file_data); } diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc index 2a0720b4e6f..1876e1967ec 100644 --- a/gcc/lto-streamer-in.cc +++ b/gcc/lto-streamer-in.cc @@ -1629,11 +1629,11 @@ lto_read_body_or_constructor (struct lto_file_decl_data *file_data, struct symta /* Set up the struct function. */ from = data_in->reader_cache->nodes.length (); lto_input_block ib_main (data + main_offset, header->main_size, - file_data->mode_table); + file_data); if (TREE_CODE (node->decl) == FUNCTION_DECL) { lto_input_block ib_cfg (data + cfg_offset, header->cfg_size, - file_data->mode_table); + file_data); input_function (fn_decl, data_in, &ib_main, &ib_cfg, dyn_cast <cgraph_node *>(node)); } @@ -1954,7 +1954,7 @@ lto_input_toplevel_asms (struct lto_file_decl_data *file_data, int order_base) string_offset = sizeof (*header) + header->main_size; lto_input_block ib (data + sizeof (*header), header->main_size, - file_data->mode_table); + file_data); data_in = lto_data_in_create (file_data, data + string_offset, header->string_size, vNULL); diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index 443f0cd616e..0556b34c837 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -344,14 +344,14 @@ public: /* Special constructor for the string table, it abuses this to do random access but use the uhwi decoder. */ lto_input_block (const char *data_, unsigned int p_, unsigned int len_, - const unsigned char *mode_table_) - : data (data_), mode_table (mode_table_), p (p_), len (len_) {} + const lto_file_decl_data *file_data_) + : data (data_), file_data (file_data_), p (p_), len (len_) {} lto_input_block (const char *data_, unsigned int len_, - const unsigned char *mode_table_) - : data (data_), mode_table (mode_table_), p (0), len (len_) {} + const lto_file_decl_data *file_data_) + : data (data_), file_data (file_data_), p (0), len (len_) {} const char *data; - const unsigned char *mode_table; + const lto_file_decl_data *file_data; unsigned int p; unsigned int len; }; diff --git a/gcc/lto/lto-common.cc b/gcc/lto/lto-common.cc index 537570204b3..973ab791712 100644 --- a/gcc/lto/lto-common.cc +++ b/gcc/lto/lto-common.cc @@ -1880,7 +1880,7 @@ lto_read_decls (struct lto_file_decl_data *decl_data, const void *data, uint32_t num_decl_states; lto_input_block ib_main ((const char *) data + main_offset, - header->main_size, decl_data->mode_table); + header->main_size, decl_data); data_in = lto_data_in_create (decl_data, (const char *) data + string_offset, header->string_size, resolutions); @@ -2275,6 +2275,7 @@ lto_file_finalize (struct lto_file_decl_data *file_data, lto_file *file, lto_input_mode_table (file_data); #else file_data->mode_table = lto_mode_identity_table; + file_data->mode_bits = ceil_log2 (MAX_MACHINE_MODE); #endif data = lto_get_summary_section_data (file_data, LTO_section_decls, &len); diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index ff8bccf901a..db01c8c7678 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -116,11 +116,11 @@ bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) inline machine_mode bp_unpack_machine_mode (struct bitpack_d *bp) { - int last = 1 << ceil_log2 (MAX_MACHINE_MODE); - lto_input_block *input_block = (class lto_input_block *) bp->stream; + lto_input_block *ib = (class lto_input_block *) bp->stream; + int last = 1 << ib->file_data->mode_bits; int index = bp_unpack_enum (bp, machine_mode, last); - return (machine_mode) input_block->mode_table[index]; + return (machine_mode) ib->file_data->mode_table[index]; } #endif /* GCC_TREE_STREAMER_H */ -- 2.34.1 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode 2023-06-29 20:14 ` Thomas Schwinge @ 2023-06-30 1:26 ` juzhe.zhong 2023-06-30 1:39 ` Li, Pan2 2023-06-30 8:23 ` LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block' (was: [PATCH v3] Streamer: Fix out of range memory access of machine mode) Thomas Schwinge 1 sibling, 1 reply; 33+ messages in thread From: juzhe.zhong @ 2023-06-30 1:26 UTC (permalink / raw) To: Thomas Schwinge, pan2.li, gcc-patches, rguenther, jakub Cc: Robin Dapp, jeffreyalaw, yanzhang.wang, kito.cheng, Tobias Burnus [-- Attachment #1: Type: text/plain, Size: 6005 bytes --] Thanks a lot! Really appreciate your help ! That's really helpful for RVV (RISC-V vector). Could you merge your patch after you tested? Thanks. juzhe.zhong@rivai.ai From: Thomas Schwinge Date: 2023-06-30 04:14 To: Pan Li; gcc-patches@gcc.gnu.org; Richard Biener; Jakub Jelinek CC: juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; yanzhang.wang@intel.com; kito.cheng@gmail.com; Tobias Burnus Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode Hi! On 2023-06-29T11:29:57+0200, I wrote: > On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> We extend the machine mode from 8 to 16 bits already. But there still >> one placing missing from the streamer. It has one hard coded array >> for the machine code like size 256. >> >> In the lto pass, we memset the array by MAX_MACHINE_MODE count but the >> value of the MAX_MACHINE_MODE will grow as more and more modes are >> added. While the machine mode array in tree-streamer still leave 256 as is. >> >> Then, when the MAX_MACHINE_MODE is greater than 256, the memset of >> lto_output_init_mode_table will touch the memory out of range unexpected. > > Uh. :-O > >> This patch would like to take the MAX_MACHINE_MODE as the size of the >> array in streamer, to make sure there is no potential unexpected >> memory access in future. Meanwhile, this patch also adjust some place >> which has MAX_MACHINE_MODE <= 256 assumption. > > Thanks to Jakub and Richard for guidance re the offloading compilation > case, where we've got different 'MAX_MACHINE_MODE's between stream-out > and stream-in, and a modes mapping table. > > However, with this patch, there are ICEs all over the place... I'm > having a look. Your patch has all the right ideas, there are just a few additional changes necessary. Please merge in the attached "f into Streamer: Fix out of range memory access of machine mode", with 'Co-authored-by: Thomas Schwinge <thomas@codesourcery.com>'. This has already survived compiler-side 'lto.exp' testing and 'check-target-libgomp' with Nvidia GPU offloading; AMD GPU testing is now running (not expecting any bad surprises). Will let you know by (my) tomorrow morning in case there are any more problems. Explanation: >> --- a/gcc/lto-streamer-in.cc >> +++ b/gcc/lto-streamer-in.cc >> @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >> internal_error ("cannot read LTO mode table from %s", >> file_data->file_name); >> >> - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); >> - file_data->mode_table = table; >> const struct lto_simple_header_with_strings *header >> = (const struct lto_simple_header_with_strings *) data; >> int string_offset; >> @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >> header->string_size, vNULL); >> bitpack_d bp = streamer_read_bitpack (&ib); >> >> + unsigned mode_bits = bp_unpack_value (&bp, 5); >> + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); >> + >> + file_data->mode_table = table; >> + file_data->mode_bits = mode_bits; Here, we set 'file_data->mode_bits' for the offloading case (where 'lto_input_mode_table' is called) -- but it's not set for the non-offloading case (where 'lto_input_mode_table' isn't called). (See my 'gcc/lto/lto-common.cc:lto_read_decls' change.) That's "not currently a problem", as 'file_data->mode_bits' isn't used anywhere... >> --- a/gcc/lto-streamer.h >> +++ b/gcc/lto-streamer.h >> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data >> int order_base; >> >> int unit_base; >> + >> + unsigned mode_bits; >> }; >> inline machine_mode >> bp_unpack_machine_mode (struct bitpack_d *bp) >> { >> - return (machine_mode) >> - ((class lto_input_block *) >> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; >> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >> + lto_input_block *input_block = (class lto_input_block *) bp->stream; >> + int index = bp_unpack_enum (bp, machine_mode, last); >> + >> + return (machine_mode) input_block->mode_table[index]; >> } ..., but 'file_data->mode_bits' needs to be considered here, in the stream-in for offloading, where 'file_data->mode_bits' -- that is, the host 'MAX_MACHINE_MODE' -- very likely is different from the offload device 'MAX_MACHINE_MODE'. Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture 'lto_file_decl_data *file_data' instead of just 'unsigned char *mode_table', and adjust all users. That's it. :-) >> --- a/gcc/tree-streamer.h >> +++ b/gcc/tree-streamer.h >> @@ -108,15 +108,19 @@ inline void >> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) >> { >> streamer_mode_table[mode] = 1; >> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); >> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >> + >> + bp_pack_enum (bp, machine_mode, last, mode); >> } That use of 'MAX_MACHINE_MODE' is safe, as that only concerns the stream-out phase. >> --- a/gcc/tree-streamer.cc >> +++ b/gcc/tree-streamer.cc >> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see >> During streaming in, we translate the on the disk mode using this >> table. For normal LTO it is set to identity, for ACCEL_COMPILER >> depending on the mode_table content. */ >> -unsigned char streamer_mode_table[1 << 8]; >> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; Likewise. Grüße Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode 2023-06-30 1:26 ` juzhe.zhong @ 2023-06-30 1:39 ` Li, Pan2 2023-06-30 8:50 ` [v4] " Thomas Schwinge 0 siblings, 1 reply; 33+ messages in thread From: Li, Pan2 @ 2023-06-30 1:39 UTC (permalink / raw) To: juzhe.zhong, Thomas Schwinge, gcc-patches, rguenther, jakub Cc: Robin Dapp, jeffreyalaw, Wang, Yanzhang, kito.cheng, Tobias Burnus [-- Attachment #1: Type: text/plain, Size: 7102 bytes --] That’s very cool, thanks Thomas for help! Let’s wait the AMD test running result for the final version of the patch. Pan From: juzhe.zhong@rivai.ai <juzhe.zhong@rivai.ai> Sent: Friday, June 30, 2023 9:27 AM To: Thomas Schwinge <thomas@codesourcery.com>; Li, Pan2 <pan2.li@intel.com>; gcc-patches <gcc-patches@gcc.gnu.org>; rguenther <rguenther@suse.de>; jakub <jakub@redhat.com> Cc: Robin Dapp <rdapp.gcc@gmail.com>; jeffreyalaw <jeffreyalaw@gmail.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng <kito.cheng@gmail.com>; Tobias Burnus <tobias@codesourcery.com> Subject: Re: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode Thanks a lot! Really appreciate your help ! That's really helpful for RVV (RISC-V vector). Could you merge your patch after you tested? Thanks. ________________________________ juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai> From: Thomas Schwinge<mailto:thomas@codesourcery.com> Date: 2023-06-30 04:14 To: Pan Li<mailto:pan2.li@intel.com>; gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>; Richard Biener<mailto:rguenther@suse.de>; Jakub Jelinek<mailto:jakub@redhat.com> CC: juzhe.zhong@rivai.ai<mailto:juzhe.zhong@rivai.ai>; rdapp.gcc@gmail.com<mailto:rdapp.gcc@gmail.com>; jeffreyalaw@gmail.com<mailto:jeffreyalaw@gmail.com>; yanzhang.wang@intel.com<mailto:yanzhang.wang@intel.com>; kito.cheng@gmail.com<mailto:kito.cheng@gmail.com>; Tobias Burnus<mailto:tobias@codesourcery.com> Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode Hi! On 2023-06-29T11:29:57+0200, I wrote: > On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>> wrote: >> We extend the machine mode from 8 to 16 bits already. But there still >> one placing missing from the streamer. It has one hard coded array >> for the machine code like size 256. >> >> In the lto pass, we memset the array by MAX_MACHINE_MODE count but the >> value of the MAX_MACHINE_MODE will grow as more and more modes are >> added. While the machine mode array in tree-streamer still leave 256 as is. >> >> Then, when the MAX_MACHINE_MODE is greater than 256, the memset of >> lto_output_init_mode_table will touch the memory out of range unexpected. > > Uh. :-O > >> This patch would like to take the MAX_MACHINE_MODE as the size of the >> array in streamer, to make sure there is no potential unexpected >> memory access in future. Meanwhile, this patch also adjust some place >> which has MAX_MACHINE_MODE <= 256 assumption. > > Thanks to Jakub and Richard for guidance re the offloading compilation > case, where we've got different 'MAX_MACHINE_MODE's between stream-out > and stream-in, and a modes mapping table. > > However, with this patch, there are ICEs all over the place... I'm > having a look. Your patch has all the right ideas, there are just a few additional changes necessary. Please merge in the attached "f into Streamer: Fix out of range memory access of machine mode", with 'Co-authored-by: Thomas Schwinge <thomas@codesourcery.com<mailto:thomas@codesourcery.com>>'. This has already survived compiler-side 'lto.exp' testing and 'check-target-libgomp' with Nvidia GPU offloading; AMD GPU testing is now running (not expecting any bad surprises). Will let you know by (my) tomorrow morning in case there are any more problems. Explanation: >> --- a/gcc/lto-streamer-in.cc >> +++ b/gcc/lto-streamer-in.cc >> @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >> internal_error ("cannot read LTO mode table from %s", >> file_data->file_name); >> >> - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); >> - file_data->mode_table = table; >> const struct lto_simple_header_with_strings *header >> = (const struct lto_simple_header_with_strings *) data; >> int string_offset; >> @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >> header->string_size, vNULL); >> bitpack_d bp = streamer_read_bitpack (&ib); >> >> + unsigned mode_bits = bp_unpack_value (&bp, 5); >> + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); >> + >> + file_data->mode_table = table; >> + file_data->mode_bits = mode_bits; Here, we set 'file_data->mode_bits' for the offloading case (where 'lto_input_mode_table' is called) -- but it's not set for the non-offloading case (where 'lto_input_mode_table' isn't called). (See my 'gcc/lto/lto-common.cc:lto_read_decls' change.) That's "not currently a problem", as 'file_data->mode_bits' isn't used anywhere... >> --- a/gcc/lto-streamer.h >> +++ b/gcc/lto-streamer.h >> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data >> int order_base; >> >> int unit_base; >> + >> + unsigned mode_bits; >> }; >> inline machine_mode >> bp_unpack_machine_mode (struct bitpack_d *bp) >> { >> - return (machine_mode) >> - ((class lto_input_block *) >> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; >> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >> + lto_input_block *input_block = (class lto_input_block *) bp->stream; >> + int index = bp_unpack_enum (bp, machine_mode, last); >> + >> + return (machine_mode) input_block->mode_table[index]; >> } ..., but 'file_data->mode_bits' needs to be considered here, in the stream-in for offloading, where 'file_data->mode_bits' -- that is, the host 'MAX_MACHINE_MODE' -- very likely is different from the offload device 'MAX_MACHINE_MODE'. Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture 'lto_file_decl_data *file_data' instead of just 'unsigned char *mode_table', and adjust all users. That's it. :-) >> --- a/gcc/tree-streamer.h >> +++ b/gcc/tree-streamer.h >> @@ -108,15 +108,19 @@ inline void >> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) >> { >> streamer_mode_table[mode] = 1; >> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); >> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >> + >> + bp_pack_enum (bp, machine_mode, last, mode); >> } That use of 'MAX_MACHINE_MODE' is safe, as that only concerns the stream-out phase. >> --- a/gcc/tree-streamer.cc >> +++ b/gcc/tree-streamer.cc >> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see >> During streaming in, we translate the on the disk mode using this >> table. For normal LTO it is set to identity, for ACCEL_COMPILER >> depending on the mode_table content. */ >> -unsigned char streamer_mode_table[1 << 8]; >> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; Likewise. Grüße Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [v4] Streamer: Fix out of range memory access of machine mode 2023-06-30 1:39 ` Li, Pan2 @ 2023-06-30 8:50 ` Thomas Schwinge 2023-06-30 11:44 ` Li, Pan2 2023-07-04 11:26 ` Richard Biener 0 siblings, 2 replies; 33+ messages in thread From: Thomas Schwinge @ 2023-06-30 8:50 UTC (permalink / raw) To: Pan Li, juzhe.zhong, gcc-patches, Richard Biener, Jakub Jelinek Cc: Robin Dapp, jeffreyalaw, yanzhang.wang, kito.cheng, Tobias Burnus [-- Attachment #1: Type: text/plain, Size: 6578 bytes --] Hi! On 2023-06-30T01:39:39+0000, "Li, Pan2" <pan2.li@intel.com> wrote: > That’s very cool, thanks Thomas for help! :-) > Let’s wait the AMD test running result for the final version of the patch. That's all looking good, too. > From: juzhe.zhong@rivai.ai <juzhe.zhong@rivai.ai> > Sent: Friday, June 30, 2023 9:27 AM > Could you merge your patch after you tested? I've done that, and with (already approved) <https://inbox.sourceware.org/87v8f5uzob.fsf@euler.schwinge.homeip.net> "LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block'" split out, OK to push the attached v4 "Streamer: Fix out of range memory access of machine mode"? Grüße Thomas > From: Thomas Schwinge<mailto:thomas@codesourcery.com> > Date: 2023-06-30 04:14 > Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode > Hi! > > On 2023-06-29T11:29:57+0200, I wrote: >> On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>> wrote: >>> We extend the machine mode from 8 to 16 bits already. But there still >>> one placing missing from the streamer. It has one hard coded array >>> for the machine code like size 256. >>> >>> In the lto pass, we memset the array by MAX_MACHINE_MODE count but the >>> value of the MAX_MACHINE_MODE will grow as more and more modes are >>> added. While the machine mode array in tree-streamer still leave 256 as is. >>> >>> Then, when the MAX_MACHINE_MODE is greater than 256, the memset of >>> lto_output_init_mode_table will touch the memory out of range unexpected. >> >> Uh. :-O >> >>> This patch would like to take the MAX_MACHINE_MODE as the size of the >>> array in streamer, to make sure there is no potential unexpected >>> memory access in future. Meanwhile, this patch also adjust some place >>> which has MAX_MACHINE_MODE <= 256 assumption. >> >> Thanks to Jakub and Richard for guidance re the offloading compilation >> case, where we've got different 'MAX_MACHINE_MODE's between stream-out >> and stream-in, and a modes mapping table. >> >> However, with this patch, there are ICEs all over the place... I'm >> having a look. > > Your patch has all the right ideas, there are just a few additional > changes necessary. Please merge in the attached > "f into Streamer: Fix out of range memory access of machine mode", with > 'Co-authored-by: Thomas Schwinge <thomas@codesourcery.com<mailto:thomas@codesourcery.com>>'. This has > already survived compiler-side 'lto.exp' testing and > 'check-target-libgomp' with Nvidia GPU offloading; AMD GPU testing is now > running (not expecting any bad surprises). Will let you know by (my) > tomorrow morning in case there are any more problems. > > Explanation: > >>> --- a/gcc/lto-streamer-in.cc >>> +++ b/gcc/lto-streamer-in.cc >>> @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >>> internal_error ("cannot read LTO mode table from %s", >>> file_data->file_name); >>> >>> - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); >>> - file_data->mode_table = table; >>> const struct lto_simple_header_with_strings *header >>> = (const struct lto_simple_header_with_strings *) data; >>> int string_offset; >>> @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >>> header->string_size, vNULL); >>> bitpack_d bp = streamer_read_bitpack (&ib); >>> >>> + unsigned mode_bits = bp_unpack_value (&bp, 5); >>> + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); >>> + >>> + file_data->mode_table = table; >>> + file_data->mode_bits = mode_bits; > > Here, we set 'file_data->mode_bits' for the offloading case (where > 'lto_input_mode_table' is called) -- but it's not set for the > non-offloading case (where 'lto_input_mode_table' isn't called). (See my > 'gcc/lto/lto-common.cc:lto_read_decls' change.) That's "not currently a > problem", as 'file_data->mode_bits' isn't used anywhere... > >>> --- a/gcc/lto-streamer.h >>> +++ b/gcc/lto-streamer.h >>> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data >>> int order_base; >>> >>> int unit_base; >>> + >>> + unsigned mode_bits; >>> }; > >>> inline machine_mode >>> bp_unpack_machine_mode (struct bitpack_d *bp) >>> { >>> - return (machine_mode) >>> - ((class lto_input_block *) >>> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; >>> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >>> + lto_input_block *input_block = (class lto_input_block *) bp->stream; >>> + int index = bp_unpack_enum (bp, machine_mode, last); >>> + >>> + return (machine_mode) input_block->mode_table[index]; >>> } > > ..., but 'file_data->mode_bits' needs to be considered here, in the > stream-in for offloading, where 'file_data->mode_bits' -- that is, the > host 'MAX_MACHINE_MODE' -- very likely is different from the offload > device 'MAX_MACHINE_MODE'. > > Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture > 'lto_file_decl_data *file_data' instead of just > 'unsigned char *mode_table', and adjust all users. > > That's it. :-) > >>> --- a/gcc/tree-streamer.h >>> +++ b/gcc/tree-streamer.h > >>> @@ -108,15 +108,19 @@ inline void >>> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) >>> { >>> streamer_mode_table[mode] = 1; >>> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); >>> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >>> + >>> + bp_pack_enum (bp, machine_mode, last, mode); >>> } > > That use of 'MAX_MACHINE_MODE' is safe, as that only concerns the > stream-out phase. > >>> --- a/gcc/tree-streamer.cc >>> +++ b/gcc/tree-streamer.cc >>> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see >>> During streaming in, we translate the on the disk mode using this >>> table. For normal LTO it is set to identity, for ACCEL_COMPILER >>> depending on the mode_table content. */ >>> -unsigned char streamer_mode_table[1 << 8]; >>> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > Likewise. > > > Grüße > Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Streamer-Fix-out-of-range-memory-access-of-machine-m.patch --] [-- Type: text/x-diff, Size: 8029 bytes --] From f61d1ffbf0d1e1aa65dcb96e9b32b4e2fca7ed69 Mon Sep 17 00:00:00 2001 From: Pan Li <pan2.li@intel.com> Date: Wed, 21 Jun 2023 15:58:24 +0800 Subject: [PATCH] Streamer: Fix out of range memory access of machine mode We extend the machine mode from 8 to 16 bits already. But there still one placing missing from the streamer. It has one hard coded array for the machine code like size 256. In the lto pass, we memset the array by MAX_MACHINE_MODE count but the value of the MAX_MACHINE_MODE will grow as more and more modes are added. While the machine mode array in tree-streamer still leave 256 as is. Then, when the MAX_MACHINE_MODE is greater than 256, the memset of lto_output_init_mode_table will touch the memory out of range unexpected. This patch would like to take the MAX_MACHINE_MODE as the size of the array in streamer, to make sure there is no potential unexpected memory access in future. Meanwhile, this patch also adjust some place which has MAX_MACHINE_MODE <= 256 assumption. Care is taken that for offload compilation, we interpret the stream-in data in terms of the host 'MAX_MACHINE_MODE' ('file_data->mode_bits'), which very likely is different from the offload device 'MAX_MACHINE_MODE'. gcc/ * lto-streamer-in.cc (lto_input_mode_table): Stream in the mode bits for machine mode table. * lto-streamer-out.cc (lto_write_mode_table): Stream out the HOST machine mode bits. * lto-streamer.h (struct lto_file_decl_data): New fields mode_bits. * tree-streamer.cc (streamer_mode_table): Take MAX_MACHINE_MODE as the table size. * tree-streamer.h (streamer_mode_table): Ditto. (bp_pack_machine_mode): Take 1 << ceil_log2 (MAX_MACHINE_MODE) as the packing limit. (bp_unpack_machine_mode): Ditto with 'file_data->mode_bits'. gcc/lto/ * lto-common.cc (lto_file_finalize) [!ACCEL_COMPILER]: Initialize 'file_data->mode_bits'. Signed-off-by: Pan Li <pan2.li@intel.com> Co-authored-by: Thomas Schwinge <thomas@codesourcery.com> --- gcc/lto-streamer-in.cc | 12 ++++++++---- gcc/lto-streamer-out.cc | 11 ++++++++--- gcc/lto-streamer.h | 2 ++ gcc/lto/lto-common.cc | 1 + gcc/tree-streamer.cc | 2 +- gcc/tree-streamer.h | 8 +++++--- 6 files changed, 25 insertions(+), 11 deletions(-) diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc index 6e8bc9516a6..1876e1967ec 100644 --- a/gcc/lto-streamer-in.cc +++ b/gcc/lto-streamer-in.cc @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) internal_error ("cannot read LTO mode table from %s", file_data->file_name); - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); - file_data->mode_table = table; const struct lto_simple_header_with_strings *header = (const struct lto_simple_header_with_strings *) data; int string_offset; @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) header->string_size, vNULL); bitpack_d bp = streamer_read_bitpack (&ib); + unsigned mode_bits = bp_unpack_value (&bp, 5); + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); + + file_data->mode_table = table; + file_data->mode_bits = mode_bits; + table[VOIDmode] = VOIDmode; table[BLKmode] = BLKmode; unsigned int m; - while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) + while ((m = bp_unpack_value (&bp, mode_bits)) != VOIDmode) { enum mode_class mclass = bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS); poly_uint16 size = bp_unpack_poly_value (&bp, 16); poly_uint16 prec = bp_unpack_poly_value (&bp, 16); - machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); + machine_mode inner = (machine_mode) bp_unpack_value (&bp, mode_bits); poly_uint16 nunits = bp_unpack_poly_value (&bp, 16); unsigned int ibit = 0, fbit = 0; unsigned int real_fmt_len = 0; diff --git a/gcc/lto-streamer-out.cc b/gcc/lto-streamer-out.cc index 5ab2eb4301e..36899283ded 100644 --- a/gcc/lto-streamer-out.cc +++ b/gcc/lto-streamer-out.cc @@ -3196,6 +3196,11 @@ lto_write_mode_table (void) if (inner_m != m) streamer_mode_table[(int) inner_m] = 1; } + + /* Pack the mode_bits value within 5 bits (up to 31) in the beginning. */ + unsigned mode_bits = ceil_log2 (MAX_MACHINE_MODE); + bp_pack_value (&bp, mode_bits, 5); + /* First stream modes that have GET_MODE_INNER (m) == m, so that we can refer to them afterwards. */ for (int pass = 0; pass < 2; pass++) @@ -3205,11 +3210,11 @@ lto_write_mode_table (void) machine_mode m = (machine_mode) i; if ((GET_MODE_INNER (m) == m) ^ (pass == 0)) continue; - bp_pack_value (&bp, m, 8); + bp_pack_value (&bp, m, mode_bits); bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m)); bp_pack_poly_value (&bp, GET_MODE_SIZE (m), 16); bp_pack_poly_value (&bp, GET_MODE_PRECISION (m), 16); - bp_pack_value (&bp, GET_MODE_INNER (m), 8); + bp_pack_value (&bp, GET_MODE_INNER (m), mode_bits); bp_pack_poly_value (&bp, GET_MODE_NUNITS (m), 16); switch (GET_MODE_CLASS (m)) { @@ -3229,7 +3234,7 @@ lto_write_mode_table (void) } bp_pack_string (ob, &bp, GET_MODE_NAME (m), true); } - bp_pack_value (&bp, VOIDmode, 8); + bp_pack_value (&bp, VOIDmode, mode_bits); streamer_write_bitpack (&bp); diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index 2913b808bde..0556b34c837 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data int order_base; int unit_base; + + unsigned mode_bits; }; typedef struct lto_file_decl_data *lto_file_decl_data_ptr; diff --git a/gcc/lto/lto-common.cc b/gcc/lto/lto-common.cc index c235df8a5f5..973ab791712 100644 --- a/gcc/lto/lto-common.cc +++ b/gcc/lto/lto-common.cc @@ -2275,6 +2275,7 @@ lto_file_finalize (struct lto_file_decl_data *file_data, lto_file *file, lto_input_mode_table (file_data); #else file_data->mode_table = lto_mode_identity_table; + file_data->mode_bits = ceil_log2 (MAX_MACHINE_MODE); #endif data = lto_get_summary_section_data (file_data, LTO_section_decls, &len); diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc index ed65a7692e3..a28ef9c7920 100644 --- a/gcc/tree-streamer.cc +++ b/gcc/tree-streamer.cc @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see During streaming in, we translate the on the disk mode using this table. For normal LTO it is set to identity, for ACCEL_COMPILER depending on the mode_table content. */ -unsigned char streamer_mode_table[1 << 8]; +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; /* Check that all the TS_* structures handled by the streamer_write_* and streamer_read_* routines are exactly ALL the structures defined in diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index 6d565acbbc2..ff49d1ba637 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); void streamer_write_integer_cst (struct output_block *, tree); /* In tree-streamer.cc. */ -extern unsigned char streamer_mode_table[1 << 8]; +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; void streamer_check_handled_ts_structures (void); bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, hashval_t, unsigned *); @@ -108,14 +108,16 @@ inline void bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) { streamer_mode_table[mode] = 1; - bp_pack_enum (bp, machine_mode, 1 << 8, mode); + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); + bp_pack_enum (bp, machine_mode, last, mode); } inline machine_mode bp_unpack_machine_mode (struct bitpack_d *bp) { lto_input_block *ib = (class lto_input_block *) bp->stream; - unsigned ix = bp_unpack_enum (bp, machine_mode, 1 << 8); + int last = 1 << ib->file_data->mode_bits; + unsigned ix = bp_unpack_enum (bp, machine_mode, last); return (machine_mode) ib->file_data->mode_table[ix]; } -- 2.34.1 ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [v4] Streamer: Fix out of range memory access of machine mode 2023-06-30 8:50 ` [v4] " Thomas Schwinge @ 2023-06-30 11:44 ` Li, Pan2 2023-07-04 11:26 ` Richard Biener 1 sibling, 0 replies; 33+ messages in thread From: Li, Pan2 @ 2023-06-30 11:44 UTC (permalink / raw) To: Thomas Schwinge, juzhe.zhong, gcc-patches, Richard Biener, Jakub Jelinek Cc: Robin Dapp, jeffreyalaw, Wang, Yanzhang, kito.cheng, Tobias Burnus Thanks Thomas for make it happen. Then we have 2 patches, right? V4 Streamer and V1 LTO: Capture. Not quite sure if these 2 has some dependencies when commit (I suppose both are well tested and approved). But anything I can do to make some progress please feel free to let me know. Again, very appreciate for the great help from Thomas, it is really save my day! Pan -----Original Message----- From: Thomas Schwinge <thomas@codesourcery.com> Sent: Friday, June 30, 2023 4:50 PM To: Li, Pan2 <pan2.li@intel.com>; juzhe.zhong@rivai.ai; gcc-patches@gcc.gnu.org; Richard Biener <rguenther@suse.de>; Jakub Jelinek <jakub@redhat.com> Cc: Robin Dapp <rdapp.gcc@gmail.com>; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; Tobias Burnus <tobias@codesourcery.com> Subject: [v4] Streamer: Fix out of range memory access of machine mode Hi! On 2023-06-30T01:39:39+0000, "Li, Pan2" <pan2.li@intel.com> wrote: > That’s very cool, thanks Thomas for help! :-) > Let’s wait the AMD test running result for the final version of the patch. That's all looking good, too. > From: juzhe.zhong@rivai.ai <juzhe.zhong@rivai.ai> > Sent: Friday, June 30, 2023 9:27 AM > Could you merge your patch after you tested? I've done that, and with (already approved) <https://inbox.sourceware.org/87v8f5uzob.fsf@euler.schwinge.homeip.net> "LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block'" split out, OK to push the attached v4 "Streamer: Fix out of range memory access of machine mode"? Grüße Thomas > From: Thomas Schwinge<mailto:thomas@codesourcery.com> > Date: 2023-06-30 04:14 > Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode > Hi! > > On 2023-06-29T11:29:57+0200, I wrote: >> On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>> wrote: >>> We extend the machine mode from 8 to 16 bits already. But there still >>> one placing missing from the streamer. It has one hard coded array >>> for the machine code like size 256. >>> >>> In the lto pass, we memset the array by MAX_MACHINE_MODE count but the >>> value of the MAX_MACHINE_MODE will grow as more and more modes are >>> added. While the machine mode array in tree-streamer still leave 256 as is. >>> >>> Then, when the MAX_MACHINE_MODE is greater than 256, the memset of >>> lto_output_init_mode_table will touch the memory out of range unexpected. >> >> Uh. :-O >> >>> This patch would like to take the MAX_MACHINE_MODE as the size of the >>> array in streamer, to make sure there is no potential unexpected >>> memory access in future. Meanwhile, this patch also adjust some place >>> which has MAX_MACHINE_MODE <= 256 assumption. >> >> Thanks to Jakub and Richard for guidance re the offloading compilation >> case, where we've got different 'MAX_MACHINE_MODE's between stream-out >> and stream-in, and a modes mapping table. >> >> However, with this patch, there are ICEs all over the place... I'm >> having a look. > > Your patch has all the right ideas, there are just a few additional > changes necessary. Please merge in the attached > "f into Streamer: Fix out of range memory access of machine mode", with > 'Co-authored-by: Thomas Schwinge <thomas@codesourcery.com<mailto:thomas@codesourcery.com>>'. This has > already survived compiler-side 'lto.exp' testing and > 'check-target-libgomp' with Nvidia GPU offloading; AMD GPU testing is now > running (not expecting any bad surprises). Will let you know by (my) > tomorrow morning in case there are any more problems. > > Explanation: > >>> --- a/gcc/lto-streamer-in.cc >>> +++ b/gcc/lto-streamer-in.cc >>> @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >>> internal_error ("cannot read LTO mode table from %s", >>> file_data->file_name); >>> >>> - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); >>> - file_data->mode_table = table; >>> const struct lto_simple_header_with_strings *header >>> = (const struct lto_simple_header_with_strings *) data; >>> int string_offset; >>> @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) >>> header->string_size, vNULL); >>> bitpack_d bp = streamer_read_bitpack (&ib); >>> >>> + unsigned mode_bits = bp_unpack_value (&bp, 5); >>> + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); >>> + >>> + file_data->mode_table = table; >>> + file_data->mode_bits = mode_bits; > > Here, we set 'file_data->mode_bits' for the offloading case (where > 'lto_input_mode_table' is called) -- but it's not set for the > non-offloading case (where 'lto_input_mode_table' isn't called). (See my > 'gcc/lto/lto-common.cc:lto_read_decls' change.) That's "not currently a > problem", as 'file_data->mode_bits' isn't used anywhere... > >>> --- a/gcc/lto-streamer.h >>> +++ b/gcc/lto-streamer.h >>> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data >>> int order_base; >>> >>> int unit_base; >>> + >>> + unsigned mode_bits; >>> }; > >>> inline machine_mode >>> bp_unpack_machine_mode (struct bitpack_d *bp) >>> { >>> - return (machine_mode) >>> - ((class lto_input_block *) >>> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; >>> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >>> + lto_input_block *input_block = (class lto_input_block *) bp->stream; >>> + int index = bp_unpack_enum (bp, machine_mode, last); >>> + >>> + return (machine_mode) input_block->mode_table[index]; >>> } > > ..., but 'file_data->mode_bits' needs to be considered here, in the > stream-in for offloading, where 'file_data->mode_bits' -- that is, the > host 'MAX_MACHINE_MODE' -- very likely is different from the offload > device 'MAX_MACHINE_MODE'. > > Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture > 'lto_file_decl_data *file_data' instead of just > 'unsigned char *mode_table', and adjust all users. > > That's it. :-) > >>> --- a/gcc/tree-streamer.h >>> +++ b/gcc/tree-streamer.h > >>> @@ -108,15 +108,19 @@ inline void >>> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) >>> { >>> streamer_mode_table[mode] = 1; >>> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); >>> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); >>> + >>> + bp_pack_enum (bp, machine_mode, last, mode); >>> } > > That use of 'MAX_MACHINE_MODE' is safe, as that only concerns the > stream-out phase. > >>> --- a/gcc/tree-streamer.cc >>> +++ b/gcc/tree-streamer.cc >>> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see >>> During streaming in, we translate the on the disk mode using this >>> table. For normal LTO it is set to identity, for ACCEL_COMPILER >>> depending on the mode_table content. */ >>> -unsigned char streamer_mode_table[1 << 8]; >>> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > Likewise. > > > Grüße > Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [v4] Streamer: Fix out of range memory access of machine mode 2023-06-30 8:50 ` [v4] " Thomas Schwinge 2023-06-30 11:44 ` Li, Pan2 @ 2023-07-04 11:26 ` Richard Biener 2023-07-04 12:40 ` Li, Pan2 1 sibling, 1 reply; 33+ messages in thread From: Richard Biener @ 2023-07-04 11:26 UTC (permalink / raw) To: Thomas Schwinge Cc: Pan Li, juzhe.zhong, gcc-patches, Jakub Jelinek, Robin Dapp, jeffreyalaw, yanzhang.wang, kito.cheng, Tobias Burnus On Fri, 30 Jun 2023, Thomas Schwinge wrote: > Hi! > > On 2023-06-30T01:39:39+0000, "Li, Pan2" <pan2.li@intel.com> wrote: > > That?s very cool, thanks Thomas for help! > > :-) > > > Let?s wait the AMD test running result for the final version of the patch. > > That's all looking good, too. > > > From: juzhe.zhong@rivai.ai <juzhe.zhong@rivai.ai> > > Sent: Friday, June 30, 2023 9:27 AM > > > Could you merge your patch after you tested? > > I've done that, and with (already approved) > <https://inbox.sourceware.org/87v8f5uzob.fsf@euler.schwinge.homeip.net> > "LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block'" > split out, OK to push the attached > v4 "Streamer: Fix out of range memory access of machine mode"? OK. Thanks, Richard. > > Gr??e > Thomas > > > > From: Thomas Schwinge<mailto:thomas@codesourcery.com> > > Date: 2023-06-30 04:14 > > > Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode > > Hi! > > > > On 2023-06-29T11:29:57+0200, I wrote: > >> On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>> wrote: > >>> We extend the machine mode from 8 to 16 bits already. But there still > >>> one placing missing from the streamer. It has one hard coded array > >>> for the machine code like size 256. > >>> > >>> In the lto pass, we memset the array by MAX_MACHINE_MODE count but the > >>> value of the MAX_MACHINE_MODE will grow as more and more modes are > >>> added. While the machine mode array in tree-streamer still leave 256 as is. > >>> > >>> Then, when the MAX_MACHINE_MODE is greater than 256, the memset of > >>> lto_output_init_mode_table will touch the memory out of range unexpected. > >> > >> Uh. :-O > >> > >>> This patch would like to take the MAX_MACHINE_MODE as the size of the > >>> array in streamer, to make sure there is no potential unexpected > >>> memory access in future. Meanwhile, this patch also adjust some place > >>> which has MAX_MACHINE_MODE <= 256 assumption. > >> > >> Thanks to Jakub and Richard for guidance re the offloading compilation > >> case, where we've got different 'MAX_MACHINE_MODE's between stream-out > >> and stream-in, and a modes mapping table. > >> > >> However, with this patch, there are ICEs all over the place... I'm > >> having a look. > > > > Your patch has all the right ideas, there are just a few additional > > changes necessary. Please merge in the attached > > "f into Streamer: Fix out of range memory access of machine mode", with > > 'Co-authored-by: Thomas Schwinge <thomas@codesourcery.com<mailto:thomas@codesourcery.com>>'. This has > > already survived compiler-side 'lto.exp' testing and > > 'check-target-libgomp' with Nvidia GPU offloading; AMD GPU testing is now > > running (not expecting any bad surprises). Will let you know by (my) > > tomorrow morning in case there are any more problems. > > > > Explanation: > > > >>> --- a/gcc/lto-streamer-in.cc > >>> +++ b/gcc/lto-streamer-in.cc > >>> @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > >>> internal_error ("cannot read LTO mode table from %s", > >>> file_data->file_name); > >>> > >>> - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); > >>> - file_data->mode_table = table; > >>> const struct lto_simple_header_with_strings *header > >>> = (const struct lto_simple_header_with_strings *) data; > >>> int string_offset; > >>> @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > >>> header->string_size, vNULL); > >>> bitpack_d bp = streamer_read_bitpack (&ib); > >>> > >>> + unsigned mode_bits = bp_unpack_value (&bp, 5); > >>> + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); > >>> + > >>> + file_data->mode_table = table; > >>> + file_data->mode_bits = mode_bits; > > > > Here, we set 'file_data->mode_bits' for the offloading case (where > > 'lto_input_mode_table' is called) -- but it's not set for the > > non-offloading case (where 'lto_input_mode_table' isn't called). (See my > > 'gcc/lto/lto-common.cc:lto_read_decls' change.) That's "not currently a > > problem", as 'file_data->mode_bits' isn't used anywhere... > > > >>> --- a/gcc/lto-streamer.h > >>> +++ b/gcc/lto-streamer.h > >>> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data > >>> int order_base; > >>> > >>> int unit_base; > >>> + > >>> + unsigned mode_bits; > >>> }; > > > >>> inline machine_mode > >>> bp_unpack_machine_mode (struct bitpack_d *bp) > >>> { > >>> - return (machine_mode) > >>> - ((class lto_input_block *) > >>> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > >>> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > >>> + lto_input_block *input_block = (class lto_input_block *) bp->stream; > >>> + int index = bp_unpack_enum (bp, machine_mode, last); > >>> + > >>> + return (machine_mode) input_block->mode_table[index]; > >>> } > > > > ..., but 'file_data->mode_bits' needs to be considered here, in the > > stream-in for offloading, where 'file_data->mode_bits' -- that is, the > > host 'MAX_MACHINE_MODE' -- very likely is different from the offload > > device 'MAX_MACHINE_MODE'. > > > > Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture > > 'lto_file_decl_data *file_data' instead of just > > 'unsigned char *mode_table', and adjust all users. > > > > That's it. :-) > > > >>> --- a/gcc/tree-streamer.h > >>> +++ b/gcc/tree-streamer.h > > > >>> @@ -108,15 +108,19 @@ inline void > >>> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) > >>> { > >>> streamer_mode_table[mode] = 1; > >>> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); > >>> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > >>> + > >>> + bp_pack_enum (bp, machine_mode, last, mode); > >>> } > > > > That use of 'MAX_MACHINE_MODE' is safe, as that only concerns the > > stream-out phase. > > > >>> --- a/gcc/tree-streamer.cc > >>> +++ b/gcc/tree-streamer.cc > >>> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see > >>> During streaming in, we translate the on the disk mode using this > >>> table. For normal LTO it is set to identity, for ACCEL_COMPILER > >>> depending on the mode_table content. */ > >>> -unsigned char streamer_mode_table[1 << 8]; > >>> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > > > Likewise. > > > > > > Gr??e > > Thomas > > > ----------------- > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra?e 201, 80634 M?nchen; Gesellschaft mit beschr?nkter Haftung; Gesch?ftsf?hrer: Thomas Heurung, Frank Th?rauf; Sitz der Gesellschaft: M?nchen; Registergericht M?nchen, HRB 106955 > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [v4] Streamer: Fix out of range memory access of machine mode 2023-07-04 11:26 ` Richard Biener @ 2023-07-04 12:40 ` Li, Pan2 0 siblings, 0 replies; 33+ messages in thread From: Li, Pan2 @ 2023-07-04 12:40 UTC (permalink / raw) To: Richard Biener, Thomas Schwinge Cc: juzhe.zhong, gcc-patches, Jakub Jelinek, Robin Dapp, jeffreyalaw, Wang, Yanzhang, kito.cheng, Tobias Burnus Thanks Richard for reviewing. Hi Thomas, Looks Richard approved the V4 version of Streamer, could you please help to install it? Or I can help if you are in the middle of something. Thanks again for testing and fixing this issue entirely. Pan -----Original Message----- From: Richard Biener <rguenther@suse.de> Sent: Tuesday, July 4, 2023 7:26 PM To: Thomas Schwinge <thomas@codesourcery.com> Cc: Li, Pan2 <pan2.li@intel.com>; juzhe.zhong@rivai.ai; gcc-patches@gcc.gnu.org; Jakub Jelinek <jakub@redhat.com>; Robin Dapp <rdapp.gcc@gmail.com>; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; Tobias Burnus <tobias@codesourcery.com> Subject: Re: [v4] Streamer: Fix out of range memory access of machine mode On Fri, 30 Jun 2023, Thomas Schwinge wrote: > Hi! > > On 2023-06-30T01:39:39+0000, "Li, Pan2" <pan2.li@intel.com> wrote: > > That?s very cool, thanks Thomas for help! > > :-) > > > Let?s wait the AMD test running result for the final version of the patch. > > That's all looking good, too. > > > From: juzhe.zhong@rivai.ai <juzhe.zhong@rivai.ai> > > Sent: Friday, June 30, 2023 9:27 AM > > > Could you merge your patch after you tested? > > I've done that, and with (already approved) > <https://inbox.sourceware.org/87v8f5uzob.fsf@euler.schwinge.homeip.net> > "LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block'" > split out, OK to push the attached > v4 "Streamer: Fix out of range memory access of machine mode"? OK. Thanks, Richard. > > Gr??e > Thomas > > > > From: Thomas Schwinge<mailto:thomas@codesourcery.com> > > Date: 2023-06-30 04:14 > > > Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode > > Hi! > > > > On 2023-06-29T11:29:57+0200, I wrote: > >> On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches <gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>> wrote: > >>> We extend the machine mode from 8 to 16 bits already. But there still > >>> one placing missing from the streamer. It has one hard coded array > >>> for the machine code like size 256. > >>> > >>> In the lto pass, we memset the array by MAX_MACHINE_MODE count but the > >>> value of the MAX_MACHINE_MODE will grow as more and more modes are > >>> added. While the machine mode array in tree-streamer still leave 256 as is. > >>> > >>> Then, when the MAX_MACHINE_MODE is greater than 256, the memset of > >>> lto_output_init_mode_table will touch the memory out of range unexpected. > >> > >> Uh. :-O > >> > >>> This patch would like to take the MAX_MACHINE_MODE as the size of the > >>> array in streamer, to make sure there is no potential unexpected > >>> memory access in future. Meanwhile, this patch also adjust some place > >>> which has MAX_MACHINE_MODE <= 256 assumption. > >> > >> Thanks to Jakub and Richard for guidance re the offloading compilation > >> case, where we've got different 'MAX_MACHINE_MODE's between stream-out > >> and stream-in, and a modes mapping table. > >> > >> However, with this patch, there are ICEs all over the place... I'm > >> having a look. > > > > Your patch has all the right ideas, there are just a few additional > > changes necessary. Please merge in the attached > > "f into Streamer: Fix out of range memory access of machine mode", with > > 'Co-authored-by: Thomas Schwinge <thomas@codesourcery.com<mailto:thomas@codesourcery.com>>'. This has > > already survived compiler-side 'lto.exp' testing and > > 'check-target-libgomp' with Nvidia GPU offloading; AMD GPU testing is now > > running (not expecting any bad surprises). Will let you know by (my) > > tomorrow morning in case there are any more problems. > > > > Explanation: > > > >>> --- a/gcc/lto-streamer-in.cc > >>> +++ b/gcc/lto-streamer-in.cc > >>> @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > >>> internal_error ("cannot read LTO mode table from %s", > >>> file_data->file_name); > >>> > >>> - unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8); > >>> - file_data->mode_table = table; > >>> const struct lto_simple_header_with_strings *header > >>> = (const struct lto_simple_header_with_strings *) data; > >>> int string_offset; > >>> @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) > >>> header->string_size, vNULL); > >>> bitpack_d bp = streamer_read_bitpack (&ib); > >>> > >>> + unsigned mode_bits = bp_unpack_value (&bp, 5); > >>> + unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << mode_bits); > >>> + > >>> + file_data->mode_table = table; > >>> + file_data->mode_bits = mode_bits; > > > > Here, we set 'file_data->mode_bits' for the offloading case (where > > 'lto_input_mode_table' is called) -- but it's not set for the > > non-offloading case (where 'lto_input_mode_table' isn't called). (See my > > 'gcc/lto/lto-common.cc:lto_read_decls' change.) That's "not currently a > > problem", as 'file_data->mode_bits' isn't used anywhere... > > > >>> --- a/gcc/lto-streamer.h > >>> +++ b/gcc/lto-streamer.h > >>> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data > >>> int order_base; > >>> > >>> int unit_base; > >>> + > >>> + unsigned mode_bits; > >>> }; > > > >>> inline machine_mode > >>> bp_unpack_machine_mode (struct bitpack_d *bp) > >>> { > >>> - return (machine_mode) > >>> - ((class lto_input_block *) > >>> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > >>> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > >>> + lto_input_block *input_block = (class lto_input_block *) bp->stream; > >>> + int index = bp_unpack_enum (bp, machine_mode, last); > >>> + > >>> + return (machine_mode) input_block->mode_table[index]; > >>> } > > > > ..., but 'file_data->mode_bits' needs to be considered here, in the > > stream-in for offloading, where 'file_data->mode_bits' -- that is, the > > host 'MAX_MACHINE_MODE' -- very likely is different from the offload > > device 'MAX_MACHINE_MODE'. > > > > Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture > > 'lto_file_decl_data *file_data' instead of just > > 'unsigned char *mode_table', and adjust all users. > > > > That's it. :-) > > > >>> --- a/gcc/tree-streamer.h > >>> +++ b/gcc/tree-streamer.h > > > >>> @@ -108,15 +108,19 @@ inline void > >>> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) > >>> { > >>> streamer_mode_table[mode] = 1; > >>> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); > >>> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > >>> + > >>> + bp_pack_enum (bp, machine_mode, last, mode); > >>> } > > > > That use of 'MAX_MACHINE_MODE' is safe, as that only concerns the > > stream-out phase. > > > >>> --- a/gcc/tree-streamer.cc > >>> +++ b/gcc/tree-streamer.cc > >>> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see > >>> During streaming in, we translate the on the disk mode using this > >>> table. For normal LTO it is set to identity, for ACCEL_COMPILER > >>> depending on the mode_table content. */ > >>> -unsigned char streamer_mode_table[1 << 8]; > >>> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > > > Likewise. > > > > > > Gr??e > > Thomas > > > ----------------- > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra?e 201, 80634 M?nchen; Gesellschaft mit beschr?nkter Haftung; Gesch?ftsf?hrer: Thomas Heurung, Frank Th?rauf; Sitz der Gesellschaft: M?nchen; Registergericht M?nchen, HRB 106955 > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 33+ messages in thread
* LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block' (was: [PATCH v3] Streamer: Fix out of range memory access of machine mode) 2023-06-29 20:14 ` Thomas Schwinge 2023-06-30 1:26 ` juzhe.zhong @ 2023-06-30 8:23 ` Thomas Schwinge 2023-06-30 8:39 ` Richard Biener 1 sibling, 1 reply; 33+ messages in thread From: Thomas Schwinge @ 2023-06-30 8:23 UTC (permalink / raw) To: gcc-patches, Richard Biener, Jakub Jelinek Cc: Pan Li, juzhe.zhong, rdapp.gcc, jeffreyalaw, yanzhang.wang, kito.cheng, Tobias Burnus [-- Attachment #1: Type: text/plain, Size: 770 bytes --] Hi! On 2023-06-29T22:14:59+0200, I wrote: > [the new] 'file_data->mode_bits' needs to be considered [somewhere] > > Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture > 'lto_file_decl_data *file_data' instead of just > 'unsigned char *mode_table', and adjust all users. I've split this out as a preparational "no change in behavior" patch; is "LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block'" OK to push, see attached? Grüße Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-LTO-Capture-lto_file_decl_data-file_data-in-class-lt.patch --] [-- Type: text/x-diff, Size: 9233 bytes --] From 1b75a8680bdef16633e3fa2479832a1b71dae43f Mon Sep 17 00:00:00 2001 From: Thomas Schwinge <thomas@codesourcery.com> Date: Thu, 29 Jun 2023 21:33:06 +0200 Subject: [PATCH] LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block' ... instead of just 'unsigned char *mode_table'. Preparation for a forthcoming change, where we need to capture an additional 'file_data' item, so it seems easier to just capture that one proper. gcc/ * lto-streamer.h (class lto_input_block): Capture 'lto_file_decl_data *file_data' instead of just 'unsigned char *mode_table'. * ipa-devirt.cc (ipa_odr_read_section): Adjust. * ipa-fnsummary.cc (inline_read_section): Likewise. * ipa-icf.cc (sem_item_optimizer::read_section): Likewise. * ipa-modref.cc (read_section): Likewise. * ipa-prop.cc (ipa_prop_read_section, read_replacements_section): Likewise. * ipa-sra.cc (isra_read_summary_section): Likewise. * lto-cgraph.cc (input_cgraph_opt_section): Likewise. * lto-section-in.cc (lto_create_simple_input_block): Likewise. * lto-streamer-in.cc (lto_read_body_or_constructor) (lto_input_toplevel_asms): Likewise. * tree-streamer.h (bp_unpack_machine_mode): Likewise. gcc/lto/ * lto-common.cc (lto_read_decls): Adjust. --- gcc/ipa-devirt.cc | 2 +- gcc/ipa-fnsummary.cc | 2 +- gcc/ipa-icf.cc | 2 +- gcc/ipa-modref.cc | 2 +- gcc/ipa-prop.cc | 4 ++-- gcc/ipa-sra.cc | 2 +- gcc/lto-cgraph.cc | 2 +- gcc/lto-section-in.cc | 2 +- gcc/lto-streamer-in.cc | 6 +++--- gcc/lto-streamer.h | 10 +++++----- gcc/lto/lto-common.cc | 2 +- gcc/tree-streamer.h | 6 +++--- 12 files changed, 21 insertions(+), 21 deletions(-) diff --git a/gcc/ipa-devirt.cc b/gcc/ipa-devirt.cc index 2c61a497cee..87529be4515 100644 --- a/gcc/ipa-devirt.cc +++ b/gcc/ipa-devirt.cc @@ -4147,7 +4147,7 @@ ipa_odr_read_section (struct lto_file_decl_data *file_data, const char *data, class data_in *data_in; lto_input_block ib ((const char *) data + main_offset, header->main_size, - file_data->mode_table); + file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc index a5f5a50c8a5..37c1edc2f3a 100644 --- a/gcc/ipa-fnsummary.cc +++ b/gcc/ipa-fnsummary.cc @@ -4528,7 +4528,7 @@ inline_read_section (struct lto_file_decl_data *file_data, const char *data, unsigned int f_count; lto_input_block ib ((const char *) data + main_offset, header->main_size, - file_data->mode_table); + file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc index cb9f768d85d..836d0914ded 100644 --- a/gcc/ipa-icf.cc +++ b/gcc/ipa-icf.cc @@ -2204,7 +2204,7 @@ sem_item_optimizer::read_section (lto_file_decl_data *file_data, unsigned int count; lto_input_block ib_main ((const char *) data + main_offset, 0, - header->main_size, file_data->mode_table); + header->main_size, file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/ipa-modref.cc b/gcc/ipa-modref.cc index e3196df8aa9..278b2dbd828 100644 --- a/gcc/ipa-modref.cc +++ b/gcc/ipa-modref.cc @@ -3816,7 +3816,7 @@ read_section (struct lto_file_decl_data *file_data, const char *data, unsigned int f_count; lto_input_block ib ((const char *) data + main_offset, header->main_size, - file_data->mode_table); + file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc index 704fe01b02c..8f2119b72e3 100644 --- a/gcc/ipa-prop.cc +++ b/gcc/ipa-prop.cc @@ -5337,7 +5337,7 @@ ipa_prop_read_section (struct lto_file_decl_data *file_data, const char *data, unsigned int count; lto_input_block ib_main ((const char *) data + main_offset, - header->main_size, file_data->mode_table); + header->main_size, file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, @@ -5561,7 +5561,7 @@ read_replacements_section (struct lto_file_decl_data *file_data, unsigned int count; lto_input_block ib_main ((const char *) data + main_offset, - header->main_size, file_data->mode_table); + header->main_size, file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, header->string_size, vNULL); diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc index 21d281a9756..c35e03b7abd 100644 --- a/gcc/ipa-sra.cc +++ b/gcc/ipa-sra.cc @@ -2944,7 +2944,7 @@ isra_read_summary_section (struct lto_file_decl_data *file_data, unsigned int count; lto_input_block ib_main ((const char *) data + main_offset, - header->main_size, file_data->mode_table); + header->main_size, file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/lto-cgraph.cc b/gcc/lto-cgraph.cc index aed5e9ddb18..32c0f5ac6db 100644 --- a/gcc/lto-cgraph.cc +++ b/gcc/lto-cgraph.cc @@ -2174,7 +2174,7 @@ input_cgraph_opt_section (struct lto_file_decl_data *file_data, unsigned int count; lto_input_block ib_main ((const char *) data + main_offset, - header->main_size, file_data->mode_table); + header->main_size, file_data); data_in = lto_data_in_create (file_data, (const char *) data + string_offset, diff --git a/gcc/lto-section-in.cc b/gcc/lto-section-in.cc index 07cf7326582..5ff00a3c130 100644 --- a/gcc/lto-section-in.cc +++ b/gcc/lto-section-in.cc @@ -262,7 +262,7 @@ lto_create_simple_input_block (struct lto_file_decl_data *file_data, *datar = data; return new lto_input_block (data + main_offset, header->main_size, - file_data->mode_table); + file_data); } diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc index 2cb83406db5..6e8bc9516a6 100644 --- a/gcc/lto-streamer-in.cc +++ b/gcc/lto-streamer-in.cc @@ -1629,11 +1629,11 @@ lto_read_body_or_constructor (struct lto_file_decl_data *file_data, struct symta /* Set up the struct function. */ from = data_in->reader_cache->nodes.length (); lto_input_block ib_main (data + main_offset, header->main_size, - file_data->mode_table); + file_data); if (TREE_CODE (node->decl) == FUNCTION_DECL) { lto_input_block ib_cfg (data + cfg_offset, header->cfg_size, - file_data->mode_table); + file_data); input_function (fn_decl, data_in, &ib_main, &ib_cfg, dyn_cast <cgraph_node *>(node)); } @@ -1954,7 +1954,7 @@ lto_input_toplevel_asms (struct lto_file_decl_data *file_data, int order_base) string_offset = sizeof (*header) + header->main_size; lto_input_block ib (data + sizeof (*header), header->main_size, - file_data->mode_table); + file_data); data_in = lto_data_in_create (file_data, data + string_offset, header->string_size, vNULL); diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index fc7133d07ba..2913b808bde 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -344,14 +344,14 @@ public: /* Special constructor for the string table, it abuses this to do random access but use the uhwi decoder. */ lto_input_block (const char *data_, unsigned int p_, unsigned int len_, - const unsigned char *mode_table_) - : data (data_), mode_table (mode_table_), p (p_), len (len_) {} + const lto_file_decl_data *file_data_) + : data (data_), file_data (file_data_), p (p_), len (len_) {} lto_input_block (const char *data_, unsigned int len_, - const unsigned char *mode_table_) - : data (data_), mode_table (mode_table_), p (0), len (len_) {} + const lto_file_decl_data *file_data_) + : data (data_), file_data (file_data_), p (0), len (len_) {} const char *data; - const unsigned char *mode_table; + const lto_file_decl_data *file_data; unsigned int p; unsigned int len; }; diff --git a/gcc/lto/lto-common.cc b/gcc/lto/lto-common.cc index 537570204b3..c235df8a5f5 100644 --- a/gcc/lto/lto-common.cc +++ b/gcc/lto/lto-common.cc @@ -1880,7 +1880,7 @@ lto_read_decls (struct lto_file_decl_data *decl_data, const void *data, uint32_t num_decl_states; lto_input_block ib_main ((const char *) data + main_offset, - header->main_size, decl_data->mode_table); + header->main_size, decl_data); data_in = lto_data_in_create (decl_data, (const char *) data + string_offset, header->string_size, resolutions); diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index 170d61cf20b..6d565acbbc2 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -114,9 +114,9 @@ bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) inline machine_mode bp_unpack_machine_mode (struct bitpack_d *bp) { - return (machine_mode) - ((class lto_input_block *) - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; + lto_input_block *ib = (class lto_input_block *) bp->stream; + unsigned ix = bp_unpack_enum (bp, machine_mode, 1 << 8); + return (machine_mode) ib->file_data->mode_table[ix]; } #endif /* GCC_TREE_STREAMER_H */ -- 2.34.1 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block' (was: [PATCH v3] Streamer: Fix out of range memory access of machine mode) 2023-06-30 8:23 ` LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block' (was: [PATCH v3] Streamer: Fix out of range memory access of machine mode) Thomas Schwinge @ 2023-06-30 8:39 ` Richard Biener 0 siblings, 0 replies; 33+ messages in thread From: Richard Biener @ 2023-06-30 8:39 UTC (permalink / raw) To: Thomas Schwinge Cc: gcc-patches, Jakub Jelinek, Pan Li, juzhe.zhong, rdapp.gcc, jeffreyalaw, yanzhang.wang, kito.cheng, Tobias Burnus On Fri, 30 Jun 2023, Thomas Schwinge wrote: > Hi! > > On 2023-06-29T22:14:59+0200, I wrote: > > [the new] 'file_data->mode_bits' needs to be considered [somewhere] > > > > Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture > > 'lto_file_decl_data *file_data' instead of just > > 'unsigned char *mode_table', and adjust all users. > > I've split this out as a preparational "no change in behavior" patch; is > "LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block'" > OK to push, see attached? Yes. Richard. > > Gr??e > Thomas > > > ----------------- > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra?e 201, 80634 M?nchen; Gesellschaft mit beschr?nkter Haftung; Gesch?ftsf?hrer: Thomas Heurung, Frank Th?rauf; Sitz der Gesellschaft: M?nchen; Registergericht M?nchen, HRB 106955 > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2023-07-04 12:40 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-19 8:07 [PATCH v1] RISC-V: Fix out of range memory access when lto mode init pan2.li 2023-06-19 8:16 ` Li, Pan2 2023-06-19 8:40 ` Richard Biener 2023-06-19 9:08 ` Li, Pan2 2023-06-19 9:10 ` Jakub Jelinek 2023-06-19 9:05 ` [PATCH] RISC-V: Fix out of range memory access of machine mode table pan2.li 2023-06-19 9:15 ` Richard Biener 2023-06-19 9:16 ` Jakub Jelinek 2023-06-19 13:35 ` Li, Pan2 2023-06-20 7:50 ` Li, Pan2 2023-06-20 8:03 ` Jakub Jelinek 2023-06-20 14:08 ` Li, Pan2 2023-06-20 15:25 ` Jakub Jelinek 2023-06-21 6:59 ` Li, Pan2 2023-06-21 7:16 ` Jakub Jelinek 2023-06-21 7:23 ` Li, Pan2 2023-06-22 0:19 ` Li, Pan2 2023-06-28 18:37 ` Jeff Law 2023-06-21 7:58 ` [PATCH v3] Streamer: Fix out of range memory access of machine mode pan2.li 2023-06-22 15:26 ` Li, Pan2 2023-06-29 9:29 ` Thomas Schwinge 2023-06-29 9:33 ` juzhe.zhong 2023-06-29 9:47 ` Thomas Schwinge 2023-06-29 9:52 ` juzhe.zhong 2023-06-29 20:14 ` Thomas Schwinge 2023-06-30 1:26 ` juzhe.zhong 2023-06-30 1:39 ` Li, Pan2 2023-06-30 8:50 ` [v4] " Thomas Schwinge 2023-06-30 11:44 ` Li, Pan2 2023-07-04 11:26 ` Richard Biener 2023-07-04 12:40 ` Li, Pan2 2023-06-30 8:23 ` LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block' (was: [PATCH v3] Streamer: Fix out of range memory access of machine mode) Thomas Schwinge 2023-06-30 8:39 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).