[PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h

[PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h

[Joel Sherrill]

Hi

The recent addition of the sig2str block of code for definitions and 
prototypes resulted in the following one line program not compiling
for RTEMS targets:

#include <sys/signal.h>

Turned out that __STDINT_EXP used to conditionalize the definition
of SIG2STR_MAX isn't defined unless <sys/_intsup.h> is included.
I guess the test code got lucky.

It's a simple patch that needed more background and investigation
than code.

Is it safe to assume that including each POSIX and Standard C Library file
independently should compile? If so, I will file a ticket to at least at
those to the RTEMS compile only tests like the ones we have that check
a method can be used per the specific includes in the POSIX specification.

Sorry this slipped through.

--joel

Joel Sherrill (1):
  sys/signal.h: <sys/_intsup.h> is needed for __STDINT_EXP

 newlib/libc/include/sys/signal.h | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.24.4

[PATCH newlib 1/1] sys/signal.h: <sys/_intsup.h> is needed for __STDINT_EXP

[Joel Sherrill]

---
 newlib/libc/include/sys/signal.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/newlib/libc/include/sys/signal.h b/newlib/libc/include/sys/signal.h
index 255782eb6..e909abb52 100644
--- a/newlib/libc/include/sys/signal.h
+++ b/newlib/libc/include/sys/signal.h
@@ -10,6 +10,10 @@ extern "C" {
 #include <sys/cdefs.h>
 #include <sys/features.h>
 #include <sys/types.h>
+/* Using __MISC_VISIBLE until POSIX Issue 8 is officially released */
+#if __MISC_VISIBLE
+#include <sys/_intsup.h>
+#endif
 #include <sys/_sigset.h>
 #include <sys/_timespec.h>
 #include <stdint.h>
-- 
2.24.4

Re: Fw: [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h

[C Howland]

------------------------------
> *From:* Newlib <newlib-bounces+craig.howland=caci.com@sourceware.org> on
> behalf of Joel Sherrill <joel@rtems.org>
> *Sent:* Wednesday, August 25, 2021 3:12 PM
> *To:* newlib@sourceware.org <newlib@sourceware.org>
> *Subject:* [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h
>
>
>
> Hi
>
> The recent addition of the sig2str block of code for definitions and
> prototypes resulted in the following one line program not compiling
> for RTEMS targets:
>
> #include <sys/signal.h>
>
> Turned out that __STDINT_EXP used to conditionalize the definition
> of SIG2STR_MAX isn't defined unless <sys/_intsup.h> is included.
> I guess the test code got lucky.
>
> It's a simple patch that needed more background and investigation
> than code.
>
> Is it safe to assume that including each POSIX and Standard C Library file
> independently should compile? If so, I will file a ticket to at least at
> those to the RTEMS compile only tests like the ones we have that check
> a method can be used per the specific includes in the POSIX specification.
>
>      While I would think that #include on any "top level" include file
would have to compile on its own, sys/signal.h does not fall under that
umbrella.  That is, I don't think any valid use would call for
#include <sys/signal.h>
rather than
#include <signal.h>
So I think the real question is whether the latter works.
     By "top level" include I mean one that is intended to be directly
included by a user program, as opposed to indirectly included through
another include (as one would expect sys/signal.h to be nested to
<signal.h>).
     I'm not saying it is not a good idea that it can compile standalone,
but that I don't think it should be viewed as a requirement for every file
under include, especially most of them under sys.  There are some under sys
that are called to be directly included by user programs, specifically
sys/types.h, but the vast majority are not, intended to be nested from
other system includes.  So making test cases to specifically test for this
does not actually seem to be a good general idea for all include files, but
maybe only a subset.
     Aside from that general-approach thinking, something seems very
strange here.  sys/signal.h does include stdint.h and stdint.h does include
sys/_intsup.h.  So something about your test case failing seems like it has
to be wrong.  (I am not set up to compile with the current version, so I
can't easily check it.)
                 Craig

> Sorry this slipped through.
>
> --joel
>
> Joel Sherrill (1):
>   sys/signal.h: <sys/_intsup.h> is needed for __STDINT_EXP
>
>  newlib/libc/include/sys/signal.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> --
> 2.24.4
>
>
> ------------------------------
>

Re: Fw: [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h

[Joel Sherrill]

On Wed, Aug 25, 2021 at 2:49 PM C Howland <cc1964t@gmail.com> wrote:
>
> ------------------------------
> > *From:* Newlib <newlib-bounces+craig.howland=caci.com@sourceware.org> on
> > behalf of Joel Sherrill <joel@rtems.org>
> > *Sent:* Wednesday, August 25, 2021 3:12 PM
> > *To:* newlib@sourceware.org <newlib@sourceware.org>
> > *Subject:* [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h
> >
> >
> >
> > Hi
> >
> > The recent addition of the sig2str block of code for definitions and
> > prototypes resulted in the following one line program not compiling
> > for RTEMS targets:
> >
> > #include <sys/signal.h>
> >
> > Turned out that __STDINT_EXP used to conditionalize the definition
> > of SIG2STR_MAX isn't defined unless <sys/_intsup.h> is included.
> > I guess the test code got lucky.
> >
> > It's a simple patch that needed more background and investigation
> > than code.
> >
> > Is it safe to assume that including each POSIX and Standard C Library file
> > independently should compile? If so, I will file a ticket to at least at
> > those to the RTEMS compile only tests like the ones we have that check
> > a method can be used per the specific includes in the POSIX specification.
> >
> >      While I would think that #include on any "top level" include file
> would have to compile on its own, sys/signal.h does not fall under that
> umbrella.  That is, I don't think any valid use would call for
> #include <sys/signal.h>
> rather than
> #include <signal.h>
> So I think the real question is whether the latter works.

Yes. And after tearing into the code that didn't compile, I discovered
that #include <signal.h> does compile unless you add -ffreestanding.
When you add that, newlib's stdint.h does not get used. Instead gcc's
version does and that does not define __STDINT_EXP. This is gcc's
stdint.h:

====================================
#ifndef _GCC_WRAP_STDINT_H
#if __STDC_HOSTED__
# if defined __cplusplus && __cplusplus >= 201103L
#  undef __STDC_LIMIT_MACROS
#  define __STDC_LIMIT_MACROS
#  undef __STDC_CONSTANT_MACROS
#  define __STDC_CONSTANT_MACROS
# endif
# include_next <stdint.h>
#else
# include "stdint-gcc.h"
#endif
#define _GCC_WRAP_STDINT_H
#endif
====================================

We recently added this to sys/signal.h:

#if __STDINT_EXP(INT_MAX) > 0x7fff
#define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("4294967295") - 1)
#else
#define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("65535") - 1)
#endif

And -ffreestanding breaks that. I see two solutions:

(1) Add __STDINT_EXP to gcc's stdint-gcc.h
(2) Switch that #if to something else

I think something like this is an OK substitute works equivalently:

#if __SIZEOF_INT__ > 2

And toss my initial patch.

>      By "top level" include I mean one that is intended to be directly
> included by a user program, as opposed to indirectly included through
> another include (as one would expect sys/signal.h to be nested to
> <signal.h>).

Yes. I should have been clearer. That's what I was wondering if was
a good test case.

>      I'm not saying it is not a good idea that it can compile standalone,
> but that I don't think it should be viewed as a requirement for every file
> under include, especially most of them under sys.  There are some under sys
> that are called to be directly included by user programs, specifically
> sys/types.h, but the vast majority are not, intended to be nested from
> other system includes.  So making test cases to specifically test for this
> does not actually seem to be a good general idea for all include files, but
> maybe only a subset.

I was only thinking of ones called out in POSIX or C. Those often are listed
as single includes in the specifications of the methods.

--joel


>      Aside from that general-approach thinking, something seems very
> strange here.  sys/signal.h does include stdint.h and stdint.h does include
> sys/_intsup.h.  So something about your test case failing seems like it has
> to be wrong.  (I am not set up to compile with the current version, so I
> can't easily check it.)



>                  Craig
>
> > Sorry this slipped through.
> >
> > --joel
> >
> > Joel Sherrill (1):
> >   sys/signal.h: <sys/_intsup.h> is needed for __STDINT_EXP
> >
> >  newlib/libc/include/sys/signal.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > --
> > 2.24.4
> >
> >
> > ------------------------------
> >

Re: Fw: [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h

[Corinna Vinschen]

On Aug 26 18:28, Joel Sherrill wrote:
> We recently added this to sys/signal.h:
> 
> #if __STDINT_EXP(INT_MAX) > 0x7fff
> #define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("4294967295") - 1)
> #else
> #define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("65535") - 1)
> #endif
> 
> And -ffreestanding breaks that. I see two solutions:
> 
> (1) Add __STDINT_EXP to gcc's stdint-gcc.h
> (2) Switch that #if to something else
> 
> I think something like this is an OK substitute works equivalently:
> 
> #if __SIZEOF_INT__ > 2

or even __SIZEOF_INT__ >= 4 to be extra *extra* paranoid, but yeah,
sure, go ahead.  The commmit message should just briefly explain the
freestanding implementation issue.


Corinna

Re: Fw: [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h

[Joel Sherrill]

On Fri, Aug 27, 2021 at 5:16 AM Corinna Vinschen <vinschen@redhat.com> wrote:
>
> On Aug 26 18:28, Joel Sherrill wrote:
> > We recently added this to sys/signal.h:
> >
> > #if __STDINT_EXP(INT_MAX) > 0x7fff
> > #define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("4294967295") - 1)
> > #else
> > #define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("65535") - 1)
> > #endif
> >
> > And -ffreestanding breaks that. I see two solutions:
> >
> > (1) Add __STDINT_EXP to gcc's stdint-gcc.h
> > (2) Switch that #if to something else
> >
> > I think something like this is an OK substitute works equivalently:
> >
> > #if __SIZEOF_INT__ > 2
>
> or even __SIZEOF_INT__ >= 4 to be extra *extra* paranoid, but yeah,
> sure, go ahead.  The commmit message should just briefly explain the
> freestanding implementation issue.

Patch posted. Hopefully the commit message is clear enough.

This one definitely is in the land of subtle small things.

Thanks for the feedback and push to look for a deeper explanation.

--joel

>
>
> Corinna
>

Re: [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h

[C Howland]

>
>
> ------------------------------
> *From:* Newlib <newlib-bounces+craig.howland=caci.com@sourceware.org> on
> behalf of Joel Sherrill <joel@rtems.org>
> *Sent:* Friday, August 27, 2021 9:47 AM
> *To:* Newlib <newlib@sourceware.org>
> *Subject:* Re: Fw: [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h
>
>
> On Fri, Aug 27, 2021 at 5:16 AM Corinna Vinschen <vinschen@redhat.com>
> wrote:
> >
> > On Aug 26 18:28, Joel Sherrill wrote:
> > > We recently added this to sys/signal.h:
> > >
> > > #if __STDINT_EXP(INT_MAX) > 0x7fff
> > > #define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("4294967295") - 1)
> > > #else
> > > #define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("65535") - 1)
> > > #endif
> > >
> > > And -ffreestanding breaks that. I see two solutions:
> > >
> > > (1) Add __STDINT_EXP to gcc's stdint-gcc.h
> > > (2) Switch that #if to something else
> > >
> > > I think something like this is an OK substitute works equivalently:
> > >
> > > #if __SIZEOF_INT__ > 2
> >
> > or even __SIZEOF_INT__ >= 4 to be extra *extra* paranoid, but yeah,
> > sure, go ahead.  The commmit message should just briefly explain the
> > freestanding implementation issue.
>
> Patch posted. Hopefully the commit message is clear enough.
>
> This one definitely is in the land of subtle small things.
>
> Thanks for the feedback and push to look for a deeper explanation.
>
> --joel
>
> >
> > Corinna
>

 Just to close out the details on this more-esoteric subject:
-ffreestanding is improper for using Newlib, so we don't need to account
for it in general.  (A freestanding implementation may only use a subset of
the library, and stdint.h is not one of that list--which in C11 is
<float.h>, <iso646.h>, <limits.h>, <stdalign.h>, <stdarg.h>, <stdbool.h>,
<stddef.h>, <stdint.h>,and <stdnoreturn.h>.)  So the original is fine, but
so is the tweak.
Craig

Re: [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h

[Joel Sherrill]

On Fri, Aug 27, 2021, 10:12 AM C Howland <cc1964t@gmail.com> wrote:

> >
> >
> > ------------------------------
> > *From:* Newlib <newlib-bounces+craig.howland=caci.com@sourceware.org> on
> > behalf of Joel Sherrill <joel@rtems.org>
> > *Sent:* Friday, August 27, 2021 9:47 AM
> > *To:* Newlib <newlib@sourceware.org>
> > *Subject:* Re: Fw: [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h
> >
> >
> > On Fri, Aug 27, 2021 at 5:16 AM Corinna Vinschen <vinschen@redhat.com>
> > wrote:
> > >
> > > On Aug 26 18:28, Joel Sherrill wrote:
> > > > We recently added this to sys/signal.h:
> > > >
> > > > #if __STDINT_EXP(INT_MAX) > 0x7fff
> > > > #define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("4294967295") - 1)
> > > > #else
> > > > #define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("65535") - 1)
> > > > #endif
> > > >
> > > > And -ffreestanding breaks that. I see two solutions:
> > > >
> > > > (1) Add __STDINT_EXP to gcc's stdint-gcc.h
> > > > (2) Switch that #if to something else
> > > >
> > > > I think something like this is an OK substitute works equivalently:
> > > >
> > > > #if __SIZEOF_INT__ > 2
> > >
> > > or even __SIZEOF_INT__ >= 4 to be extra *extra* paranoid, but yeah,
> > > sure, go ahead.  The commmit message should just briefly explain the
> > > freestanding implementation issue.
> >
> > Patch posted. Hopefully the commit message is clear enough.
> >
> > This one definitely is in the land of subtle small things.
> >
> > Thanks for the feedback and push to look for a deeper explanation.
> >
> > --joel
> >
> > >
> > > Corinna
> >
>
>  Just to close out the details on this more-esoteric subject:
> -ffreestanding is improper for using Newlib, so we don't need to account
> for it in general.  (A freestanding implementation may only use a subset of
> the library, and stdint.h is not one of that list--which in C11 is
> <float.h>, <iso646.h>, <limits.h>, <stdalign.h>, <stdarg.h>, <stdbool.h>,
> <stddef.h>, <stdint.h>,and <stdnoreturn.h>.)


You have stdint.h in the allowed list. :)

So the original is fine, but
> so is the tweak.
>

Better to avoid any dependency on something newlib specific in favor of
something gcc provides.

FWIW the freestanding option is used to compile the FreeBSD kernel code
RTEMS uses for tcpip, USB, etc. We are currently using about 4000 files
from them between the kernel and commands. I don't know the origin of using
that compile option but do know the FreeBSD kernel is guilty of using names
like malloc with different signatures.

--joel

Craig
>

Re: [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h

[C Howland]

On Fri, 27 Aug 2021 at 12:17, Joel Sherrill <joel@rtems.org> wrote:

>
>
> On Fri, Aug 27, 2021, 10:12 AM C Howland <cc1964t@gmail.com> wrote:
>
>> >
>> >
>> > ------------------------------
>> > *From:* Newlib <newlib-bounces+craig.howland=caci.com@sourceware.org>
>> on
>> > behalf of Joel Sherrill <joel@rtems.org>
>> > *Sent:* Friday, August 27, 2021 9:47 AM
>> > *To:* Newlib <newlib@sourceware.org>
>> > *Subject:* Re: Fw: [PATCH newlib 0/1] sys/signal.h needs sys/_intsup.h
>> >
>> >
>> > On Fri, Aug 27, 2021 at 5:16 AM Corinna Vinschen <vinschen@redhat.com>
>> > wrote:
>> > >
>> > > On Aug 26 18:28, Joel Sherrill wrote:
>> > > > We recently added this to sys/signal.h:
>> > > >
>> > > > #if __STDINT_EXP(INT_MAX) > 0x7fff
>> > > > #define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("4294967295") - 1)
>> > > > #else
>> > > > #define SIG2STR_MAX (sizeof("RTMAX+") + sizeof("65535") - 1)
>> > > > #endif
>> > > >
>> > > > And -ffreestanding breaks that. I see two solutions:
>> > > >
>> > > > (1) Add __STDINT_EXP to gcc's stdint-gcc.h
>> > > > (2) Switch that #if to something else
>> > > >
>> > > > I think something like this is an OK substitute works equivalently:
>> > > >
>> > > > #if __SIZEOF_INT__ > 2
>> > >
>> > > or even __SIZEOF_INT__ >= 4 to be extra *extra* paranoid, but yeah,
>> > > sure, go ahead.  The commmit message should just briefly explain the
>> > > freestanding implementation issue.
>> >
>> > Patch posted. Hopefully the commit message is clear enough.
>> >
>> > This one definitely is in the land of subtle small things.
>> >
>> > Thanks for the feedback and push to look for a deeper explanation.
>> >
>> > --joel
>> >
>> > >
>> > > Corinna
>> >
>>
>>  Just to close out the details on this more-esoteric subject:
>> -ffreestanding is improper for using Newlib, so we don't need to account
>> for it in general.  (A freestanding implementation may only use a subset
>> of
>> the library, and stdint.h is not one of that list--which in C11 is
>> <float.h>, <iso646.h>, <limits.h>, <stdalign.h>, <stdarg.h>, <stdbool.h>,
>> <stddef.h>, <stdint.h>,and <stdnoreturn.h>.)
>
>
> You have stdint.h in the allowed list. :)
>
>
Oops, typo, sorry about that:  signal.h is not in the list.
OK, granted, a subset being used for that special purpose with freestanding
then does need to support that, so knowing that list I had included becomes
of more interest.
(The references for it are the C standard section 4, or several paragraphs
in the GCC manuals-search for freestanding.)

So the original is fine, but
>> so is the tweak.
>>
>
> Better to avoid any dependency on something newlib specific in favor of
> something gcc provides.
>
> FWIW the freestanding option is used to compile the FreeBSD kernel code
> RTEMS uses for tcpip, USB, etc. We are currently using about 4000 files
> from them between the kernel and commands. I don't know the origin of using
> that compile option but do know the FreeBSD kernel is guilty of using names
> like malloc with different signatures.
>
> --joel
>
> Craig
>>
>