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

login
register
mail settings
Submitter Marek Polacek
Date June 20, 2014, 8:42 p.m.
Message ID <20140620204259.GE14420@redhat.com>
Download mbox | patch
Permalink /patch/362345/
State New
Headers show

Comments

Marek Polacek - June 20, 2014, 8:42 p.m.
On Fri, Jun 20, 2014 at 09:01:14PM +0200, 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,
> 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);
> ?
 
Ok, that's much better.  I actually can use internal_error - we have 
that function in both diagnostic.c and errors.c, while fatal_error is
only in diagnostic.c.

> > +	  }
> > +      }
> > +    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.

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

Sure.  So can anybody look at this, please?

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
Richard Henderson - June 23, 2014, 2:25 p.m.
On 06/20/2014 01:42 PM, 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.

Looks pretty good.  Just a few nits.

> +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)
> +	  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));

The strlen result is used in both arms of the if.  Tidier to hoist it, I think.

> +	for (j = 0; j < len; j++)
> +	  if (GET_RTX_FORMAT (i)[j] != 'w')
> +	    internal_error ("%s format %s should contain only w, but "
> +			    "has %c\n", GET_RTX_NAME (i), GET_RTX_FORMAT (i),
> +			    GET_RTX_FORMAT (i)[j]);

The loop is strspn.  Perhaps tidier as

  const size_t spn = strspn(GET_RTX_FORMAT (i), "w");
  if (spn != len)
    internal_error (...);


r~
Richard Guenther - June 23, 2014, 2:40 p.m.
On Mon, Jun 23, 2014 at 4:25 PM, Richard Henderson <rth@redhat.com> wrote:
> On 06/20/2014 01:42 PM, 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.
>
> Looks pretty good.  Just a few nits.
>
>> +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)
>> +       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));
>
> The strlen result is used in both arms of the if.  Tidier to hoist it, I think.
>
>> +     for (j = 0; j < len; j++)
>> +       if (GET_RTX_FORMAT (i)[j] != 'w')
>> +         internal_error ("%s format %s should contain only w, but "
>> +                         "has %c\n", GET_RTX_NAME (i), GET_RTX_FORMAT (i),
>> +                         GET_RTX_FORMAT (i)[j]);
>
> The loop is strspn.  Perhaps tidier as
>
>   const size_t spn = strspn(GET_RTX_FORMAT (i), "w");
>   if (spn != len)
>     internal_error (...);

Note that RTX_HWINT_WIDTH is wrong because of CONST_WIDE_INT.

Also I don't like increasing the array sizes - this is wrong in the same
way as [1] is.

Can we instead refactor expmed.c to avoid allocating rtx_def directly?
Like by using rtx in init_expmed_rtl and allocating from an obstack
(or not care and GC-allocate anyway).

Richard.

>
> r~
>

Patch

diff --git gcc/genpreds.c gcc/genpreds.c
index b14a4ac..7e62124 100644
--- gcc/genpreds.c
+++ gcc/genpreds.c
@@ -1471,6 +1471,36 @@  parse_option (const char *opt)
     return 0;
 }
 
+/* Verify RTX codes.  */
+
+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)
+	  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')
+	    internal_error ("%s format %s should contain only w, but "
+			    "has %c\n", GET_RTX_NAME (i), GET_RTX_FORMAT (i),
+			    GET_RTX_FORMAT (i)[j]);
+	if (len > RTX_HWINT_WIDTH)
+	  internal_error ("%s format %s longer than RTX_HWINT_WIDTH %d\n",
+			  GET_RTX_NAME (i), GET_RTX_FORMAT (i),
+			  (int) RTX_HWINT_WIDTH);
+      }
+}
+
 /* Master control.  */
 int
 main (int argc, char **argv)
@@ -1518,5 +1548,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;