From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 2A63B3856965 for ; Tue, 4 Jul 2023 11:26:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2A63B3856965 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 616C3227BD; Tue, 4 Jul 2023 11:26:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1688469989; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=cGZ5LEF55Wm6/Gxn4Cza3CFPIJVxFb2FDd29JC+UKQU=; b=Mv+eDhjTD8DIQ+8uEZ1kUyQMLru4uOk7NgEilC8QdxPyLuPDnOPo+rdMaDR4MW+TguOYUt Eh5lU/MamKhUlh0GYtec2uvK42dkbL50b4P90+7K9q3xhyqCx7lhD2kd5JNm2Z3yd68Xok b+R0PUI3V4NB/V/Nx+NB+StCpqzfX74= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1688469989; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=cGZ5LEF55Wm6/Gxn4Cza3CFPIJVxFb2FDd29JC+UKQU=; b=UGN3soUNr/wqZj9xq/JVMj6PWnYeYddd5Lne7OUnwmpov0IqSiHsrpwO2Eb7vrgJVRapAS ju2NveNLsMi+UQBQ== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id F20AB2C141; Tue, 4 Jul 2023 11:26:28 +0000 (UTC) Date: Tue, 4 Jul 2023 11:26:28 +0000 (UTC) From: Richard Biener To: Thomas Schwinge cc: Pan Li , juzhe.zhong@rivai.ai, gcc-patches@gcc.gnu.org, Jakub Jelinek , Robin Dapp , jeffreyalaw@gmail.com, yanzhang.wang@intel.com, kito.cheng@gmail.com, Tobias Burnus Subject: Re: [v4] Streamer: Fix out of range memory access of machine mode In-Reply-To: <87mt0hcp12.fsf@euler.schwinge.homeip.net> Message-ID: References: <20230619080710.1536456-1-pan2.li@intel.com> <20230621075824.1990571-1-pan2.li@intel.com> <874jmqwr8q.fsf@euler.schwinge.homeip.net> <87sfaauit8.fsf@dem-tschwing-1.ger.mentorg.com> <24F5CA0EC7859D5E+2023063009263644698181@rivai.ai> <87mt0hcp12.fsf@euler.schwinge.homeip.net> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, 30 Jun 2023, Thomas Schwinge wrote: > Hi! > > On 2023-06-30T01:39:39+0000, "Li, Pan2" wrote: > > That?s very cool, thanks Thomas for help! > > :-) > > > Let?s wait the AMD test running result for the final version of the patch. > > That's all looking good, too. > > > From: juzhe.zhong@rivai.ai > > Sent: Friday, June 30, 2023 9:27 AM > > > Could you merge your patch after you tested? > > I've done that, and with (already approved) > > "LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block'" > split out, OK to push the attached > v4 "Streamer: Fix out of range memory access of machine mode"? OK. Thanks, Richard. > > Gr??e > Thomas > > > > From: Thomas Schwinge > > Date: 2023-06-30 04:14 > > > Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machine mode > > Hi! > > > > On 2023-06-29T11:29:57+0200, I wrote: > >> On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches > 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 >'. 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 (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 (1 << mode_bits); > >>> + > >>> + file_data->mode_table = table; > >>> + file_data->mode_bits = mode_bits; > > > > Here, we set 'file_data->mode_bits' for the offloading case (where > > 'lto_input_mode_table' is called) -- but it's not set for the > > non-offloading case (where 'lto_input_mode_table' isn't called). (See my > > 'gcc/lto/lto-common.cc:lto_read_decls' change.) That's "not currently a > > problem", as 'file_data->mode_bits' isn't used anywhere... > > > >>> --- a/gcc/lto-streamer.h > >>> +++ b/gcc/lto-streamer.h > >>> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data > >>> int order_base; > >>> > >>> int unit_base; > >>> + > >>> + unsigned mode_bits; > >>> }; > > > >>> inline machine_mode > >>> bp_unpack_machine_mode (struct bitpack_d *bp) > >>> { > >>> - return (machine_mode) > >>> - ((class lto_input_block *) > >>> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)]; > >>> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > >>> + lto_input_block *input_block = (class lto_input_block *) bp->stream; > >>> + int index = bp_unpack_enum (bp, machine_mode, last); > >>> + > >>> + return (machine_mode) input_block->mode_table[index]; > >>> } > > > > ..., but 'file_data->mode_bits' needs to be considered here, in the > > stream-in for offloading, where 'file_data->mode_bits' -- that is, the > > host 'MAX_MACHINE_MODE' -- very likely is different from the offload > > device 'MAX_MACHINE_MODE'. > > > > Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture > > 'lto_file_decl_data *file_data' instead of just > > 'unsigned char *mode_table', and adjust all users. > > > > That's it. :-) > > > >>> --- a/gcc/tree-streamer.h > >>> +++ b/gcc/tree-streamer.h > > > >>> @@ -108,15 +108,19 @@ inline void > >>> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) > >>> { > >>> streamer_mode_table[mode] = 1; > >>> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); > >>> + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); > >>> + > >>> + bp_pack_enum (bp, machine_mode, last, mode); > >>> } > > > > That use of 'MAX_MACHINE_MODE' is safe, as that only concerns the > > stream-out phase. > > > >>> --- a/gcc/tree-streamer.cc > >>> +++ b/gcc/tree-streamer.cc > >>> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see > >>> During streaming in, we translate the on the disk mode using this > >>> table. For normal LTO it is set to identity, for ACCEL_COMPILER > >>> depending on the mode_table content. */ > >>> -unsigned char streamer_mode_table[1 << 8]; > >>> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > > > Likewise. > > > > > > Gr??e > > Thomas > > > ----------------- > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra?e 201, 80634 M?nchen; Gesellschaft mit beschr?nkter Haftung; Gesch?ftsf?hrer: Thomas Heurung, Frank Th?rauf; Sitz der Gesellschaft: M?nchen; Registergericht M?nchen, HRB 106955 > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)