Re: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode

["juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai>]

[-- 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