This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification
- From: Marek Polacek <polacek at redhat dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Jeff Law <law at redhat dot com>, Richard Henderson <rth at redhat dot com>
- Date: Fri, 20 Jun 2014 22:42:59 +0200
- Subject: Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification
- Authentication-results: sourceware.org; auth=none
- References: <20140620173640 dot GC14420 at redhat dot com> <20140620190113 dot GD31640 at tucnak dot redhat dot com>
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.
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;
Marek