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

On Wed, Aug 25, 2021 at 2:49 PM C Howland <[email protected]> wrote:
>
> ------------------------------
> > *From:* Newlib <[email protected]> on
> > behalf of Joel Sherrill <[email protected]>
> > *Sent:* Wednesday, August 25, 2021 3:12 PM
> > *To:* [email protected] <[email protected]>
> > *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
> >
> >
> > ------------------------------
> >