Patchwork Fix arrays in rtx.u + add minor rtx verification

login
register
mail settings
Submitter Marek Polacek
Date June 20, 2014, 5:36 p.m.
Message ID <20140620173640.GC14420@redhat.com>
Download mbox | patch
Permalink /patch/362308/
State New
Headers show

Comments

Marek Polacek - June 20, 2014, 5:36 p.m.
When implementing -fsanitize=bounds I noticed a whole slew of
errors about accessing u.fld[] field in rtx_def.  Turned out this
is indeed a bug, the array should have a size of 8; u.hwint[] array
had similar issue.  Thus fixed, plus I added some verification code
to genpreds.c (can't do it in gengenrtl.c as that doesn't include
rtl.h) so this won't happen again.

Verified that the bootstrap crashes by bootstrapping with changed
RTX_FLD_WIDTH/RTX_HWINT_WIDTH, otherwise the bootstrapp passes.

Ok for trunk?

2014-06-20  Marek Polacek  <polacek@redhat.com>

	* genpreds.c (verify_rtx_codes): New function.
	(main): Call it.
	* rtl.h (RTX_FLD_WIDTH, RTX_HWINT_WIDTH): Define.
	(struct rtx_def): Use them.


	Marek
Jakub Jelinek - June 20, 2014, 7:01 p.m.
On Fri, Jun 20, 2014 at 07:36:41PM +0200, Marek Polacek wrote:
> 2014-06-20  Marek Polacek  <polacek@redhat.com>
> 
> 	* genpreds.c (verify_rtx_codes): New function.
> 	(main): Call it.
> 	* rtl.h (RTX_FLD_WIDTH, RTX_HWINT_WIDTH): Define.
> 	(struct rtx_def): Use them.

Note, e.g. Coverity also complains about this stuff loudly too,
apparently not just in the problematic case where rtx_def is used
in a middle of structure, but also when used in flexible array
like spot.  Most RTLs are allocated through rtx_alloc and the size
is determined from RTX_HDR_SIZE (i.e. offsetof) and/or RTX_CODE_SIZE,
so your rtl.h change IMHO shouldn't affect anything but make the
expmed.c init_expmed_rtl structure somewhat longer.

> --- gcc/genpreds.c
> +++ gcc/genpreds.c
> @@ -1471,6 +1471,40 @@ parse_option (const char *opt)
>      return 0;
>  }
>  
> +/* Verify RTX codes.  We can't call fatal_error here, so call
> +   gcc_unreachable after error to really abort.  */
> +
> +static void
> +verify_rtx_codes (void)
> +{
> +  unsigned int i, j;
> +
> +  for (i = 0; i < NUM_RTX_CODE; i++)
> +    if (strchr (GET_RTX_FORMAT (i), 'w') == NULL)
> +      {
> +	if (strlen (GET_RTX_FORMAT (i)) > RTX_FLD_WIDTH)
> +	  {
> +	    error ("insufficient size of RTX_FLD_WIDTH");
> +	    gcc_unreachable ();

I think it would be nice to be more verbose, i.e. say
which rtx has longer format string than RTX_FLD_WIDTH, and perhaps also
the size and RTX_FLD_WIDTH value.  Also, can't you use internal_error
instead of error + gcc_unreachable ?

So perhaps
	    internal_error ("%s format %s longer than RTX_FLD_WIDTH %d\n",
			    GET_RTX_NAME (i), GET_RTX_FORMAT (i),
			    (int) RTX_FLD_WIDTH);
?

> +	  }
> +      }
> +    else
> +      {
> +	const size_t len = strlen (GET_RTX_FORMAT (i));
> +	for (j = 0; j < len; j++)
> +	  if (GET_RTX_FORMAT (i)[j] != 'w')
> +	    {
> +	      error ("rtx format does not contain only hwint entries");
> +	      gcc_unreachable ();
> +	    }
> +	if (len > RTX_HWINT_WIDTH)
> +	  {
> +	    error ("insufficient size of RTL_MAX_HWINT_WIDTH");
> +	    gcc_unreachable ();
> +	  }
> +      }

And similarly here.

Otherwise, LGTM, but as I've been discussing this with Marek,
I'd prefer somebody else to review it.

	Jakub
Jeff Law - June 20, 2014, 7:55 p.m.
On 06/20/14 13:01, Jakub Jelinek wrote:
> On Fri, Jun 20, 2014 at 07:36:41PM +0200, Marek Polacek wrote:
>> 2014-06-20  Marek Polacek  <polacek@redhat.com>
>>
>> 	* genpreds.c (verify_rtx_codes): New function.
>> 	(main): Call it.
>> 	* rtl.h (RTX_FLD_WIDTH, RTX_HWINT_WIDTH): Define.
>> 	(struct rtx_def): Use them.
>
> Note, e.g. Coverity also complains about this stuff loudly too,
Yes it does.  IIRC, these warnings from Coverity were the cause of GCC 
reaching the #1 or #2 position across Red Hat's packages in terms of 
Coverity warnings.  Not a good position to be in.


> like spot.  Most RTLs are allocated through rtx_alloc and the size
> is determined from RTX_HDR_SIZE (i.e. offsetof) and/or RTX_CODE_SIZE,
> so your rtl.h change IMHO shouldn't affect anything but make the
> expmed.c init_expmed_rtl structure somewhat longer.
Right.  This comment was actually very helpful in that I wasn't aware of 
precisely which cases Marek was trying to address.

Presumably the [1] sizing is what prevents any compile-time checking of 
this?

No strong opinion on the internal_error vs error+unreachable.

>
>> --- gcc/genpreds.c
>> +++ gcc/genpreds.c
>> @@ -1471,6 +1471,40 @@ parse_option (const char *opt)
>>       return 0;
>>   }
>>
>> +/* Verify RTX codes.  We can't call fatal_error here, so call
>> +   gcc_unreachable after error to really abort.  */
>> +
>> +static void
>> +verify_rtx_codes (void)
>> +{
>> +  unsigned int i, j;
>> +
>> +  for (i = 0; i < NUM_RTX_CODE; i++)
>> +    if (strchr (GET_RTX_FORMAT (i), 'w') == NULL)
>> +      {
>> +	if (strlen (GET_RTX_FORMAT (i)) > RTX_FLD_WIDTH)
>> +	  {
>> +	    error ("insufficient size of RTX_FLD_WIDTH");
>> +	    gcc_unreachable ();
>
> I think it would be nice to be more verbose, i.e. say
> which rtx has longer format string than RTX_FLD_WIDTH, and perhaps also
> the size and RTX_FLD_WIDTH value.  Also, can't you use internal_error
> instead of error + gcc_unreachable ?
Agreed, definitely indicate which RTX code is problematical.

Jeff
Jakub Jelinek - June 20, 2014, 9:10 p.m.
On Fri, Jun 20, 2014 at 01:55:41PM -0600, Jeff Law wrote:
> >like spot.  Most RTLs are allocated through rtx_alloc and the size
> >is determined from RTX_HDR_SIZE (i.e. offsetof) and/or RTX_CODE_SIZE,
> >so your rtl.h change IMHO shouldn't affect anything but make the
> >expmed.c init_expmed_rtl structure somewhat longer.
> Right.  This comment was actually very helpful in that I wasn't aware of
> precisely which cases Marek was trying to address.
> 
> Presumably the [1] sizing is what prevents any compile-time checking of
> this?

First version of Marek's patch did that (never instrumented [], [0] and
[1] arrays, no matter where they appeared, and instrumented everything
else).
Latest patch only never instruments [] (which, by definition can only
appear at the end of structure), other arrays (no matter what size)
aren't instrumented if they aren't followed by any fields, or
if the base of the handled components is not INDIRECT_REF/MEM_REF
(so, typically is a decl).
u.fld[1] array is the last field, so we don't warn for that, but when
rtx_def appears in another structure (in expmed.c) or if e.g. even
some code had a rtx_def typed variable and accessed say u.fld[1] in there,
it would be instrumented.

Whether we should have a strict array bounds mode where we would instrument
even arrays at the end of structures (with the exception of []) is something
to be discussed.

	Jakub
Marek Polacek - June 20, 2014, 9:31 p.m.
On Fri, Jun 20, 2014 at 11:10:18PM +0200, Jakub Jelinek wrote:
> On Fri, Jun 20, 2014 at 01:55:41PM -0600, Jeff Law wrote:
> > >like spot.  Most RTLs are allocated through rtx_alloc and the size
> > >is determined from RTX_HDR_SIZE (i.e. offsetof) and/or RTX_CODE_SIZE,
> > >so your rtl.h change IMHO shouldn't affect anything but make the
> > >expmed.c init_expmed_rtl structure somewhat longer.
> > Right.  This comment was actually very helpful in that I wasn't aware of
> > precisely which cases Marek was trying to address.
> > 
> > Presumably the [1] sizing is what prevents any compile-time checking of
> > this?
> 
> First version of Marek's patch did that (never instrumented [], [0] and
> [1] arrays, no matter where they appeared, and instrumented everything
> else).
> Latest patch only never instruments [] (which, by definition can only
> appear at the end of structure), other arrays (no matter what size)
> aren't instrumented if they aren't followed by any fields, or
> if the base of the handled components is not INDIRECT_REF/MEM_REF
> (so, typically is a decl).
> u.fld[1] array is the last field, so we don't warn for that, but when
> rtx_def appears in another structure (in expmed.c) or if e.g. even
> some code had a rtx_def typed variable and accessed say u.fld[1] in there,
> it would be instrumented.

Yeah - init_expmed in expmed.c has
XEXP (&all.plus, 1) = &all.reg;
which is expanded to
(((&all.plus)->u.fld[1]).rt_rtx) = &all.reg;
but since the expression (&A)->B is the same as A.B (if &A is a valid
pointer expression), the above was turned into
all.plus.u.fld[1].rt_rtx) = &all.reg;
and that doesn't contain any INDIRECT_REF/MEM_REFs -> it's being
instrumented.  With this patch I don't see any -fsanitize=bounds
errors when doing bootstrap-ubsan.
 
> Whether we should have a strict array bounds mode where we would instrument
> even arrays at the end of structures (with the exception of []) is something
> to be discussed.

This should be basically just about adding a new option - e.g.
-fsanitize=bounds-strict.

	Marek

Patch

diff --git gcc/genpreds.c gcc/genpreds.c
index b14a4ac..3826757 100644
--- gcc/genpreds.c
+++ gcc/genpreds.c
@@ -1471,6 +1471,40 @@  parse_option (const char *opt)
     return 0;
 }
 
+/* Verify RTX codes.  We can't call fatal_error here, so call
+   gcc_unreachable after error to really abort.  */
+
+static void
+verify_rtx_codes (void)
+{
+  unsigned int i, j;
+
+  for (i = 0; i < NUM_RTX_CODE; i++)
+    if (strchr (GET_RTX_FORMAT (i), 'w') == NULL)
+      {
+	if (strlen (GET_RTX_FORMAT (i)) > RTX_FLD_WIDTH)
+	  {
+	    error ("insufficient size of RTX_FLD_WIDTH");
+	    gcc_unreachable ();
+	  }
+      }
+    else
+      {
+	const size_t len = strlen (GET_RTX_FORMAT (i));
+	for (j = 0; j < len; j++)
+	  if (GET_RTX_FORMAT (i)[j] != 'w')
+	    {
+	      error ("rtx format does not contain only hwint entries");
+	      gcc_unreachable ();
+	    }
+	if (len > RTX_HWINT_WIDTH)
+	  {
+	    error ("insufficient size of RTL_MAX_HWINT_WIDTH");
+	    gcc_unreachable ();
+	  }
+      }
+}
+
 /* Master control.  */
 int
 main (int argc, char **argv)
@@ -1518,5 +1552,7 @@  main (int argc, char **argv)
   if (have_error || ferror (stdout) || fflush (stdout) || fclose (stdout))
     return FATAL_EXIT_CODE;
 
+  verify_rtx_codes ();
+
   return SUCCESS_EXIT_CODE;
 }
diff --git gcc/rtl.h gcc/rtl.h
index 6ec91a8..3f2e774 100644
--- gcc/rtl.h
+++ gcc/rtl.h
@@ -264,6 +264,12 @@  struct GTY((variable_size)) hwivec_def {
 #define CWI_PUT_NUM_ELEM(RTX, NUM)					\
   (RTL_FLAG_CHECK1("CWI_PUT_NUM_ELEM", (RTX), CONST_WIDE_INT)->u2.num_elem = (NUM))
 
+/* The maximum number of entries in the FLD array in rtx.  */
+#define RTX_FLD_WIDTH 8
+
+/* The maximum number of entries in the HWINT array in rtx.  */
+#define RTX_HWINT_WIDTH (MAX (REAL_WIDTH, 3))
+
 /* RTL expression ("rtx").  */
 
 struct GTY((chain_next ("RTX_NEXT (&%h)"),
@@ -378,8 +384,8 @@  struct GTY((chain_next ("RTX_NEXT (&%h)"),
      The number of operands and their types are controlled
      by the `code' field, according to rtl.def.  */
   union u {
-    rtunion fld[1];
-    HOST_WIDE_INT hwint[1];
+    rtunion fld[RTX_FLD_WIDTH];
+    HOST_WIDE_INT hwint[RTX_HWINT_WIDTH];
     struct block_symbol block_sym;
     struct real_value rv;
     struct fixed_value fv;