Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification

2014-07-05 Thread Richard Biener 
On July 4, 2014 10:50:01 PM CEST, Jakub Jelinek ja...@redhat.com wrote:
On Mon, Jun 23, 2014 at 04:40:43PM +0200, Richard Biener wrote:
 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).

So like this?  Bootstrapped/regtested on x86_64-linux and i686-linux,
haven't seen any measurable difference in 1000x invocations of null .c
file
compilations.  It is just 18 GC allocations + I ggc_free all of them.

Yes.

Thanks,
Richard.

2014-07-04  Jakub Jelinek  ja...@redhat.com

   * expmed.c (struct init_expmed_rtl): Change all fields but
   pow2 and cint from struct rtx_def to rtx.
   (init_expmed_one_conv, init_expmed_one_mode): Adjust for that change.
   (init_expmed): Likewise.  Allocate all the 18 rtxes and ggc_free them
   at the end again.

--- gcc/expmed.c.jj2014-07-03 16:37:50.0 +0200
+++ gcc/expmed.c   2014-07-04 17:23:54.229174101 +0200
@@ -88,24 +88,24 @@ mask_rtx (enum machine_mode mode, int bi
 
 struct init_expmed_rtl
 {
-  struct rtx_def reg;
-  struct rtx_def plus;
-  struct rtx_def neg;
-  struct rtx_def mult;
-  struct rtx_def sdiv;
-  struct rtx_def udiv;
-  struct rtx_def sdiv_32;
-  struct rtx_def smod_32;
-  struct rtx_def wide_mult;
-  struct rtx_def wide_lshr;
-  struct rtx_def wide_trunc;
-  struct rtx_def shift;
-  struct rtx_def shift_mult;
-  struct rtx_def shift_add;
-  struct rtx_def shift_sub0;
-  struct rtx_def shift_sub1;
-  struct rtx_def zext;
-  struct rtx_def trunc;
+  rtx reg;
+  rtx plus;
+  rtx neg;
+  rtx mult;
+  rtx sdiv;
+  rtx udiv;
+  rtx sdiv_32;
+  rtx smod_32;
+  rtx wide_mult;
+  rtx wide_lshr;
+  rtx wide_trunc;
+  rtx shift;
+  rtx shift_mult;
+  rtx shift_add;
+  rtx shift_sub0;
+  rtx shift_sub1;
+  rtx zext;
+  rtx trunc;
 
   rtx pow2[MAX_BITS_PER_WORD];
   rtx cint[MAX_BITS_PER_WORD];
@@ -127,9 +127,9 @@ init_expmed_one_conv (struct init_expmed
  - (GET_MODE_CLASS (from_mode) == MODE_PARTIAL_INT));
   
   /* Assume cost of zero-extend and sign-extend is the same.  */
-  which = (to_size  from_size ? all-trunc : all-zext);
+  which = (to_size  from_size ? all-trunc : all-zext);
 
-  PUT_MODE (all-reg, from_mode);
+  PUT_MODE (all-reg, from_mode);
set_convert_cost (to_mode, from_mode, speed, set_src_cost (which,
speed));
 }
 
@@ -142,32 +142,32 @@ init_expmed_one_mode (struct init_expmed
 
   mode_bitsize = GET_MODE_UNIT_BITSIZE (mode);
 
-  PUT_MODE (all-reg, mode);
-  PUT_MODE (all-plus, mode);
-  PUT_MODE (all-neg, mode);
-  PUT_MODE (all-mult, mode);
-  PUT_MODE (all-sdiv, mode);
-  PUT_MODE (all-udiv, mode);
-  PUT_MODE (all-sdiv_32, mode);
-  PUT_MODE (all-smod_32, mode);
-  PUT_MODE (all-wide_trunc, mode);
-  PUT_MODE (all-shift, mode);
-  PUT_MODE (all-shift_mult, mode);
-  PUT_MODE (all-shift_add, mode);
-  PUT_MODE (all-shift_sub0, mode);
-  PUT_MODE (all-shift_sub1, mode);
-  PUT_MODE (all-zext, mode);
-  PUT_MODE (all-trunc, mode);
-
-  set_add_cost (speed, mode, set_src_cost (all-plus, speed));
-  set_neg_cost (speed, mode, set_src_cost (all-neg, speed));
-  set_mul_cost (speed, mode, set_src_cost (all-mult, speed));
-  set_sdiv_cost (speed, mode, set_src_cost (all-sdiv, speed));
-  set_udiv_cost (speed, mode, set_src_cost (all-udiv, speed));
+  PUT_MODE (all-reg, mode);
+  PUT_MODE (all-plus, mode);
+  PUT_MODE (all-neg, mode);
+  PUT_MODE (all-mult, mode);
+  PUT_MODE (all-sdiv, mode);
+  PUT_MODE (all-udiv, mode);
+  PUT_MODE (all-sdiv_32, mode);
+  PUT_MODE (all-smod_32, mode);
+  PUT_MODE (all-wide_trunc, mode);
+  PUT_MODE (all-shift, mode);
+  PUT_MODE (all-shift_mult, mode);
+  PUT_MODE (all-shift_add, mode);
+  PUT_MODE (all-shift_sub0, mode);
+  PUT_MODE (all-shift_sub1, mode);
+  PUT_MODE (all-zext, mode);
+  PUT_MODE (all-trunc, mode);
+
+  set_add_cost (speed, mode, set_src_cost (all-plus, speed));
+  set_neg_cost (speed, mode, set_src_cost (all-neg, speed));
+  set_mul_cost (speed, mode, set_src_cost (all-mult, speed));
+  set_sdiv_cost (speed, mode, set_src_cost (all-sdiv, speed));
+  set_udiv_cost (speed, mode, set_src_cost (all-udiv, speed));
 
-  set_sdiv_pow2_cheap (speed, mode, (set_src_cost (all-sdiv_32,
speed)
+  set_sdiv_pow2_cheap (speed, mode, (set_src_cost (all-sdiv_32,
speed)
= 2 * add_cost (speed, mode)));
-  set_smod_pow2_cheap (speed, mode, (set_src_cost (all-smod_32,
speed)
+  set_smod_pow2_cheap (speed, mode, (set_src_cost (all-smod_32,
speed)
= 4 * add_cost (speed, mode)));
 
   set_shift_cost (speed, mode, 0, 0);
@@ -181,13 +181,13 @@ init_expmed_one_mode (struct init_expmed
   n = MIN (MAX_BITS_PER_WORD, mode_bitsize);
   for (m = 1; m  n; m++)
 {
-  XEXP (all-shift, 1) = all-cint[m];
-  XEXP (all-shift_mult, 1) = all-pow2[m];
+  XEXP (all-shift, 1) = all-cint[m];
+  XEXP (all-shift_mult, 1) = all-pow2[m];
 
-  set_shift_cost (speed, mode, m,

Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification

2014-07-04 Thread Jakub Jelinek 
On Mon, Jun 23, 2014 at 04:40:43PM +0200, Richard Biener wrote:
 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).

So like this?  Bootstrapped/regtested on x86_64-linux and i686-linux,
haven't seen any measurable difference in 1000x invocations of null .c file
compilations.  It is just 18 GC allocations + I ggc_free all of them.

2014-07-04  Jakub Jelinek  ja...@redhat.com

* expmed.c (struct init_expmed_rtl): Change all fields but
pow2 and cint from struct rtx_def to rtx.
(init_expmed_one_conv, init_expmed_one_mode): Adjust for that change.
(init_expmed): Likewise.  Allocate all the 18 rtxes and ggc_free them
at the end again.

--- gcc/expmed.c.jj 2014-07-03 16:37:50.0 +0200
+++ gcc/expmed.c2014-07-04 17:23:54.229174101 +0200
@@ -88,24 +88,24 @@ mask_rtx (enum machine_mode mode, int bi
 
 struct init_expmed_rtl
 {
-  struct rtx_def reg;
-  struct rtx_def plus;
-  struct rtx_def neg;
-  struct rtx_def mult;
-  struct rtx_def sdiv;
-  struct rtx_def udiv;
-  struct rtx_def sdiv_32;
-  struct rtx_def smod_32;
-  struct rtx_def wide_mult;
-  struct rtx_def wide_lshr;
-  struct rtx_def wide_trunc;
-  struct rtx_def shift;
-  struct rtx_def shift_mult;
-  struct rtx_def shift_add;
-  struct rtx_def shift_sub0;
-  struct rtx_def shift_sub1;
-  struct rtx_def zext;
-  struct rtx_def trunc;
+  rtx reg;
+  rtx plus;
+  rtx neg;
+  rtx mult;
+  rtx sdiv;
+  rtx udiv;
+  rtx sdiv_32;
+  rtx smod_32;
+  rtx wide_mult;
+  rtx wide_lshr;
+  rtx wide_trunc;
+  rtx shift;
+  rtx shift_mult;
+  rtx shift_add;
+  rtx shift_sub0;
+  rtx shift_sub1;
+  rtx zext;
+  rtx trunc;
 
   rtx pow2[MAX_BITS_PER_WORD];
   rtx cint[MAX_BITS_PER_WORD];
@@ -127,9 +127,9 @@ init_expmed_one_conv (struct init_expmed
   - (GET_MODE_CLASS (from_mode) == MODE_PARTIAL_INT));
   
   /* Assume cost of zero-extend and sign-extend is the same.  */
-  which = (to_size  from_size ? all-trunc : all-zext);
+  which = (to_size  from_size ? all-trunc : all-zext);
 
-  PUT_MODE (all-reg, from_mode);
+  PUT_MODE (all-reg, from_mode);
   set_convert_cost (to_mode, from_mode, speed, set_src_cost (which, speed));
 }
 
@@ -142,32 +142,32 @@ init_expmed_one_mode (struct init_expmed
 
   mode_bitsize = GET_MODE_UNIT_BITSIZE (mode);
 
-  PUT_MODE (all-reg, mode);
-  PUT_MODE (all-plus, mode);
-  PUT_MODE (all-neg, mode);
-  PUT_MODE (all-mult, mode);
-  PUT_MODE (all-sdiv, mode);
-  PUT_MODE (all-udiv, mode);
-  PUT_MODE (all-sdiv_32, mode);
-  PUT_MODE (all-smod_32, mode);
-  PUT_MODE (all-wide_trunc, mode);
-  PUT_MODE (all-shift, mode);
-  PUT_MODE (all-shift_mult, mode);
-  PUT_MODE (all-shift_add, mode);
-  PUT_MODE (all-shift_sub0, mode);
-  PUT_MODE (all-shift_sub1, mode);
-  PUT_MODE (all-zext, mode);
-  PUT_MODE (all-trunc, mode);
-
-  set_add_cost (speed, mode, set_src_cost (all-plus, speed));
-  set_neg_cost (speed, mode, set_src_cost (all-neg, speed));
-  set_mul_cost (speed, mode, set_src_cost (all-mult, speed));
-  set_sdiv_cost (speed, mode, set_src_cost (all-sdiv, speed));
-  set_udiv_cost (speed, mode, set_src_cost (all-udiv, speed));
+  PUT_MODE (all-reg, mode);
+  PUT_MODE (all-plus, mode);
+  PUT_MODE (all-neg, mode);
+  PUT_MODE (all-mult, mode);
+  PUT_MODE (all-sdiv, mode);
+  PUT_MODE (all-udiv, mode);
+  PUT_MODE (all-sdiv_32, mode);
+  PUT_MODE (all-smod_32, mode);
+  PUT_MODE (all-wide_trunc, mode);
+  PUT_MODE (all-shift, mode);
+  PUT_MODE (all-shift_mult, mode);
+  PUT_MODE (all-shift_add, mode);
+  PUT_MODE (all-shift_sub0, mode);
+  PUT_MODE (all-shift_sub1, mode);
+  PUT_MODE (all-zext, mode);
+  PUT_MODE (all-trunc, mode);
+
+  set_add_cost (speed, mode, set_src_cost (all-plus, speed));
+  set_neg_cost (speed, mode, set_src_cost (all-neg, speed));
+  set_mul_cost (speed, mode, set_src_cost (all-mult, speed));
+  set_sdiv_cost (speed, mode, set_src_cost (all-sdiv, speed));
+  set_udiv_cost (speed, mode, set_src_cost (all-udiv, speed));
 
-  set_sdiv_pow2_cheap (speed, mode, (set_src_cost (all-sdiv_32, speed)
+  set_sdiv_pow2_cheap (speed, mode, (set_src_cost (all-sdiv_32, speed)
 = 2 * add_cost (speed, mode)));
-  set_smod_pow2_cheap (speed, mode, (set_src_cost (all-smod_32, speed)
+  set_smod_pow2_cheap (speed, mode, (set_src_cost (all-smod_32, speed)
 = 4 * add_cost (speed, mode)));
 
   set_shift_cost (speed, mode, 0, 0);
@@ -181,13 +181,13 @@ init_expmed_one_mode (struct init_expmed
   n = MIN (MAX_BITS_PER_WORD, mode_bitsize);
   for (m = 1; m  n; m++)
 {
-  XEXP (all-shift, 1) = all-cint[m];
-  XEXP (all-shift_mult, 1) = all-pow2[m];
+  XEXP (all-shift, 1) = all-cint[m];
+  XEXP (all-shift_mult, 1) = all-pow2[m];
 
-  set_shift_cost (speed, mode, m, set_src_cost (all-shift, speed));
-  set_shiftadd_cost (speed, mode, m,

Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification

2014-06-23 Thread Richard Henderson 
On 06/20/2014 01:42 PM, Marek Polacek wrote:
 2014-06-20  Marek Polacek  pola...@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~

Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification

2014-06-23 Thread Richard Biener 
On Mon, Jun 23, 2014 at 4:25 PM, Richard Henderson r...@redhat.com wrote:
 On 06/20/2014 01:42 PM, Marek Polacek wrote:
 2014-06-20  Marek Polacek  pola...@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~

Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification

2014-06-20 Thread Jakub Jelinek 
On Fri, Jun 20, 2014 at 07:36:41PM +0200, Marek Polacek wrote:
 2014-06-20  Marek Polacek  pola...@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

Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification

2014-06-20 Thread Jeff Law 

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  pola...@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

Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification

2014-06-20 Thread Marek Polacek 
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  pola...@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  pola...@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

Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification

2014-06-20 Thread Jakub Jelinek 
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

Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification

2014-06-20 Thread Marek Polacek 
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