| Message ID | 20140620173640.GC14420@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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;