[v3] Streamer: Fix out of range memory access of machine mode
Checks
Commit Message
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(-)
Comments
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
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
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
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
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
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
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
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
@@ -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;
@@ -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);
@@ -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;
@@ -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
@@ -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 */