From: "juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai>
To: "Thomas Schwinge" <thomas@codesourcery.com>,
pan2.li <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>,
yanzhang.wang <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
Date: Fri, 30 Jun 2023 09:26:37 +0800 [thread overview]
Message-ID: <24F5CA0EC7859D5E+2023063009263644698181@rivai.ai> (raw)
In-Reply-To: <87sfaauit8.fsf@dem-tschwing-1.ger.mentorg.com>
[-- 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
next prev parent reply other threads:[~2023-06-30 1:26 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=24F5CA0EC7859D5E+2023063009263644698181@rivai.ai \
--to=juzhe.zhong@rivai.ai \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=jeffreyalaw@gmail.com \
--cc=kito.cheng@gmail.com \
--cc=pan2.li@intel.com \
--cc=rdapp.gcc@gmail.com \
--cc=rguenther@suse.de \
--cc=thomas@codesourcery.com \
--cc=tobias@codesourcery.com \
--cc=yanzhang.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).