Re: [PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345)

[Richard Biener <rguenther at suse dot de>]

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  <jakub@redhat.com>
> 
> 	PR tree-optimization/79345
> 	* gensupport.h (struct pattern_stats): Add min_scratch_opno field.
> 	* gensupport.c (get_pattern_stats_1) <case MATCH_SCRATCH>: 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 (<s>mul<mode>3_highpart): Swap match_dup and
> 	match_scratch numbers.
> 	* config/i386/sse.md (avx2_gathersi<mode>, avx2_gatherdi<mode>):
> 	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 "<s>mul<mode>3_highpart"
>  			   (match_operand:SWI48 1 "nonimmediate_operand"))
>  			 (any_extend:<DWI>
>  			   (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>mode));")
> +  "operands[3] = GEN_INT (GET_MODE_BITSIZE (<MODE>mode));")
>  
>  (define_insn "*<s>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<mode>"
>  		   (unspec:VEC_GATHER_MODE
>  		     [(match_operand:VEC_GATHER_MODE 1 "register_operand")
>  		      (mem:<ssescalarmode>
> -			(match_par_dup 7
> +			(match_par_dup 6
>  			  [(match_operand 2 "vsib_address_operand")
>  			   (match_operand:<VEC_GATHER_IDXSI>
>  			      3 "register_operand")
> @@ -18881,10 +18881,10 @@ (define_expand "avx2_gathersi<mode>"
>  		      (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<mode>"
>  		   (unspec:VEC_GATHER_MODE
>  		     [(match_operand:<VEC_GATHER_SRCDI> 1 "register_operand")
>  		      (mem:<ssescalarmode>
> -			(match_par_dup 7
> +			(match_par_dup 6
>  			  [(match_operand 2 "vsib_address_operand")
>  			   (match_operand:<VEC_GATHER_IDXDI>
>  			      3 "register_operand")
> @@ -18942,10 +18942,10 @@ (define_expand "avx2_gatherdi<mode>"
>  		      (mem:BLK (scratch))
>  		      (match_operand:<VEC_GATHER_SRCDI> 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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)