[PATCH] Fix C++ enums handling in build_range_check

[PATCH] Fix C++ enums handling in build_range_check

[Jakub Jelinek]

Hi!

This bug has been caught in OOo, present in both 3.4.x and on the trunk.
Surprisingly it has not been introduced by my recent build_range_check
patch and exists on vanilla 3.4.x where that patch has not been commited.

extern "C" void abort (void);

enum E { E0, E1, E2, E3, E4, E5 };

void
test (enum E e)
{
  if (e == E2 || e == E3 || e == E1)
    abort ();
}

int
main (void)
{
  test (E4);
  test (E5);
  test (E0);
  return 0;
}

The above testcase will abort with -O1 and above.
The problem is that this enum has TYPE_PRECISION 3 and build_range_check
special cases c >= 1 && c <= 127 range comparisons (where 127 stands
for largest signed value in that type).
This results in "optimizing" the e >= 1 && e <= 3 test into
(signed E) e > 0, but as the comparison is in the end not done in the
3 bit type but in SImode which is the underlying enum's TYPE_MODE,
we get it wrong.

Ok for trunk and 3.4?

For 3.4 where the range-test-1.[cC] tests don't exist I'd like to commit
the whole tests instead of commiting a testcase like above.

2004-07-09  Jakub Jelinek  <jakub@redhat.com>

	* fold-const.c (build_range_check): Use TYPE_MODE's precision for
	enumerals.

	* g++.dg/opt/range-test-1.C: Add new test (#26).

--- gcc/fold-const.c.jj	2004-07-09 13:50:54.000000000 +0200
+++ gcc/fold-const.c	2004-07-09 18:10:52.945905468 +0200
@@ -3844,7 +3844,13 @@ build_range_check (tree type, tree exp, 
       HOST_WIDE_INT hi;
       int prec;
 
-      prec = TYPE_PRECISION (etype);
+      /* For enums the comparison will be done in the underlying type,
+	 so using enum's precision is wrong here.
+	 Consider e.g. enum { A, B, C, D, E }, low == B and high == D.  */
+      if (TREE_CODE (etype) == ENUMERAL_TYPE)
+	prec = GET_MODE_BITSIZE (TYPE_MODE (etype));
+      else
+	prec = TYPE_PRECISION (etype);
       if (prec <= HOST_BITS_PER_WIDE_INT)
 	{
 	  hi = 0;
--- gcc/testsuite/g++.dg/opt/range-test-1.C.jj	2004-06-19 00:38:06.000000000 +0200
+++ gcc/testsuite/g++.dg/opt/range-test-1.C	2004-07-09 18:06:27.501797366 +0200
@@ -176,6 +176,9 @@ T(24, signed char, x == SCHAR_MIN || x =
 T(25, integers, x == int_smallest || x == int_largest,
   { int_smallest C int_largest }, { int_minus1 C int_zero C int_one
     C int_2ndsmallest C int_2ndlargest C int_3rdsmallest C int_3rdlargest })
+T(26, enum3, x == enum3_one || x == enum3_two || x == enum3_three,
+  { enum3_one C enum3_two C enum3_three }, { enum3_zero C enum3_four
+    C enum3_five C enum3_six C enum3_seven })
 
 /* These should be optimized into unconditional jumps.  */
 T(o1, unsigned long, x <= 16 || (x >= 17 && x <= -1UL),

	Jakub

Re: [PATCH] Fix C++ enums handling in build_range_check

[Roger Sayle]

On Fri, 9 Jul 2004, Jakub Jelinek wrote:
> 2004-07-09  Jakub Jelinek  <jakub@redhat.com>
>
> 	* fold-const.c (build_range_check): Use TYPE_MODE's precision for
> 	enumerals.
>
> 	* g++.dg/opt/range-test-1.C: Add new test (#26).
>
> Ok for trunk and 3.4?

This looks "obviously" Ok for mainline.  You didn't list the target
triple that you bootstrapped and regression tested on, but assuming
these also pass against the gcc-3_4-branch, this patch is Ok there
too.

Roger
--

Re: [PATCH] Fix C++ enums handling in build_range_check

[Andrew Pinski]

> 
> 
> On Fri, 9 Jul 2004, Jakub Jelinek wrote:
> > 2004-07-09  Jakub Jelinek  <jakub@redhat.com>
> >
> > 	* fold-const.c (build_range_check): Use TYPE_MODE's precision for
> > 	enumerals.
> >
> > 	* g++.dg/opt/range-test-1.C: Add new test (#26).
> >
> > Ok for trunk and 3.4?
> 
> This looks "obviously" Ok for mainline.  You didn't list the target
> triple that you bootstrapped and regression tested on, but assuming
> these also pass against the gcc-3_4-branch, this patch is Ok there
> too.

And this is PR 16372.

-- Andrew

Re: [PATCH] Fix C++ enums handling in build_range_check

[Richard Henderson]

On Fri, Jul 09, 2004 at 10:24:08AM -0600, Roger Sayle wrote:
> This looks "obviously" Ok for mainline.

No, for mainline, particularly after jsm28's bitfield patches,
I would like to get arithmetic on odd-sided types done correctly.


r~

Re: [PATCH] Fix C++ enums handling in build_range_check

[Roger Sayle]

On Fri, 9 Jul 2004, Richard Henderson wrote:
> No, for mainline, particularly after jsm28's bitfield patches,
> I would like to get arithmetic on odd-sided types done correctly.

I think you'll need to get "buy in" from the C++ maintainers:
http://gcc.gnu.org/ml/gcc-patches/2004-06/msg00060.html
http://gcc.gnu.org/ml/gcc-patches/2004-06/msg00075.html

And as I mentioned in my post to work-around PR middle-end/15069
http://gcc.gnu.org/ml/gcc-patches/2004-05/msg02002.html
There are "known" problems in tree-ssa's DOM pass in addition to
the missing support for odd-sized types in RTL expansion.

Not that these problems can't be fixed, and Joseph's bitfield
patch is a huge step forward in this direction, but I suspect
it might take a while...  Supporting odd-sized types is almost
a new feature; might it not make sense to disable or workaround
regressions until all of the required infrastructure is in place?

Roger
--
p.s. The GNU pascal front-end used to be an excellent source
of odd-sized type problems, such as PR middle-end/11264.

Re: [PATCH] Fix C++ enums handling in build_range_check

[Joseph S. Myers]

On Fri, 9 Jul 2004, Roger Sayle wrote:

> I think you'll need to get "buy in" from the C++ maintainers:
> http://gcc.gnu.org/ml/gcc-patches/2004-06/msg00060.html
> http://gcc.gnu.org/ml/gcc-patches/2004-06/msg00075.html
> 
> And as I mentioned in my post to work-around PR middle-end/15069
> http://gcc.gnu.org/ml/gcc-patches/2004-05/msg02002.html
> There are "known" problems in tree-ssa's DOM pass in addition to
> the missing support for odd-sized types in RTL expansion.

I hope the known problems have bug reports open, at least if they can now
be triggered with C bit-fields.  With bugs reported as problems are found,
we can fix them.

> it might take a while...  Supporting odd-sized types is almost
> a new feature; might it not make sense to disable or workaround
> regressions until all of the required infrastructure is in place?

The langhook reduce_bit_field_operations, and the code in
c_common_signed_or_unsigned_type that only creates bit-field types for C
and not for C++ enums, and the limitation of the bit-field handling in
expand_expr_real_1 to INTEGER_TYPEs, are effectively such workarounds,
intended to ensure that my patch (intended to fix C standards issues) did
not affect languages other than C.  It is quite possible that with
appropriate bugfixes in both common code and front ends, and a clear
common understanding of exactly what these types mean in GENERIC, all
these restrictions can go away.

> p.s. The GNU pascal front-end used to be an excellent source
> of odd-sized type problems, such as PR middle-end/11264.

Do the GNU Pascal maintainers yet have any interest in merging into GCC
CVS?

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)