[PATCH] Make large enum constants unsigned

[PATCH] Make large enum constants unsigned

[Stefan Kristiansson]

Hi,

this fixes a bug noted in two threads on the binutils mailing list:
https://sourceware.org/ml/binutils/2014-05/msg00152.html
https://sourceware.org/ml/binutils/2014-05/msg00195.html

Both threads describe the problem pretty well, but the gist of it
is that constants are generated that will be interpreted as signed.

2014-05-22  Stefan Kristiansson  <stefan.kristiansson@saunalahti.fi>

	* enum.scm (gen-enum-decl): Emit 'ULL' after constants larger than
	 #x80000000

Index: cgen/enum.scm
===================================================================
RCS file: /cvs/src/src/cgen/enum.scm,v
retrieving revision 1.13
diff -u -r1.13 enum.scm
--- cgen/enum.scm	13 Feb 2010 03:39:15 -0000	1.13
+++ cgen/enum.scm	22 May 2014 05:23:23 -0000
@@ -298,7 +298,10 @@
 		     ""
 		     (string-append " = "
 				    (if (number? (cadr e))
-					(number->string (cadr e))
+					(string-append (number->string (cadr e))
+						       (if (> (cadr e)
+							      #x80000000)
+							   "ULL" ""))
 					(cadr e))))
 		 ))
 	       (if (and san? include-sanitize-marker?)

Re: [PATCH] Make large enum constants unsigned

[Maciej W. Rozycki]

On Thu, 22 May 2014, Stefan Kristiansson wrote:

> this fixes a bug noted in two threads on the binutils mailing list:
> https://sourceware.org/ml/binutils/2014-05/msg00152.html
> https://sourceware.org/ml/binutils/2014-05/msg00195.html
> 
> Both threads describe the problem pretty well, but the gist of it
> is that constants are generated that will be interpreted as signed.
> 
> 2014-05-22  Stefan Kristiansson  <stefan.kristiansson@saunalahti.fi>
> 
> 	* enum.scm (gen-enum-decl): Emit 'ULL' after constants larger than
> 	 #x80000000

 Well, `ULL' is non-standard for pre-C99 compilers and also not needed 
because an enum will never have a type that is wider than `int'.  So for 
portability's sake I suggest that you use `U' or `u' as I proposed (I 
prefer lowercase `u' for decimal constants because I find it easier to 
spot among digits, however please feel free to take your pick).

  Maciej

Re: [PATCH] Make large enum constants unsigned

[Stefan Kristiansson]

On Thu, May 22, 2014 at 9:21 AM, Maciej W. Rozycki
<macro@codesourcery.com> wrote:
> On Thu, 22 May 2014, Stefan Kristiansson wrote:
>
>> this fixes a bug noted in two threads on the binutils mailing list:
>> https://sourceware.org/ml/binutils/2014-05/msg00152.html
>> https://sourceware.org/ml/binutils/2014-05/msg00195.html
>>
>> Both threads describe the problem pretty well, but the gist of it
>> is that constants are generated that will be interpreted as signed.
>>
>> 2014-05-22  Stefan Kristiansson  <stefan.kristiansson@saunalahti.fi>
>>
>>       * enum.scm (gen-enum-decl): Emit 'ULL' after constants larger than
>>        #x80000000
>
>  Well, `ULL' is non-standard for pre-C99 compilers and also not needed
> because an enum will never have a type that is wider than `int'.  So for
> portability's sake I suggest that you use `U' or `u' as I proposed (I
> prefer lowercase `u' for decimal constants because I find it easier to
> spot among digits, however please feel free to take your pick).
>

Hmm, right.
I actually had a plain 'U' in my first version of the patch, but then
changed it to 'ULL' last minute with the thought of being future proof
against possible larger mask values.
But as you point out, that will have issues anyway, so I'll repost
with the original patch.

As for 'u' vs 'U', a 'U' is emitted in the .h file even if I put a 'u'
in the .scm.
If 'u' in the output is preferred, maybe someone can point out the
reason for this?

Stefan

Re: [PATCH] Make large enum constants unsigned

[Stefan Kristiansson]

On Thu, May 22, 2014 at 09:52:50AM +0300, Stefan Kristiansson wrote:
> 
> As for 'u' vs 'U', a 'U' is emitted in the .h file even if I put a 'u'
> in the .scm.
> If 'u' in the output is preferred, maybe someone can point out the
> reason for this?
> 

I found the reason, there's a 'string-upcase' a couple of rows above
what the patch touches.
I'm not sure getting around that is worth pursuing though?

Anyway, below is the updated patch.

2014-05-22  Stefan Kristiansson  <stefan.kristiansson@saunalahti.fi>

	* enum.scm (gen-enum-decl): Emit 'U' after constants larger than
	 #x80000000

Index: cgen/enum.scm
===================================================================
RCS file: /cvs/src/src/cgen/enum.scm,v
retrieving revision 1.13
diff -u -r1.13 enum.scm
--- cgen/enum.scm	13 Feb 2010 03:39:15 -0000	1.13
+++ cgen/enum.scm	22 May 2014 07:09:00 -0000
@@ -298,7 +298,10 @@
 		     ""
 		     (string-append " = "
 				    (if (number? (cadr e))
-					(number->string (cadr e))
+					(string-append (number->string (cadr e))
+						       (if (> (cadr e)
+							      #x80000000)
+							   "U" ""))
 					(cadr e))))
 		 ))
 	       (if (and san? include-sanitize-marker?)

RE: [PATCH] Make large enum constants unsigned

[Pierre Muller]

  Couldn't we support 64-bit constants already,
by adding
+						       (if (> (cadr e)
+							      #x100000000)
+							   "LL" ""))
Right after the conditional "U" postfix?

This way, the pre C-99 standard would only be violated when
it is really required anyhow, no?

Pierre Muller

> -----Message d'origine-----
> De : Stefan Kristiansson [mailto:stefan.kristiansson@saunalahti.fi]
> Envoyé : jeudi 22 mai 2014 09:10
> À : Maciej W. Rozycki
> Cc : cgen@sourceware.org; Alan Modra; fche@sourceware.org; Christian
> Svensson; Pierre Muller
> Objet : Re: [PATCH] Make large enum constants unsigned
> 
> On Thu, May 22, 2014 at 09:52:50AM +0300, Stefan Kristiansson wrote:
> >
> > As for 'u' vs 'U', a 'U' is emitted in the .h file even if I put a
> 'u'
> > in the .scm.
> > If 'u' in the output is preferred, maybe someone can point out the
> > reason for this?
> >
> 
> I found the reason, there's a 'string-upcase' a couple of rows above
> what the patch touches.
> I'm not sure getting around that is worth pursuing though?
> 
> Anyway, below is the updated patch.
> 
> 2014-05-22  Stefan Kristiansson  <stefan.kristiansson@saunalahti.fi>
> 
> 	* enum.scm (gen-enum-decl): Emit 'U' after constants larger than
> 	 #x80000000
> 
> Index: cgen/enum.scm
> ===================================================================
> RCS file: /cvs/src/src/cgen/enum.scm,v
> retrieving revision 1.13
> diff -u -r1.13 enum.scm
> --- cgen/enum.scm	13 Feb 2010 03:39:15 -0000	1.13
> +++ cgen/enum.scm	22 May 2014 07:09:00 -0000
> @@ -298,7 +298,10 @@
>  		     ""
>  		     (string-append " = "
>  				    (if (number? (cadr e))
> -					(number->string (cadr e))
> +					(string-append (number->string (cadr
e))
> +						       (if (> (cadr e)
> +							      #x80000000)
> +							   "U" ""))
>  					(cadr e))))
>  		 ))
>  	       (if (and san? include-sanitize-marker?)

Re: [PATCH] Make large enum constants unsigned

[Stefan Kristiansson]

On Thu, May 22, 2014 at 4:33 PM, Pierre Muller
<pierre.muller@ics-cnrs.unistra.fr> wrote:
>   Couldn't we support 64-bit constants already,
> by adding
> +                                                      (if (> (cadr e)
> +                                                             #x100000000)
> +                                                          "LL" ""))
> Right after the conditional "U" postfix?
>
> This way, the pre C-99 standard would only be violated when
> it is really required anyhow, no?
>

That would solve the standard violation problem, but the fact that
enums are limited to the size of an int still remains, so it wouldn't
help much.

Stefan

Re: [PATCH] Make large enum constants unsigned

[Frank Ch. Eigler]

Hi -

> this fixes a bug noted in two threads on the binutils mailing list:
> https://sourceware.org/ml/binutils/2014-05/msg00152.html
> https://sourceware.org/ml/binutils/2014-05/msg00195.html
> 
> Both threads describe the problem pretty well, but the gist of it
> is that constants are generated that will be interpreted as signed.
> 
> 2014-05-22  Stefan Kristiansson  <stefan.kristiansson@saunalahti.fi>
> 
> 	* enum.scm (gen-enum-decl): Emit 'ULL' after constants larger than
> 	 #x80000000
> [...]

> -					(number->string (cadr e))
> +					(string-append (number->string (cadr e))
> +						       (if (> (cadr e)
> +							      #x80000000)
> +							   "ULL" ""))

Have you thought of having cgen emit it as

     (string-append "(signed int)" (number->string (cadr e)))

... or even find a number->string conversion function that takes into
account the destionation language's encoding constraints.

- FChE

Re: [PATCH] Make large enum constants unsigned

[Maciej W. Rozycki]

On Thu, 22 May 2014, Stefan Kristiansson wrote:

> I found the reason, there's a 'string-upcase' a couple of rows above
> what the patch touches.
> I'm not sure getting around that is worth pursuing though?

 Not for me.

  Maciej

Re: [PATCH] Make large enum constants unsigned

[Stefan Kristiansson]

On Thu, May 22, 2014 at 6:09 PM, Frank Ch. Eigler <fche@elastic.org> wrote:
> Have you thought of having cgen emit it as
>
>      (string-append "(signed int)" (number->string (cadr e)))
>

Ignoring the 'string-upcase' that would turn that into "(SIGNED INT)",
how would that be better than?

      (string-append (number->string (cadr e)) "U")


I dismissed that, since it would create a lot larger diff in generated
files than in the isolated cases where it's actually a problem.

> ... or even find a number->string conversion function that takes into
> account the destionation language's encoding constraints.
>

That might certainly be an option, but way over my current Scheme capabilities.
If you give me some pointers I could try to improve them.

Stefan

Re: [PATCH] Make large enum constants unsigned

[Frank Ch. Eigler]

Hi -

> >      (string-append "(signed int)" (number->string (cadr e)))
> [...]
> how would that be better than?
>       (string-append (number->string (cadr e)) "U")

It would be a closer match to expressing our desire to match C enum
typing, but I'd be fine with "U" suffixing too.


> I dismissed that, since it would create a lot larger diff in generated
> files than in the isolated cases where it's actually a problem. [...]

IMHO, that's not such a big deal.  We don't usually look closely at
the diffs of generated files, once the generating tools are trustworthy.


- FChE

Re: [PATCH] Make large enum constants unsigned

[Maciej W. Rozycki]

On Thu, 22 May 2014, Frank Ch. Eigler wrote:

> > >      (string-append "(signed int)" (number->string (cadr e)))
> > [...]
> > how would that be better than?
> >       (string-append (number->string (cadr e)) "U")
> 
> It would be a closer match to expressing our desire to match C enum
> typing, but I'd be fine with "U" suffixing too.

 TBH I don't understand what you mean here, a C compiler is free to choose 
from `char', `int' and `unsigned int' as the underlying (compatible) type 
of an enumeration as long as the type can represent all the member values 
defined.  How exactly the signed `int' type is going to be advantageous 
over the `unsigned int' type here?  If this is used to produce a bit mask, 
which I gather it is, then I find using an unsigned type more natural.

  Maciej

Re: [PATCH] Make large enum constants unsigned

[Frank Ch. Eigler]

Hi -

> > > >      (string-append "(signed int)" (number->string (cadr e)))
> > > [...]
> > > how would that be better than?
> > >       (string-append (number->string (cadr e)) "U")
> > 
> > It would be a closer match to expressing our desire to match C enum
> > typing, but I'd be fine with "U" suffixing too.
> 
>  TBH I don't understand what you mean here, a C compiler is free to choose 
> from `char', `int' and `unsigned int' as the underlying (compatible) type 
> of an enumeration as long as the type can represent all the member values 
> defined.  [...]

I'm not a language lawyer, and in different levels of the language
this has changed, but my understanding is that to be most compatible,
the enumerators (the given literals) need to be 'int's.  (It's a
separate question as to what type the compiler would allocate to an
enum FOO variable.)


> over the `unsigned int' type here?  If this is used to produce a bit mask, 
> which I gather it is, then I find using an unsigned type more natural.

I see what you mean.


- FChE

Re: [PATCH] Make large enum constants unsigned

[Stefan Kristiansson]

On Thu, May 22, 2014 at 05:06:21PM -0400, Frank Ch. Eigler wrote:
> Hi -
> 
> > >      (string-append "(signed int)" (number->string (cadr e)))
> > [...]
> > how would that be better than?
> >       (string-append (number->string (cadr e)) "U")
> 
> It would be a closer match to expressing our desire to match C enum
> typing, but I'd be fine with "U" suffixing too.
> 
> 
> > I dismissed that, since it would create a lot larger diff in generated
> > files than in the isolated cases where it's actually a problem. [...]
> 
> IMHO, that's not such a big deal.  We don't usually look closely at
> the diffs of generated files, once the generating tools are trustworthy.
> 

Ok, that's fine by me if others are ok with it.
Below is the patch that does that.

2014-05-29  Stefan Kristiansson  <stefan.kristiansson@saunalahti.fi>

	* enum.scm (gen-enum-decl): Emit 'U' after constants.

Index: enum.scm
===================================================================
RCS file: /cvs/src/src/cgen/enum.scm,v
retrieving revision 1.13
diff -u -r1.13 enum.scm
--- enum.scm	13 Feb 2010 03:39:15 -0000	1.13
+++ enum.scm	29 May 2014 20:08:12 -0000
@@ -298,7 +298,8 @@
 		     ""
 		     (string-append " = "
 				    (if (number? (cadr e))
-					(number->string (cadr e))
+					(string-append (number->string (cadr e))
+						       "U")
 					(cadr e))))
 		 ))
 	       (if (and san? include-sanitize-marker?)

Re: [PATCH] Make large enum constants unsigned

[Frank Ch. Eigler]

Hi -

> [...]
> Ok, that's fine by me if others are ok with it.
> Below is the patch that does that.
> 
> 2014-05-29  Stefan Kristiansson  <stefan.kristiansson@saunalahti.fi>
> 
> 	* enum.scm (gen-enum-decl): Emit 'U' after constants.
> [...]

One last piece of homework: can you identify in your patch the range
of c compilers / standards-compliance-CFLAGS tested with this change?
I'm a bit worried that U-suffixing may be rejected by earlier or
non-gnu compilers.  

A little experiment shows, gcc 4.8 with -ansi -pedantic outright
rejects large integers, with or without U suffix.

 gcc  -ansi -pedantic -c foo.c     
foo.c:1:14: warning: ISO C restricts enumerator values to range of ‘int’ [-Wpedantic]
 enum i { x = 4000000000U };

It accepts with a     (int)4000000000    casting formulation instead.

- FChE