Re: [PATCH] Fix for short-enums comparison bug

Re: [PATCH] Fix for short-enums comparison bug

[Jeffrey A Law]

  In message <x490ee6l2e.fsf@janus.pgt.com>you write:
  > The following message is a courtesy copy of an article
  > that has been posted to gnu.gcc,gnu.gcc.bug as well.
Thanks.


  > I provide below a one-line patch which fixes the bug; however I'd like
  > to provide a little bit of explanation about the nature of the bug and
  > how I found it.  My apologies if this is a bit long.
No need to apologize, it is precisely the kind of analysis that makes my
job of reviewing patches a whole lot easier :-)

  > Here's a very simple example which illustrates the bug in question:
  > 
  > typedef enum {A=0, B, C, D} T;
  > main(){
  >   T x;
  >   for (x=A; x<=D; ++x)
  >     printf("%d ", (int)x);
  >   putchar('\n');
  > }
  > 
  > If you compile this with no compiler flags set and run it will print 
  > 
  > 0 1 2 3  
  > 
  > as expected.  If you compile with -fshort-enums it will loop
  > indefinitely, printing all values from 0-255 over and over.
I'm not a language lawyer -- we really need one to determine what the behavior
of that code should be.

I don't have an ANSI C standard here, nor do I have a copy of the ISO C9X
standard.  Can someone who does dig into them and determine what happens for
an assignment out of the range of an enumerated type and what assumptions the
compiler can/can not make about the values an enumerated variable can have?


  > Try running this with and without the -fshort-enums flag.  If the
  > intent of -fshort-enums is to really restrict the value of an
  > enumerated type to lie within the min and max of that type, a warning
  > should be produced when we set x equal to 10.  But no such warning is
  > produced, just bizarre results (the program prints 10<=3 if
  > -fshort-enums is used).
Actually, whether or not a warning must be produced is a standards issue.  Even
if the standards don't demand it, a warning would be nice.  Someone would have
to grovel around the front-end though to produce the warning.  Once we've
left the language front end it's too late.


Jeff

Re: [PATCH] Fix for short-enums comparison bug

[Gavin Romig-Koch]

Jeffrey A Law writes:
 >   > Here's a very simple example which illustrates the bug in question:
 >   > 
 >   > typedef enum {A=0, B, C, D} T;
 >   > main(){
 >   >   T x;
 >   >   for (x=A; x<=D; ++x)
 >   >     printf("%d ", (int)x);
 >   >   putchar('\n');
 >   > }
 >   > 
 >   > If you compile this with no compiler flags set and run it will print 
 >   > 
 >   > 0 1 2 3  
 >   > 
 >   > as expected.  If you compile with -fshort-enums it will loop
 >   > indefinitely, printing all values from 0-255 over and over.

 > I don't have an ANSI C standard here, nor do I have a copy of the ISO C9X
 > standard.  Can someone who does dig into them and determine what happens for
 > an assignment out of the range of an enumerated type and what assumptions
 > the compiler can/can not make about the values an enumerated variable
 > can have?

c89 and c9x say that enumerations have to act like some other integer type.
Which integer type is implementation defined, and can vary from enumeration
to enumeration, but it has to be capable of representing all the values
in the enumeration.

It looks like GCC is choosing unsigned char for this particular enum
with -fshort-enums, which is ok standard-wise.  Given this though,
I don't see why this example shouldn't work the same as it does without
-fshort-enums.


 >   > Try running this with and without the -fshort-enums flag.  If the
 >   > intent of -fshort-enums is to really restrict the value of an
 >   > enumerated type to lie within the min and max of that type, a warning
 >   > should be produced when we set x equal to 10.  But no such warning is
 >   > produced, just bizarre results (the program prints 10<=3 if
 >   > -fshort-enums is used).
 > Actually, whether or not a warning must be produced is a standards issue.  Even
 > if the standards don't demand it, a warning would be nice.  Someone would have
 > to grovel around the front-end though to produce the warning.  Once we've
 > left the language front end it's too late.

IMO, the intent of -fshort-enums is only to have the compiler choose
a smaller integral type for the representation of an enum, and not to 
change the kinds of diagnostics produced.  Warnings about creating
enum values outside the range of the enum, and/or outside the range
of the representation of the enum should be a separate option.  One
or both of these options may already exist in gcc, but I don't know
without looking.


                                           -gavin...

Re: [PATCH] Fix for short-enums comparison bug

[Joern Rennecke]

> It looks like GCC is choosing unsigned char for this particular enum
> with -fshort-enums, which is ok standard-wise.  Given this though,
> I don't see why this example shouldn't work the same as it does without
> -fshort-enums.

It's probably a biv elimination problem.  Try -fno-strength-reduce.

Re: [PATCH] Fix for short-enums comparison bug

[Charles G Waldman]

Gavin Romig-Koch writes:

 > It looks like GCC is choosing unsigned char for this particular enum
 > with -fshort-enums, which is ok standard-wise.  Given this though,
 > I don't see why this example shouldn't work the same as it does without
 > -fshort-enums.

I tried to explain this in my original posting.

Please see the block of code in the function shorten_compare (in
c-common.c) starting at line 2258 (in the egcs-1.1.1 release), which
for your convenience I've appended below.

In gcc-2.7.x the call to signed_or_unsigned_type (see ***) returned
the underlying integral type, but the following change:

Mon Oct 28 13:08:51 1996  Richard Kenner  (kenner@vlsi1.ultra.nyu.edu)
	(signed_or_unsigned_type): If already right signedness, return.

caused signed_or_unsigned_type to return the enumerated type itself
rather than the underlying integral type.  

Thus maxval and minval refer to the maximum and minimum of the
declared enum values rather than of the underlying integral type, and
the subsequent logic optimizes away the comparison.

My proposed patch fixes this.


--------------------------------------------

Excerpt from shorten_compare(c_common.c):

  /* If comparing an integer against a constant more bits wide,
     maybe we can deduce a value of 1 or 0 independent of the data.
     Or else truncate the constant now
     rather than extend the variable at run time.

     This is only interesting if the constant is the wider arg.
     Also, it is not safe if the constant is unsigned and the
     variable arg is signed, since in this case the variable
     would be sign-extended and then regarded as unsigned.
     Our technique fails in this case because the lowest/highest
     possible unsigned results don't follow naturally from the
     lowest/highest possible values of the variable operand.
     For just EQ_EXPR and NE_EXPR there is another technique that
     could be used: see if the constant can be faithfully represented
     in the other operand's type, by truncating it and reextending it
     and see if that preserves the constant's value.  */

  if (!real1 && !real2
      && TREE_CODE (primop1) == INTEGER_CST
      && TYPE_PRECISION (TREE_TYPE (primop0)) < TYPE_PRECISION (*restype_ptr))
    {
      int min_gt, max_gt, min_lt, max_lt;
      tree maxval, minval;
      /* 1 if comparison is nominally unsigned.  */
      int unsignedp = TREE_UNSIGNED (*restype_ptr);
      tree val;

[***] type = signed_or_unsigned_type (unsignedp0, TREE_TYPE (primop0));

      maxval = TYPE_MAX_VALUE (type);
      minval = TYPE_MIN_VALUE (type);

      if (unsignedp && !unsignedp0)
	*restype_ptr = signed_type (*restype_ptr);

      if (TREE_TYPE (primop1) != *restype_ptr)
	primop1 = convert (*restype_ptr, primop1);
      if (type != *restype_ptr)
	{
	  minval = convert (*restype_ptr, minval);
	  maxval = convert (*restype_ptr, maxval);
	}

      if (unsignedp && unsignedp0)
	{
	  min_gt = INT_CST_LT_UNSIGNED (primop1, minval);
	  max_gt = INT_CST_LT_UNSIGNED (primop1, maxval);
	  min_lt = INT_CST_LT_UNSIGNED (minval, primop1);
	  max_lt = INT_CST_LT_UNSIGNED (maxval, primop1);
	}
      else
	{
	  min_gt = INT_CST_LT (primop1, minval);
	  max_gt = INT_CST_LT (primop1, maxval);
	  min_lt = INT_CST_LT (minval, primop1);
	  max_lt = INT_CST_LT (maxval, primop1);
	}

      val = 0;
      /* This used to be a switch, but Genix compiler can't handle that.  */
      if (code == NE_EXPR)
	{
	  if (max_lt || min_gt)
	    val = boolean_true_node;
	}
      else if (code == EQ_EXPR)
	{
	  if (max_lt || min_gt)
	    val = boolean_false_node;
	}
      else if (code == LT_EXPR)
	{
	  if (max_lt)
	    val = boolean_true_node;
	  if (!min_lt)
	    val = boolean_false_node;
	}
      else if (code == GT_EXPR)
	{
	  if (min_gt)
	    val = boolean_true_node;
	  if (!max_gt)
	    val = boolean_false_node;
	}
      else if (code == LE_EXPR)
	{
	  if (!max_gt)
	    val = boolean_true_node;
	  if (min_gt)
	    val = boolean_false_node;
	}
      else if (code == GE_EXPR)
	{
	  if (!min_lt)
	    val = boolean_true_node;
	  if (max_lt)
	    val = boolean_false_node;
	}

Re: [PATCH] Fix for short-enums comparison bug

[Jeffrey A Law]

  In message < 14018.60839.325601.426391@cetus.cygnus.com >you write:
  > c89 and c9x say that enumerations have to act like some other integer type.
  > Which integer type is implementation defined, and can vary from enumeration
  > to enumeration, but it has to be capable of representing all the values
  > in the enumeration.
Yes the underlying type must be capable of representing all the values for
the enum.  But does the enum restrict the actual values which can be used
with the expected results?

My quick read of the C++ standard made it appear as if assigning a value that
was not a member of the enum resulted in undefined results.  Is this possibly a
case where C++ and C differ?


jeff

Re: [PATCH] Fix for short-enums comparison bug

[Gavin Romig-Koch]

Jeffrey A Law writes:
 > Yes the underlying type must be capable of representing all the values for
 > the enum.  But does the enum restrict the actual values which can be used
 > with the expected results?

The enum can restrict the actual values which can be used to the values
of the choosen underlying type.  The underlying type must be an existing
type (the compiler can't make up a new "tiny" type for the underlying
type).  If you know the underlying type, you know what values can be
used.  

 > My quick read of the C++ standard made it appear as if assigning a value that
 > was not a member of the enum resulted in undefined results.  Is this possibly a
 > case where C++ and C differ?

Probably.  There was talk in the C++ committee to restrict the usable
values farther than C does, but I don't remember the outcome of that
discussion.  c9x kept the old C rules.

                                      -gavin...
          

Re: [PATCH] Fix for short-enums comparison bug

[Joe Buck]

> My quick read of the C++ standard made it appear as if assigning a value that
> was not a member of the enum resulted in undefined results.

Wrong.  The ISO C++ standard defines what values must be assignable to the
enum, and it is a larger set of values than just the members.  I don't
have the language in front of me, but the intent is to require the ability
to hold intermediate values and AND and OR of legal values.  Thus code
like the following is standard-conformant:

enum flags {
	flag_none = 0x0,
	flag_a = 0x1,
	flag_b = 0x2,
	flag_c = 0x4,
	flag_d = 0x8
};

...
	flags foo = (flags)(flag_a | flag+b);	// OK
	flags bar = (flags) 12;			// OK
	flags xyz = (flags) 23;			// not OK

the last one is not guaranteed to work because the compiler is allowed to
use a four-bit unsigned field and 23 will not fit.

> case where C++ and C differ?

Re: [PATCH] Fix for short-enums comparison bug

[Jeffrey A Law]

  In message < 199902111942.LAA16560@atrus.synopsys.com >you write:
  > Wrong.  The ISO C++ standard defines what values must be assignable to the
  > enum, and it is a larger set of values than just the members.  I don't
  > have the language in front of me, but the intent is to require the ability
  > to hold intermediate values and AND and OR of legal values.  Thus code
  > like the following is standard-conformant:
  > 
  > enum flags {
  > 	flag_none = 0x0,
  > 	flag_a = 0x1,
  > 	flag_b = 0x2,
  > 	flag_c = 0x4,
  > 	flag_d = 0x8
  > };
  > 
  > ...
  > 	flags foo = (flags)(flag_a | flag+b);	// OK
  > 	flags bar = (flags) 12;			// OK
  > 	flags xyz = (flags) 23;			// not OK
  > 
  > the last one is not guaranteed to work because the compiler is allowed to
  > use a four-bit unsigned field and 23 will not fit.
OK.  Thanks.  So if we go back to the original sample code:


typedef enum {A=0, B, C, D} T;
main(){
  T x;
  for (x=A; x<=D; ++x)
    printf("%d ", (int)x);
  putchar('\n');
}

So the compiler could use a 2 bit unsigned field for x since the values for 
enum T are 0, 1, 2, 3.  Other values will not fit and would be considered
invalid.  Thus removing the test x <= D is technically valid for C++.  Right?

That (of course) isn't binding for C.

jeff

Re: [PATCH] Fix for short-enums comparison bug

[Joe Buck]

> OK.  Thanks.  So if we go back to the original sample code:
> 
> 
> typedef enum {A=0, B, C, D} T;
> main(){
>   T x;
>   for (x=A; x<=D; ++x)
>     printf("%d ", (int)x);
>   putchar('\n');
> }
> 
> So the compiler could use a 2 bit unsigned field for x since the values for 
> enum T are 0, 1, 2, 3.  Other values will not fit and would be considered
> invalid.  Thus removing the test x <= D is technically valid for C++.  Right?

Hmm.  I'm not sure.  It would seem so.  It does seem that the code is
risky, at least, and might merit a warning (I definitely think that
g++ must not remove this test without issuing a warning).

Note also that the operators =, <=, and ++ can be overloaded for an
enum, which would change things yet again (I could define ++ to wrap
around if I wanted to).

> That (of course) isn't binding for C.

Yes, in C enums are basically ints of some form.  However, even in C
there might be similar cases, if one of the enum values is the maximum
value of the integral type that is used.

Re: [PATCH] Fix for short-enums comparison bug

[Tim Hollebeek]

Jeffrey A Law writes ...
> 
> typedef enum {A=0, B, C, D} T;
> main(){
>   T x;
>   for (x=A; x<=D; ++x)
>     printf("%d ", (int)x);
>   putchar('\n');
> }
> 
> So the compiler could use a 2 bit unsigned field for x since the values for 
> enum T are 0, 1, 2, 3.  Other values will not fit and would be considered
> invalid.  Thus removing the test x <= D is technically valid for C++.  Right?

I seriously doubt it, since arguments to integer operators undergo
integral promotion.  The rules are different for C++, but I believe
the answer is no, since none of the standard integral types is allowed
to have only 2 bits.

---------------------------------------------------------------------------
Tim Hollebeek                           | "Everything above is a true
email: tim@wfn-shop.princeton.edu       |  statement, for sufficiently
URL: http://wfn-shop.princeton.edu/~tim |  false values of true."

Re: [PATCH] Fix for short-enums comparison bug

[Jeffrey A Law]

  In message < 199902112216.RAA27319@wagner.Princeton.EDU >you write:
  > I seriously doubt it, since arguments to integer operators undergo
  > integral promotion.  The rules are different for C++, but I believe
  > the answer is no, since none of the standard integral types is allowed
  > to have only 2 bits.
I'm not looking for "I doubt/I believe".  I need folks to actually check the
relavent standard and say "The standard ..." :-)

jeff

Re: [PATCH] Fix for short-enums comparison bug

[Joe Buck]

>   In message < 199902112216.RAA27319@wagner.Princeton.EDU >you write:
>   > I seriously doubt it, since arguments to integer operators undergo
>   > integral promotion.  The rules are different for C++, but I believe
>   > the answer is no, since none of the standard integral types is allowed
>   > to have only 2 bits.
> I'm not looking for "I doubt/I believe".  I need folks to actually check the
> relavent standard and say "The standard ..." :-)

The relevant part of the C++ standard has the label "dcl.enum": look
for .../dcl.html#dcl.enum in your favorite copy of the standard (I
assume you have an up-to-date version inside Cygnus).

I'm currently looking at the Dec. 96 draft, so I don't know if there were
changes made later (you can check your local copy).  Two things are
described: the legal integral values that can be assigned to the enum, and
the "underlying type", which is implementation-dependent but can't be
larger than int unless there are enum values that don't fit in int or
unsigned int.

Stroustrup "The C++ Programming Language, Third Edition"'s section 4.8 may
be a clearer explanation of what is intended (though of course it is
non-normative).

Re: [PATCH] Fix for short-enums comparison bug

[Joe Buck]

> OK.  Thanks.  So if we go back to the original sample code:
> 
> 
> typedef enum {A=0, B, C, D} T;
> main(){
>   T x;
>   for (x=A; x<=D; ++x)
>     printf("%d ", (int)x);
>   putchar('\n');
> }
> 
> So the compiler could use a 2 bit unsigned field for x since the values for 
> enum T are 0, 1, 2, 3.  Other values will not fit and would be considered
> invalid.  Thus removing the test x <= D is technically valid for C++.  Right?

Hmm.  I'm not sure.  It would seem so.  It does seem that the code is
risky, at least, and might merit a warning (I definitely think that
g++ must not remove this test without issuing a warning).

Note also that the operators =, <=, and ++ can be overloaded for an
enum, which would change things yet again (I could define ++ to wrap
around if I wanted to).

> That (of course) isn't binding for C.

Yes, in C enums are basically ints of some form.  However, even in C
there might be similar cases, if one of the enum values is the maximum
value of the integral type that is used.

Re: [PATCH] Fix for short-enums comparison bug

[Tim Hollebeek]

Jeffrey A Law writes ...
> 
> typedef enum {A=0, B, C, D} T;
> main(){
>   T x;
>   for (x=A; x<=D; ++x)
>     printf("%d ", (int)x);
>   putchar('\n');
> }
> 
> So the compiler could use a 2 bit unsigned field for x since the values for 
> enum T are 0, 1, 2, 3.  Other values will not fit and would be considered
> invalid.  Thus removing the test x <= D is technically valid for C++.  Right?

I seriously doubt it, since arguments to integer operators undergo
integral promotion.  The rules are different for C++, but I believe
the answer is no, since none of the standard integral types is allowed
to have only 2 bits.

---------------------------------------------------------------------------
Tim Hollebeek                           | "Everything above is a true
email: tim@wfn-shop.princeton.edu       |  statement, for sufficiently
URL: http://wfn-shop.princeton.edu/~tim |  false values of true."

Re: [PATCH] Fix for short-enums comparison bug

[Jeffrey A Law]

  In message < 14018.60839.325601.426391@cetus.cygnus.com >you write:
  > c89 and c9x say that enumerations have to act like some other integer type.
  > Which integer type is implementation defined, and can vary from enumeration
  > to enumeration, but it has to be capable of representing all the values
  > in the enumeration.
Yes the underlying type must be capable of representing all the values for
the enum.  But does the enum restrict the actual values which can be used
with the expected results?

My quick read of the C++ standard made it appear as if assigning a value that
was not a member of the enum resulted in undefined results.  Is this possibly a
case where C++ and C differ?


jeff

Re: [PATCH] Fix for short-enums comparison bug

[Gavin Romig-Koch]

Jeffrey A Law writes:
 >   > Here's a very simple example which illustrates the bug in question:
 >   > 
 >   > typedef enum {A=0, B, C, D} T;
 >   > main(){
 >   >   T x;
 >   >   for (x=A; x<=D; ++x)
 >   >     printf("%d ", (int)x);
 >   >   putchar('\n');
 >   > }
 >   > 
 >   > If you compile this with no compiler flags set and run it will print 
 >   > 
 >   > 0 1 2 3  
 >   > 
 >   > as expected.  If you compile with -fshort-enums it will loop
 >   > indefinitely, printing all values from 0-255 over and over.

 > I don't have an ANSI C standard here, nor do I have a copy of the ISO C9X
 > standard.  Can someone who does dig into them and determine what happens for
 > an assignment out of the range of an enumerated type and what assumptions
 > the compiler can/can not make about the values an enumerated variable
 > can have?

c89 and c9x say that enumerations have to act like some other integer type.
Which integer type is implementation defined, and can vary from enumeration
to enumeration, but it has to be capable of representing all the values
in the enumeration.

It looks like GCC is choosing unsigned char for this particular enum
with -fshort-enums, which is ok standard-wise.  Given this though,
I don't see why this example shouldn't work the same as it does without
-fshort-enums.


 >   > Try running this with and without the -fshort-enums flag.  If the
 >   > intent of -fshort-enums is to really restrict the value of an
 >   > enumerated type to lie within the min and max of that type, a warning
 >   > should be produced when we set x equal to 10.  But no such warning is
 >   > produced, just bizarre results (the program prints 10<=3 if
 >   > -fshort-enums is used).
 > Actually, whether or not a warning must be produced is a standards issue.  Even
 > if the standards don't demand it, a warning would be nice.  Someone would have
 > to grovel around the front-end though to produce the warning.  Once we've
 > left the language front end it's too late.

IMO, the intent of -fshort-enums is only to have the compiler choose
a smaller integral type for the representation of an enum, and not to 
change the kinds of diagnostics produced.  Warnings about creating
enum values outside the range of the enum, and/or outside the range
of the representation of the enum should be a separate option.  One
or both of these options may already exist in gcc, but I don't know
without looking.


                                           -gavin...

Re: [PATCH] Fix for short-enums comparison bug

[Joern Rennecke]

> It looks like GCC is choosing unsigned char for this particular enum
> with -fshort-enums, which is ok standard-wise.  Given this though,
> I don't see why this example shouldn't work the same as it does without
> -fshort-enums.

It's probably a biv elimination problem.  Try -fno-strength-reduce.

Re: [PATCH] Fix for short-enums comparison bug

[Charles G Waldman]

Gavin Romig-Koch writes:

 > It looks like GCC is choosing unsigned char for this particular enum
 > with -fshort-enums, which is ok standard-wise.  Given this though,
 > I don't see why this example shouldn't work the same as it does without
 > -fshort-enums.

I tried to explain this in my original posting.

Please see the block of code in the function shorten_compare (in
c-common.c) starting at line 2258 (in the egcs-1.1.1 release), which
for your convenience I've appended below.

In gcc-2.7.x the call to signed_or_unsigned_type (see ***) returned
the underlying integral type, but the following change:

Mon Oct 28 13:08:51 1996  Richard Kenner  (kenner@vlsi1.ultra.nyu.edu)
	(signed_or_unsigned_type): If already right signedness, return.

caused signed_or_unsigned_type to return the enumerated type itself
rather than the underlying integral type.  

Thus maxval and minval refer to the maximum and minimum of the
declared enum values rather than of the underlying integral type, and
the subsequent logic optimizes away the comparison.

My proposed patch fixes this.


--------------------------------------------

Excerpt from shorten_compare(c_common.c):

  /* If comparing an integer against a constant more bits wide,
     maybe we can deduce a value of 1 or 0 independent of the data.
     Or else truncate the constant now
     rather than extend the variable at run time.

     This is only interesting if the constant is the wider arg.
     Also, it is not safe if the constant is unsigned and the
     variable arg is signed, since in this case the variable
     would be sign-extended and then regarded as unsigned.
     Our technique fails in this case because the lowest/highest
     possible unsigned results don't follow naturally from the
     lowest/highest possible values of the variable operand.
     For just EQ_EXPR and NE_EXPR there is another technique that
     could be used: see if the constant can be faithfully represented
     in the other operand's type, by truncating it and reextending it
     and see if that preserves the constant's value.  */

  if (!real1 && !real2
      && TREE_CODE (primop1) == INTEGER_CST
      && TYPE_PRECISION (TREE_TYPE (primop0)) < TYPE_PRECISION (*restype_ptr))
    {
      int min_gt, max_gt, min_lt, max_lt;
      tree maxval, minval;
      /* 1 if comparison is nominally unsigned.  */
      int unsignedp = TREE_UNSIGNED (*restype_ptr);
      tree val;

[***] type = signed_or_unsigned_type (unsignedp0, TREE_TYPE (primop0));

      maxval = TYPE_MAX_VALUE (type);
      minval = TYPE_MIN_VALUE (type);

      if (unsignedp && !unsignedp0)
	*restype_ptr = signed_type (*restype_ptr);

      if (TREE_TYPE (primop1) != *restype_ptr)
	primop1 = convert (*restype_ptr, primop1);
      if (type != *restype_ptr)
	{
	  minval = convert (*restype_ptr, minval);
	  maxval = convert (*restype_ptr, maxval);
	}

      if (unsignedp && unsignedp0)
	{
	  min_gt = INT_CST_LT_UNSIGNED (primop1, minval);
	  max_gt = INT_CST_LT_UNSIGNED (primop1, maxval);
	  min_lt = INT_CST_LT_UNSIGNED (minval, primop1);
	  max_lt = INT_CST_LT_UNSIGNED (maxval, primop1);
	}
      else
	{
	  min_gt = INT_CST_LT (primop1, minval);
	  max_gt = INT_CST_LT (primop1, maxval);
	  min_lt = INT_CST_LT (minval, primop1);
	  max_lt = INT_CST_LT (maxval, primop1);
	}

      val = 0;
      /* This used to be a switch, but Genix compiler can't handle that.  */
      if (code == NE_EXPR)
	{
	  if (max_lt || min_gt)
	    val = boolean_true_node;
	}
      else if (code == EQ_EXPR)
	{
	  if (max_lt || min_gt)
	    val = boolean_false_node;
	}
      else if (code == LT_EXPR)
	{
	  if (max_lt)
	    val = boolean_true_node;
	  if (!min_lt)
	    val = boolean_false_node;
	}
      else if (code == GT_EXPR)
	{
	  if (min_gt)
	    val = boolean_true_node;
	  if (!max_gt)
	    val = boolean_false_node;
	}
      else if (code == LE_EXPR)
	{
	  if (!max_gt)
	    val = boolean_true_node;
	  if (min_gt)
	    val = boolean_false_node;
	}
      else if (code == GE_EXPR)
	{
	  if (!min_lt)
	    val = boolean_true_node;
	  if (max_lt)
	    val = boolean_false_node;
	}

Re: [PATCH] Fix for short-enums comparison bug

[Jeffrey A Law]

  In message <x490ee6l2e.fsf@janus.pgt.com>you write:
  > The following message is a courtesy copy of an article
  > that has been posted to gnu.gcc,gnu.gcc.bug as well.
Thanks.


  > I provide below a one-line patch which fixes the bug; however I'd like
  > to provide a little bit of explanation about the nature of the bug and
  > how I found it.  My apologies if this is a bit long.
No need to apologize, it is precisely the kind of analysis that makes my
job of reviewing patches a whole lot easier :-)

  > Here's a very simple example which illustrates the bug in question:
  > 
  > typedef enum {A=0, B, C, D} T;
  > main(){
  >   T x;
  >   for (x=A; x<=D; ++x)
  >     printf("%d ", (int)x);
  >   putchar('\n');
  > }
  > 
  > If you compile this with no compiler flags set and run it will print 
  > 
  > 0 1 2 3  
  > 
  > as expected.  If you compile with -fshort-enums it will loop
  > indefinitely, printing all values from 0-255 over and over.
I'm not a language lawyer -- we really need one to determine what the behavior
of that code should be.

I don't have an ANSI C standard here, nor do I have a copy of the ISO C9X
standard.  Can someone who does dig into them and determine what happens for
an assignment out of the range of an enumerated type and what assumptions the
compiler can/can not make about the values an enumerated variable can have?


  > Try running this with and without the -fshort-enums flag.  If the
  > intent of -fshort-enums is to really restrict the value of an
  > enumerated type to lie within the min and max of that type, a warning
  > should be produced when we set x equal to 10.  But no such warning is
  > produced, just bizarre results (the program prints 10<=3 if
  > -fshort-enums is used).
Actually, whether or not a warning must be produced is a standards issue.  Even
if the standards don't demand it, a warning would be nice.  Someone would have
to grovel around the front-end though to produce the warning.  Once we've
left the language front end it's too late.


Jeff

Re: [PATCH] Fix for short-enums comparison bug

[Joe Buck]

> My quick read of the C++ standard made it appear as if assigning a value that
> was not a member of the enum resulted in undefined results.

Wrong.  The ISO C++ standard defines what values must be assignable to the
enum, and it is a larger set of values than just the members.  I don't
have the language in front of me, but the intent is to require the ability
to hold intermediate values and AND and OR of legal values.  Thus code
like the following is standard-conformant:

enum flags {
	flag_none = 0x0,
	flag_a = 0x1,
	flag_b = 0x2,
	flag_c = 0x4,
	flag_d = 0x8
};

...
	flags foo = (flags)(flag_a | flag+b);	// OK
	flags bar = (flags) 12;			// OK
	flags xyz = (flags) 23;			// not OK

the last one is not guaranteed to work because the compiler is allowed to
use a four-bit unsigned field and 23 will not fit.

> case where C++ and C differ?

Re: [PATCH] Fix for short-enums comparison bug

[Jeffrey A Law]

  In message < 199902112216.RAA27319@wagner.Princeton.EDU >you write:
  > I seriously doubt it, since arguments to integer operators undergo
  > integral promotion.  The rules are different for C++, but I believe
  > the answer is no, since none of the standard integral types is allowed
  > to have only 2 bits.
I'm not looking for "I doubt/I believe".  I need folks to actually check the
relavent standard and say "The standard ..." :-)

jeff

Re: [PATCH] Fix for short-enums comparison bug

[Jeffrey A Law]

  In message < 199902111942.LAA16560@atrus.synopsys.com >you write:
  > Wrong.  The ISO C++ standard defines what values must be assignable to the
  > enum, and it is a larger set of values than just the members.  I don't
  > have the language in front of me, but the intent is to require the ability
  > to hold intermediate values and AND and OR of legal values.  Thus code
  > like the following is standard-conformant:
  > 
  > enum flags {
  > 	flag_none = 0x0,
  > 	flag_a = 0x1,
  > 	flag_b = 0x2,
  > 	flag_c = 0x4,
  > 	flag_d = 0x8
  > };
  > 
  > ...
  > 	flags foo = (flags)(flag_a | flag+b);	// OK
  > 	flags bar = (flags) 12;			// OK
  > 	flags xyz = (flags) 23;			// not OK
  > 
  > the last one is not guaranteed to work because the compiler is allowed to
  > use a four-bit unsigned field and 23 will not fit.
OK.  Thanks.  So if we go back to the original sample code:


typedef enum {A=0, B, C, D} T;
main(){
  T x;
  for (x=A; x<=D; ++x)
    printf("%d ", (int)x);
  putchar('\n');
}

So the compiler could use a 2 bit unsigned field for x since the values for 
enum T are 0, 1, 2, 3.  Other values will not fit and would be considered
invalid.  Thus removing the test x <= D is technically valid for C++.  Right?

That (of course) isn't binding for C.

jeff

Re: [PATCH] Fix for short-enums comparison bug

[Gavin Romig-Koch]

Jeffrey A Law writes:
 > Yes the underlying type must be capable of representing all the values for
 > the enum.  But does the enum restrict the actual values which can be used
 > with the expected results?

The enum can restrict the actual values which can be used to the values
of the choosen underlying type.  The underlying type must be an existing
type (the compiler can't make up a new "tiny" type for the underlying
type).  If you know the underlying type, you know what values can be
used.  

 > My quick read of the C++ standard made it appear as if assigning a value that
 > was not a member of the enum resulted in undefined results.  Is this possibly a
 > case where C++ and C differ?

Probably.  There was talk in the C++ committee to restrict the usable
values farther than C does, but I don't remember the outcome of that
discussion.  c9x kept the old C rules.

                                      -gavin...
          

Re: [PATCH] Fix for short-enums comparison bug

[Joe Buck]

>   In message < 199902112216.RAA27319@wagner.Princeton.EDU >you write:
>   > I seriously doubt it, since arguments to integer operators undergo
>   > integral promotion.  The rules are different for C++, but I believe
>   > the answer is no, since none of the standard integral types is allowed
>   > to have only 2 bits.
> I'm not looking for "I doubt/I believe".  I need folks to actually check the
> relavent standard and say "The standard ..." :-)

The relevant part of the C++ standard has the label "dcl.enum": look
for .../dcl.html#dcl.enum in your favorite copy of the standard (I
assume you have an up-to-date version inside Cygnus).

I'm currently looking at the Dec. 96 draft, so I don't know if there were
changes made later (you can check your local copy).  Two things are
described: the legal integral values that can be assigned to the enum, and
the "underlying type", which is implementation-dependent but can't be
larger than int unless there are enum values that don't fit in int or
unsigned int.

Stroustrup "The C++ Programming Language, Third Edition"'s section 4.8 may
be a clearer explanation of what is intended (though of course it is
non-normative).

Re: [PATCH] Fix for short-enums comparison bug

[Mike Stump]

> To: Charles G Waldman <cgw@alum.mit.edu>
> Date: Thu, 11 Feb 1999 00:46:40 -0700
> From: Jeffrey A Law <law@hurl.cygnus.com>

> I'm not a language lawyer -- we really need one to determine what
> the behavior of that code should be.

I disagree.  The gcc documentation has enough.  It says that a
integral number of bytes are used to represent the enum.  And only if
it were a bit field, would an integral number of bits be used.

If a byte is used 4+1 = 5, 5 is > 4, so the loop must stop.  If we
used bitfields, 4+1=0 (when we use 2 bits for the enum).  But the
documentation clearly states and the users expectation is we use a
byte.

Now, if we want to change to document, we _can_ entertain this, and we
can allocate just the number of bits required.  We are allowed to do
this optimization in C++ certainly (if the user can't otherwise tell).
If we do this, then the behavior of the program _is_ correct.

> Date: Thu, 11 Feb 1999 10:31:58 -0700
> From: Jeffrey A Law <law@hurl.cygnus.com>

> Yes the underlying type must be capable of representing all the
> values for the enum.  But does the enum restrict the actual values
> which can be used with the expected results?

It does (for C++), but we don't need to make use of that much latitude
in the standard.  It we can reasonably maye _more_ code work, even
though strickly speaking it isn't portable, we should.

Re: [PATCH] Fix for short-enums comparison bug

[Mike Stump]

> To: Charles G Waldman <cgw@alum.mit.edu>
> Date: Thu, 11 Feb 1999 00:46:40 -0700
> From: Jeffrey A Law <law@hurl.cygnus.com>

> I'm not a language lawyer -- we really need one to determine what
> the behavior of that code should be.

I disagree.  The gcc documentation has enough.  It says that a
integral number of bytes are used to represent the enum.  And only if
it were a bit field, would an integral number of bits be used.

If a byte is used 4+1 = 5, 5 is > 4, so the loop must stop.  If we
used bitfields, 4+1=0 (when we use 2 bits for the enum).  But the
documentation clearly states and the users expectation is we use a
byte.

Now, if we want to change to document, we _can_ entertain this, and we
can allocate just the number of bits required.  We are allowed to do
this optimization in C++ certainly (if the user can't otherwise tell).
If we do this, then the behavior of the program _is_ correct.

> Date: Thu, 11 Feb 1999 10:31:58 -0700
> From: Jeffrey A Law <law@hurl.cygnus.com>

> Yes the underlying type must be capable of representing all the
> values for the enum.  But does the enum restrict the actual values
> which can be used with the expected results?

It does (for C++), but we don't need to make use of that much latitude
in the standard.  It we can reasonably maye _more_ code work, even
though strickly speaking it isn't portable, we should.