[PATCH] newlib-stdint.h: Remove 32 bit longs

[PATCH] newlib-stdint.h: Remove 32 bit longs

[Andy Ross]

We ran into this issue in the Zephyr project with our toolchain (gcc
built with --enable-newlib).  Basically GCC appears to be honoring a
legacy requirement to give newlib a "long" instead of "int" for
__INT32_TYPE__, which then leaks out through the current newlib
headers as a long-valued int32_t, which produces gcc warnings when a
uint32_t is passed to an unqualified printf format specifier like
"%d".

But the newlib headers, if __INT32_TYPE__ is *not* defined by the
compiler, have code to chose a "int" over "long" immediately
thereafter.  It seems whatever requirement this was honoring isn't
valid anymore.

From 784fb1760a930d0309f878bbae7bfd38137f5689 Mon Sep 17 00:00:00 2001
From: Andy Ross <andrew.j.ross@intel.com>
Date: Fri, 19 Aug 2016 09:40:42 -0700
Subject: [PATCH] newlib-stdint.h: Remove 32 bit longs

This would make __INT32_TYPE__ a "long" instead of an "int", which
would then percolate down in newlib's own headers into a typedef for
int32_t.  Which is wrong.  Newlib's headers, if __INT32_TYPE__ were
not defined, actually would chose an int for this type.  The comment
that newlib uses a 32 bit long appears to be a lie, perhaps
historical.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
---
 gcc/config/newlib-stdint.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gcc/config/newlib-stdint.h b/gcc/config/newlib-stdint.h
index eb99556..0275948 100644
--- a/gcc/config/newlib-stdint.h
+++ b/gcc/config/newlib-stdint.h
@@ -22,10 +22,9 @@ a copy of the GCC Runtime Library Exception along with this program;
 see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 <http://www.gnu.org/licenses/>.  */

-/* newlib uses 32-bit long in certain cases for all non-SPU
-   targets.  */
+/* newlib used to use a 32-bit long, no longer */
 #ifndef STDINT_LONG32
-#define STDINT_LONG32 (LONG_TYPE_SIZE == 32)
+#define STDINT_LONG32 0
 #endif

 #define SIG_ATOMIC_TYPE "int"
-- 
2.7.4

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Andrew Pinski]

On Fri, Aug 19, 2016 at 12:16 PM, Andy Ross <andrew.j.ross@intel.com> wrote:
> We ran into this issue in the Zephyr project with our toolchain (gcc
> built with --enable-newlib).  Basically GCC appears to be honoring a
> legacy requirement to give newlib a "long" instead of "int" for
> __INT32_TYPE__, which then leaks out through the current newlib
> headers as a long-valued int32_t, which produces gcc warnings when a
> uint32_t is passed to an unqualified printf format specifier like
> "%d".
>
> But the newlib headers, if __INT32_TYPE__ is *not* defined by the
> compiler, have code to chose a "int" over "long" immediately
> thereafter.  It seems whatever requirement this was honoring isn't
> valid anymore.

Couple of things missing here.  A changelog is the first thing.
The second thing is it seems like Zephyr project should be using the
PRIdx, etc. instead of just %d for int32_t to be portable.

Thanks,
Andrew


>
> From 784fb1760a930d0309f878bbae7bfd38137f5689 Mon Sep 17 00:00:00 2001
> From: Andy Ross <andrew.j.ross@intel.com>
> Date: Fri, 19 Aug 2016 09:40:42 -0700
> Subject: [PATCH] newlib-stdint.h: Remove 32 bit longs
>
> This would make __INT32_TYPE__ a "long" instead of an "int", which
> would then percolate down in newlib's own headers into a typedef for
> int32_t.  Which is wrong.  Newlib's headers, if __INT32_TYPE__ were
> not defined, actually would chose an int for this type.  The comment
> that newlib uses a 32 bit long appears to be a lie, perhaps
> historical.
>
> Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
> ---
>  gcc/config/newlib-stdint.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/config/newlib-stdint.h b/gcc/config/newlib-stdint.h
> index eb99556..0275948 100644
> --- a/gcc/config/newlib-stdint.h
> +++ b/gcc/config/newlib-stdint.h
> @@ -22,10 +22,9 @@ a copy of the GCC Runtime Library Exception along with this program;
>  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>  <http://www.gnu.org/licenses/>.  */
>
> -/* newlib uses 32-bit long in certain cases for all non-SPU
> -   targets.  */
> +/* newlib used to use a 32-bit long, no longer */
>  #ifndef STDINT_LONG32
> -#define STDINT_LONG32 (LONG_TYPE_SIZE == 32)
> +#define STDINT_LONG32 0
>  #endif
>
>  #define SIG_ATOMIC_TYPE "int"
> --
> 2.7.4
>

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Joel Sherrill]

RTEMS uses the PRI constants and we don't see warnings. Is there a specific test case which would demonstrate this is actually broken. The file newlib-stdint.h will impact more targets than Zephyr and I think they owe a demo case.

On August 19, 2016 7:37:22 PM EDT, Andrew Pinski <pinskia@gmail.com> wrote:
>On Fri, Aug 19, 2016 at 12:16 PM, Andy Ross <andrew.j.ross@intel.com>
>wrote:
>> We ran into this issue in the Zephyr project with our toolchain (gcc
>> built with --enable-newlib).  Basically GCC appears to be honoring a
>> legacy requirement to give newlib a "long" instead of "int" for
>> __INT32_TYPE__, which then leaks out through the current newlib
>> headers as a long-valued int32_t, which produces gcc warnings when a
>> uint32_t is passed to an unqualified printf format specifier like
>> "%d".
>>
>> But the newlib headers, if __INT32_TYPE__ is *not* defined by the
>> compiler, have code to chose a "int" over "long" immediately
>> thereafter.  It seems whatever requirement this was honoring isn't
>> valid anymore.
>
>Couple of things missing here.  A changelog is the first thing.
>The second thing is it seems like Zephyr project should be using the
>PRIdx, etc. instead of just %d for int32_t to be portable.
>
>Thanks,
>Andrew
>
>
>>
>> From 784fb1760a930d0309f878bbae7bfd38137f5689 Mon Sep 17 00:00:00
>2001
>> From: Andy Ross <andrew.j.ross@intel.com>
>> Date: Fri, 19 Aug 2016 09:40:42 -0700
>> Subject: [PATCH] newlib-stdint.h: Remove 32 bit longs
>>
>> This would make __INT32_TYPE__ a "long" instead of an "int", which
>> would then percolate down in newlib's own headers into a typedef for
>> int32_t.  Which is wrong.  Newlib's headers, if __INT32_TYPE__ were
>> not defined, actually would chose an int for this type.  The comment
>> that newlib uses a 32 bit long appears to be a lie, perhaps
>> historical.
>>
>> Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
>> ---
>>  gcc/config/newlib-stdint.h | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/gcc/config/newlib-stdint.h b/gcc/config/newlib-stdint.h
>> index eb99556..0275948 100644
>> --- a/gcc/config/newlib-stdint.h
>> +++ b/gcc/config/newlib-stdint.h
>> @@ -22,10 +22,9 @@ a copy of the GCC Runtime Library Exception along
>with this program;
>>  see the files COPYING3 and COPYING.RUNTIME respectively.  If not,
>see
>>  <http://www.gnu.org/licenses/>.  */
>>
>> -/* newlib uses 32-bit long in certain cases for all non-SPU
>> -   targets.  */
>> +/* newlib used to use a 32-bit long, no longer */
>>  #ifndef STDINT_LONG32
>> -#define STDINT_LONG32 (LONG_TYPE_SIZE == 32)
>> +#define STDINT_LONG32 0
>>  #endif
>>
>>  #define SIG_ATOMIC_TYPE "int"
>> --
>> 2.7.4
>>

--joel

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Andy Ross]

The reproduction is straightforward.  Just build any cross gcc with
--enable-newlib (e.g. the one in the Zephyr SDK) and compile this
(on any 32 or 64 bit 2's complement architecture) with newlib's
headers.

    #include <stdint.h>

    extern void takes_fmt(const char *fmt, ...)
        __attribute__ ((format (printf, 1, 2)));

    void foo()
    {
        int32_t x = 42;
        takes_fmt("%d", x);
    }

The use of int32_t with the untyped format specifier produces the
"expects argument of type ‘int’, but argument 2 has type ‘long int’"
warning despite the fact that this platform (it's just an i586
compiler!) is a standard ILP32 architecture.

The reason for that is this bit in newlib's headers where they trust
gcc if __INT32_TYPE__ is set:

    https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/machine/_default_types.h;h=ffc646d9e3f5392a643f8b4ca203a3ad2a7627d8;hb=HEAD#l62

And gcc, as seen by this patch, sets it to a long because it thinks
that's what newlib *wants*.

But if you look at the preprocessor code immediately following, newlib
doesn't want that at all.  If it doesn't find __INT32_TYPE__, it will
(see line 70) clearly choose a signed int, not a long.

Newlib doesn't want that at all.  This just seems like some kind of
historical mistake to me.  GCC's newlib "support" causes newlib code
to emit warnings on benign code that is unwarned in AFAIK literally
every other platform in existence.

(And I understand the point about the PRI stuff in inttypes.h, which
we're doing internally.  But that doesn't really address the bug in
our SDK compiler.)

Andy

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Andy Ross]

Same code.  Includes an attempt to format a change log entry in a more
gcc-friendly way.  I'm new here.

From c208b51fb55c6758c1059dfaee398c27da398e9d Mon Sep 17 00:00:00 2001
From: Andy Ross <andrew.j.ross@intel.com>
Date: Fri, 19 Aug 2016 09:40:42 -0700
Subject: [PATCH] newlib-stdint.h: Remove 32 bit longs

This would make __INT32_TYPE__ a "long" instead of an "int", which
would then percolate down in newlib's own headers into a typedef for
int32_t.  Which is wrong.  Newlib's headers, if __INT32_TYPE__ were
not defined, actually would chose an int for this type.  The comment
that newlib uses a 32 bit long appears to be a lie, perhaps
historical.

* gcc/config/newlib-stdint.h: synchronize 32 bit type macros with
  newlib's own headers.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
---
 gcc/config/newlib-stdint.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gcc/config/newlib-stdint.h b/gcc/config/newlib-stdint.h
index eb99556..0275948 100644
--- a/gcc/config/newlib-stdint.h
+++ b/gcc/config/newlib-stdint.h
@@ -22,10 +22,9 @@ a copy of the GCC Runtime Library Exception along with this program;
 see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 <http://www.gnu.org/licenses/>.  */

-/* newlib uses 32-bit long in certain cases for all non-SPU
-   targets.  */
+/* newlib used to use a 32-bit long, no longer */
 #ifndef STDINT_LONG32
-#define STDINT_LONG32 (LONG_TYPE_SIZE == 32)
+#define STDINT_LONG32 0
 #endif

 #define SIG_ATOMIC_TYPE "int"
-- 
2.7.4

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Andrew Pinski]

On Mon, Aug 22, 2016 at 9:09 AM, Andy Ross <andrew.j.ross@intel.com> wrote:
> The reproduction is straightforward.  Just build any cross gcc with
> --enable-newlib (e.g. the one in the Zephyr SDK) and compile this
> (on any 32 or 64 bit 2's complement architecture) with newlib's
> headers.
>
>     #include <stdint.h>
>
>     extern void takes_fmt(const char *fmt, ...)
>         __attribute__ ((format (printf, 1, 2)));
>
>     void foo()
>     {
>         int32_t x = 42;
>         takes_fmt("%d", x);
>     }
>
> The use of int32_t with the untyped format specifier produces the
> "expects argument of type ‘int’, but argument 2 has type ‘long int’"
> warning despite the fact that this platform (it's just an i586
> compiler!) is a standard ILP32 architecture.

Why do you think the above code does not have a bug in it?  int32_t is
long and changing it now is changing the ABI (especially for C++).
Before and after your patch take the following C++ program to see that
you just changed the ABI:
#include <stdint.h>

void f(int32_t a){}

>
> The reason for that is this bit in newlib's headers where they trust
> gcc if __INT32_TYPE__ is set:
>
>     https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/machine/_default_types.h;h=ffc646d9e3f5392a643f8b4ca203a3ad2a7627d8;hb=HEAD#l62
>
> And gcc, as seen by this patch, sets it to a long because it thinks
> that's what newlib *wants*.
>
> But if you look at the preprocessor code immediately following, newlib
> doesn't want that at all.  If it doesn't find __INT32_TYPE__, it will
> (see line 70) clearly choose a signed int, not a long.
>
> Newlib doesn't want that at all.  This just seems like some kind of
> historical mistake to me.  GCC's newlib "support" causes newlib code
> to emit warnings on benign code that is unwarned in AFAIK literally
> every other platform in existence.
>
> (And I understand the point about the PRI stuff in inttypes.h, which
> we're doing internally.  But that doesn't really address the bug in
> our SDK compiler.)

There is no bug in the compiler, just you misunderstanding that in32_t
can either be long or int and the reason why PRI were designed in the
first place.
I have fixed up many code which tries to use %d with int32_t inside
Cavium's simple-exec code and I don't see you can't do the same to
Zephyr (which is broken and not GCC/newlib).

Thanks,
Andrew

>
> Andy

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Andy Ross]

Andrew Pinski wrote:
> Why do you think the above code does not have a bug in it?  int32_t
> is long and changing it now is changing the ABI (especially for
> C++).

I don't follow.  There's no change to the ABI, the generated code is
identical in all cases.  Can you explain what you mean?

The problem here is that on 32 bit platforms (and *only* on 32 bit
platforms), gcc picks "long" for a 32 bit type in a way that confuses
newlib into using it for int32_t in a way that is technically legal,
but incompatible with the way the rest of the world (including newlib
itself when building with other compilers on the same platform!)
works.

So obvious code like I posted, which works everywhere, generates
warnings with a newlib cross compiler.  I would like to see that
fixed.

Andy

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Andrew Pinski]

On Mon, Aug 22, 2016 at 9:42 AM, Andy Ross <andrew.j.ross@intel.com> wrote:
> Andrew Pinski wrote:
>> Why do you think the above code does not have a bug in it?  int32_t
>> is long and changing it now is changing the ABI (especially for
>> C++).
>
> I don't follow.  There's no change to the ABI, the generated code is
> identical in all cases.  Can you explain what you mean?

The encoding of int and long are different in C++.  So if you have a
library built where you compiled it with the old compiler and try to
link it with the new one, the link will fail.
Again try my simple testcase and you will see that.

Thanks,
Andrew Pinski

>
> The problem here is that on 32 bit platforms (and *only* on 32 bit
> platforms), gcc picks "long" for a 32 bit type in a way that confuses
> newlib into using it for int32_t in a way that is technically legal,
> but incompatible with the way the rest of the world (including newlib
> itself when building with other compilers on the same platform!)
> works.
>
> So obvious code like I posted, which works everywhere, generates
> warnings with a newlib cross compiler.  I would like to see that
> fixed.
>
> Andy
>

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Andy Ross]

Andrew Pinski wrote:
> On Mon, Aug 22, 2016 at 9:42 AM, Andy Ross <andrew.j.ross@intel.com> wrote:
> > I don't follow.  There's no change to the ABI, the generated code is
> > identical in all cases.  Can you explain what you mean?
>
> The encoding of int and long are different in C++.  So if you have a
> library built where you compiled it with the old compiler and try to
> link it with the new one, the link will fail.  Again try my simple
> testcase and you will see that.

Ah, you're talking about symbol naming.  Yeah, good point.

That's sort of upsetting though.  What's the path to getting this
addressed then?  Like I said newlib is schizo in its own headers on
exactly this point, so it would already see the problem between C++
libraries compiled with gcc and putative other compilers that don't
set __INT32_TYPE__ in exactly the same way (I checked btw: clang does
set it, but not like gcc: it uses an "int" as there's no "newlib"
support to tell it otherwise).

I mean, it's sort of a mess.  Can't we just fix it?  Or deprecate
--enable-newlib or something?

Andy

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Joseph Myers]

On Mon, 22 Aug 2016, Andy Ross wrote:

> And gcc, as seen by this patch, sets it to a long because it thinks
> that's what newlib *wants*.

That would be because a newlib version that included the change

commit 843e635aaa02f16f314688ba5dd8a5edc3929095
Author: Jeff Johnston <jjohnstn@redhat.com>
Date:   Fri Dec 16 19:03:12 2005 +0000

    2005-12-16  Ralf Corsepius <ralf.corsepius@rtems.org>
    
        * libc/include/stdint.h: Prefer long over int for int32_t.
        Use __have_long32 to set up int32_t.
        * libc/include/inttypes.h: Use "#if xxx" instead of "#ifdef xxx"
        (Sync with stdint.h).

was the basis for GCC's understanding of the types used by newlib, but at 
some subsequent point newlib changed its choice of type again.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Richard Earnshaw]

On 22/08/16 17:09, Andy Ross wrote:
> The reproduction is straightforward.  Just build any cross gcc with
> --enable-newlib (e.g. the one in the Zephyr SDK) and compile this
> (on any 32 or 64 bit 2's complement architecture) with newlib's
> headers.
> 
>     #include <stdint.h>
> 
>     extern void takes_fmt(const char *fmt, ...)
>         __attribute__ ((format (printf, 1, 2)));
> 
>     void foo()
>     {
>         int32_t x = 42;
>         takes_fmt("%d", x);
>     }
> 

This code isn't portable.  %d means that the argument is of type 'int'
but there is no requirement that int32_t is equivalent to 'int'.

If you have an int32_t then you should use PRIi32 or PRId32 as
appropriate.  Of course, if the macros that those expand to still result
in warnings you probably have got a bug somewhere!

R.

> The use of int32_t with the untyped format specifier produces the
> "expects argument of type ‘int’, but argument 2 has type ‘long int’"
> warning despite the fact that this platform (it's just an i586
> compiler!) is a standard ILP32 architecture.
> 
> The reason for that is this bit in newlib's headers where they trust
> gcc if __INT32_TYPE__ is set:
> 
>     https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/machine/_default_types.h;h=ffc646d9e3f5392a643f8b4ca203a3ad2a7627d8;hb=HEAD#l62
> 
> And gcc, as seen by this patch, sets it to a long because it thinks
> that's what newlib *wants*.
> 
> But if you look at the preprocessor code immediately following, newlib
> doesn't want that at all.  If it doesn't find __INT32_TYPE__, it will
> (see line 70) clearly choose a signed int, not a long.
> 
> Newlib doesn't want that at all.  This just seems like some kind of
> historical mistake to me.  GCC's newlib "support" causes newlib code
> to emit warnings on benign code that is unwarned in AFAIK literally
> every other platform in existence.
> 
> (And I understand the point about the PRI stuff in inttypes.h, which
> we're doing internally.  But that doesn't really address the bug in
> our SDK compiler.)
> 
> Andy
> 

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Hans-Bernhard Bröker]

Am 22.08.2016 um 18:09 schrieb Andy Ross:
> The reproduction is straightforward.  Just build any cross gcc with
> --enable-newlib (e.g. the one in the Zephyr SDK) and compile this
> (on any 32 or 64 bit 2's complement architecture) with newlib's
> headers.
>
>     #include <stdint.h>
>
>     extern void takes_fmt(const char *fmt, ...)
>         __attribute__ ((format (printf, 1, 2)));
>
>     void foo()
>     {
>         int32_t x = 42;
>         takes_fmt("%d", x);
>     }
>

Let me boil this down:

That code is _wrong_.  Trying to base any decision on its behaviour can 
only be wrong, too.

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Andy Ross]

Hans-Bernhard Bröker wrote:
> Let me boil this down:
>
> That code is _wrong_.  Trying to base any decision on its behaviour
> can only be wrong, too.

Guys, I assure you I understand how the standards work, and tried to
make that clear.  The concern was self-consistency with what newlib is
using internally, and the printf warning was the easiest way to show
that.  But it turns out that the problem is deeper (e.g. the PRI*
macros in newlib only work with longs even when it picks an int for
int32_t using its own header logic) and probably not fixable in gcc as
I thought.

Andy

Re: [PATCH] newlib-stdint.h: Remove 32 bit longs

[Pavel Pisa]

Hello all,

I am not expert but I would like bring attention to some
facts which come to my mind

On Monday 22 of August 2016 18:19:06 Andy Ross wrote:
> --- a/gcc/config/newlib-stdint.h
> +++ b/gcc/config/newlib-stdint.h
> @@ -22,10 +22,9 @@ a copy of the GCC Runtime Library Exception along with
> this program; see the files COPYING3 and COPYING.RUNTIME respectively.  If
> not, see <http://www.gnu.org/licenses/>.  */
>
> -/* newlib uses 32-bit long in certain cases for all non-SPU
> -   targets.  */
> +/* newlib used to use a 32-bit long, no longer */
>  #ifndef STDINT_LONG32
> -#define STDINT_LONG32 (LONG_TYPE_SIZE == 32)
> +#define STDINT_LONG32 0
>  #endif

how this change would work on some H8 toolchain variants and
for each MSP430 build. They have int type only 16 bits
wide.

If there is available INT_TYPE_SIZE 

#ifndef STDINT_LONG32
#define STDINT_LONG32 (INT_TYPE_SIZE < 32)
#endif

could be OK. But INT_TYPE_SIZE seems is not used/available
in NewLib from the first glimpse.

But you get into significant troubles on Windows platform
with that probably because LONG is equivalent of int32_t
on both 32-bit and 64-bit platform there and most of the APIs
use it this way which means that all kind of warning would
appear if uint32_t *ptr is passed to LONG *ptr used in API.
It would change from long * and long * to
                     int * and long *

So I think that these problems should be considered before
change. Limiting use of int for int32_t only to 64-bit targets
with 64-bit long (as it is now) seems reasonable if used
consistent way.


Best wishes,

                Pavel