| Message ID | e7907443-eea0-87b5-87ba-fc34aa90c387@intel.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
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
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
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
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 >
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
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.
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 >
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"