From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5699 invoked by alias); 2 Mar 2017 08:01:10 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 5656 invoked by uid 89); 2 Mar 2017 08:01:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Mar 2017 08:01:07 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 35E62ADE1; Thu, 2 Mar 2017 08:01:05 +0000 (UTC) Date: Thu, 02 Mar 2017 08:01:00 -0000 From: Richard Biener To: Jakub Jelinek cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345) In-Reply-To: <20170301193318.GE1849@tucnak> Message-ID: References: <20170301193318.GE1849@tucnak> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2017-03/txt/msg00075.txt.bz2 On Wed, 1 Mar 2017, Jakub Jelinek wrote: > On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote: > > The patch adds better limiting to that interface and fixes one > > false positive in fixed-value.c. Two other false positives are > > fixed by the wide-int.h patch posted a few hours ago and a patch > > to genemit from Jakub. > > Here is that patch. Right now insn-emit.c for match_scratch > operands in expanders emits > rtx operand7 ATTRIBUTE_UNUSED; > ... > rtx operands[8]; > operands[0] = operand0; > ... > // but no operands[7] = something here; > ... > // C code from *.md file (which typically doesn't touch operands[7]) > ... > operand7 = operands[7]; > (void) operand7; > ... > // generated code that doesn't use operand7 > This triggers -Wuninitialized warning with Richard's patch and is really UB, > even when we actually don't use it and so hopefully optimize away. > The following patch just removes all operand7 and operands[7] references > from the generated code (i.e. operands that are for match_scratch). > > Usually match_scratch numbers come after match_operand/match_dup etc. > numbers, but as can be seen, there are few spots where that is not the case. > The patch adds verification of this requirement in genemit and then fixes > the issues it has diagnosed. > > Bootstrapped/regtested on x86_64-linux and i686-linux, plus tested with > make s-emit in a cross to > {powerpc64,aarch64,armv7hl,sparc64,s390x,cris,sh,ia64,hppa,mips}-linux, ok > for trunk? This is ok. Thanks for fixing this, Richard. > 2017-03-01 Jakub Jelinek > > PR tree-optimization/79345 > * gensupport.h (struct pattern_stats): Add min_scratch_opno field. > * gensupport.c (get_pattern_stats_1) : Update it. > (get_pattern_stats): Initialize it. > * genemit.c (gen_expand): Verify match_scratch numbers come after > match_operand/match_dup numbers. > * config/i386/i386.md (mul3_highpart): Swap match_dup and > match_scratch numbers. > * config/i386/sse.md (avx2_gathersi, avx2_gatherdi): > Likewise. > * config/s390/s390.md (trunctdsd2): Likewise. > > --- gcc/gensupport.h.jj 2017-01-01 12:45:38.000000000 +0100 > +++ gcc/gensupport.h 2017-03-01 12:06:21.816440102 +0100 > @@ -199,7 +199,8 @@ struct pattern_stats > /* The largest match_dup, match_op_dup or match_par_dup number found. */ > int max_dup_opno; > > - /* The largest match_scratch number found. */ > + /* The smallest and largest match_scratch number found. */ > + int min_scratch_opno; > int max_scratch_opno; > > /* The number of times match_dup, match_op_dup or match_par_dup appears > --- gcc/gensupport.c.jj 2017-01-05 22:10:31.000000000 +0100 > +++ gcc/gensupport.c 2017-03-01 12:21:24.830327207 +0100 > @@ -3000,6 +3000,10 @@ get_pattern_stats_1 (struct pattern_stat > break; > > case MATCH_SCRATCH: > + if (stats->min_scratch_opno == -1) > + stats->min_scratch_opno = XINT (x, 0); > + else > + stats->min_scratch_opno = MIN (stats->min_scratch_opno, XINT (x, 0)); > stats->max_scratch_opno = MAX (stats->max_scratch_opno, XINT (x, 0)); > break; > > @@ -3032,6 +3036,7 @@ get_pattern_stats (struct pattern_stats > > stats->max_opno = -1; > stats->max_dup_opno = -1; > + stats->min_scratch_opno = -1; > stats->max_scratch_opno = -1; > stats->num_dups = 0; > > --- gcc/genemit.c.jj 2017-01-01 12:45:35.000000000 +0100 > +++ gcc/genemit.c 2017-03-01 12:16:28.391343302 +0100 > @@ -448,6 +448,10 @@ gen_expand (md_rtx_info *info) > > /* Find out how many operands this function has. */ > get_pattern_stats (&stats, XVEC (expand, 1)); > + if (stats.min_scratch_opno != -1 > + && stats.min_scratch_opno <= MAX (stats.max_opno, stats.max_dup_opno)) > + fatal_at (info->loc, "define_expand for %s needs to have match_scratch " > + "numbers above all other operands", XSTR (expand, 0)); > > /* Output the function name and argument declarations. */ > printf ("rtx\ngen_%s (", XSTR (expand, 0)); > @@ -479,8 +483,6 @@ gen_expand (md_rtx_info *info) > make a local variable. */ > for (i = stats.num_generator_args; i <= stats.max_dup_opno; i++) > printf (" rtx operand%d;\n", i); > - for (; i <= stats.max_scratch_opno; i++) > - printf (" rtx operand%d ATTRIBUTE_UNUSED;\n", i); > printf (" rtx_insn *_val = 0;\n"); > printf (" start_sequence ();\n"); > > @@ -516,7 +518,7 @@ gen_expand (md_rtx_info *info) > (unless we aren't going to use them at all). */ > if (XVEC (expand, 1) != 0) > { > - for (i = 0; i < stats.num_operand_vars; i++) > + for (i = 0; i <= MAX (stats.max_opno, stats.max_dup_opno); i++) > { > printf (" operand%d = operands[%d];\n", i, i); > printf (" (void) operand%d;\n", i); > --- gcc/config/i386/i386.md.jj 2017-02-22 18:15:48.000000000 +0100 > +++ gcc/config/i386/i386.md 2017-03-01 12:23:22.882736837 +0100 > @@ -7364,11 +7364,11 @@ (define_expand "mul3_highpart" > (match_operand:SWI48 1 "nonimmediate_operand")) > (any_extend: > (match_operand:SWI48 2 "register_operand"))) > - (match_dup 4)))) > - (clobber (match_scratch:SWI48 3)) > + (match_dup 3)))) > + (clobber (match_scratch:SWI48 4)) > (clobber (reg:CC FLAGS_REG))])] > "" > - "operands[4] = GEN_INT (GET_MODE_BITSIZE (mode));") > + "operands[3] = GEN_INT (GET_MODE_BITSIZE (mode));") > > (define_insn "*muldi3_highpart_1" > [(set (match_operand:DI 0 "register_operand" "=d") > --- gcc/config/i386/sse.md.jj 2017-02-07 16:41:32.000000000 +0100 > +++ gcc/config/i386/sse.md 2017-03-01 12:24:57.193471018 +0100 > @@ -18873,7 +18873,7 @@ (define_expand "avx2_gathersi" > (unspec:VEC_GATHER_MODE > [(match_operand:VEC_GATHER_MODE 1 "register_operand") > (mem: > - (match_par_dup 7 > + (match_par_dup 6 > [(match_operand 2 "vsib_address_operand") > (match_operand: > 3 "register_operand") > @@ -18881,10 +18881,10 @@ (define_expand "avx2_gathersi" > (mem:BLK (scratch)) > (match_operand:VEC_GATHER_MODE 4 "register_operand")] > UNSPEC_GATHER)) > - (clobber (match_scratch:VEC_GATHER_MODE 6))])] > + (clobber (match_scratch:VEC_GATHER_MODE 7))])] > "TARGET_AVX2" > { > - operands[7] > + operands[6] > = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, operands[2], operands[3], > operands[5]), UNSPEC_VSIBADDR); > }) > @@ -18934,7 +18934,7 @@ (define_expand "avx2_gatherdi" > (unspec:VEC_GATHER_MODE > [(match_operand: 1 "register_operand") > (mem: > - (match_par_dup 7 > + (match_par_dup 6 > [(match_operand 2 "vsib_address_operand") > (match_operand: > 3 "register_operand") > @@ -18942,10 +18942,10 @@ (define_expand "avx2_gatherdi" > (mem:BLK (scratch)) > (match_operand: 4 "register_operand")] > UNSPEC_GATHER)) > - (clobber (match_scratch:VEC_GATHER_MODE 6))])] > + (clobber (match_scratch:VEC_GATHER_MODE 7))])] > "TARGET_AVX2" > { > - operands[7] > + operands[6] > = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, operands[2], operands[3], > operands[5]), UNSPEC_VSIBADDR); > }) > --- gcc/config/s390/s390.md.jj 2017-02-06 13:32:14.000000000 +0100 > +++ gcc/config/s390/s390.md 2017-03-01 12:45:07.184184616 +0100 > @@ -5074,15 +5074,15 @@ (define_insn "truncddsd2" > > (define_expand "trunctdsd2" > [(parallel > - [(set (match_dup 3) > + [(set (match_dup 2) > (float_truncate:DD (match_operand:TD 1 "register_operand" ""))) > (unspec:DI [(const_int DFP_RND_PREP_FOR_SHORT_PREC)] UNSPEC_ROUND) > - (clobber (match_scratch:TD 2 ""))]) > + (clobber (match_scratch:TD 3 ""))]) > (set (match_operand:SD 0 "register_operand" "") > - (float_truncate:SD (match_dup 3)))] > + (float_truncate:SD (match_dup 2)))] > "TARGET_HARD_DFP" > { > - operands[3] = gen_reg_rtx (DDmode); > + operands[2] = gen_reg_rtx (DDmode); > }) > > ; > > > Jakub > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)