Re: [PATCH 1/1] libc: Added implementations and prototypes for

On Mon, Jul 19, 2021 at 4:48 AM Corinna Vinschen <[email protected]> wrote:
>
> Hi Matt,
>
>
> thanks for this implementation.  The patch looks bascically fine.
> A few issues, though.
>
> First, it's missing the Makefile.am entries to build the new files,
> please add them.
>
> On Jul 17 12:10, Matt Joyce wrote:
> > Added implementations for sig2str() and str2sig() in libc/signal in order
> > to improve POSIX compliance. Added function prototypes to sys/signal.h.
> > ---
> >  newlib/libc/include/sys/signal.h |  12 ++
> >  newlib/libc/signal/sig2str.c     | 235 +++++++++++++++++++++++++++++++
> >  2 files changed, 247 insertions(+)
> >  create mode 100644 newlib/libc/signal/sig2str.c
> >
> > diff --git a/newlib/libc/include/sys/signal.h b/newlib/libc/include/sys/signal.h
> > index 45cc0366c..847dc59bd 100644
> > --- a/newlib/libc/include/sys/signal.h
> > +++ b/newlib/libc/include/sys/signal.h
> > @@ -238,6 +238,18 @@ int sigqueue (pid_t, int, const union sigval);
> >
> >  #endif /* __POSIX_VISIBLE >= 199309 */
> >
> > +#if __GNU_VISIBLE
> > +
> > +/* POSIX Issue 8 adds sig2str() and str2sig(). */
> > +
> > +/* This allows for the max length of the error message and longest integer. */
> > +#define SIG2STR_MAX sizeof("Unknown signal 4294967295 ")
>
> While looking through your patch I realized that this request of mine
> was nonsense.  The idea was that the buffers of length SIG2STR_MAX
> have to store descriptive signal texts, like "Floating point exception"
> or "Real-time signal 10", but that's not the case here anyway.
>
> If SIG2STR_MAX isn't already defined, gnulib defines SIG2STR_MAX
> basically as sizeof "SIGRTMAX" + sizeof (max numerical string of type
> int).  This results in different SIG2STR_MAX values depending of sizeof
> int being 2, 4, or 8.
>
> Given we strive for supporting smaller targets we might want to express
> this similary, just a bit simpler.  For one thing, do we ever want to
> support more than 4 billion RT signals?  I guess not.  My suggestion would
> be something like:
>
> #if __STDINT_EXP(INT_MAX) > 0x7fff
> #define SIG2STR_MAX (sizeof ("RTMAX+") + sizeof ("4294967295") - 1)
> #else
> #define SIG2STR_MAX (sizeof ("RTMAX+") + sizeof ("65535") - 1)
> #endif

Just checking here about the terminating NUL. The sizeof("RTMAX+") includes
a NUL so the -1 at the end of the expression subtracts the one from the
sizeof("NUM"). Is that right?

An off by one error here would be hard to catch if it slipped through.

>
> > +int
> > +sig2str(int signum, char *str)
> > +{
> > +  const sig_name_and_num *sptr;
> > +
> > +  /* If signum falls in the real time signals range, the Issue 8 standard
> > +  * gives the option of defining the saved str value as either "RTMIN+n" or
> > +  * "RTMAX-m".*/
> > +  if ((SIGRTMIN + 1) <= signum && signum <= (SIGRTMAX -1)) {
>                                                          ^^^
>                                                          space
>
> > +    sprintf(str, "RTMIN+%d", (signum-SIGRTMIN));
> > +    return 0;
> > +  }
>
> This is not entirely correct.  The POSIX draft requires the signal
> string to be returned for RT signals to be expressed as either "RTMIN+x"
> and "RTMAX-x", dependent on the signal number.  Please see
> https://www.opengroup.org/austin/docs/austin_1110.pdf, page 85, the two
> paragraphs starting at line 61678 (unfortunately copy/paste from this
> doc doesn't work nicely).

He implemented it this way initially but it looks like I optimistically misread
"the paragraph at line 61681 which gives an option on how to do the upper
half of the RT signals and thought/hoped it let the code have to produce
one format. But reading it again, there is a clear qualifier:

"If signum is between (SIGRTMIN+SIGRTMAX)/2 + 1 and SIGRTMAX−1
inclusive, ..."

>
> > +int
> > +str2sig(const char *restrict str, int *restrict pnum)
> > +{
> > +  int j = 0;
> > +  const sig_name_and_num *sptr;
> > +  char dest[SIG2STR_MAX];
> > +  int is_valid_decimal;
> > +  is_valid_decimal = atoi(str);
> > +
> > +  /* If str is a representation of a decimal value, save its integer value
> > +   * in pnum. */
> > +  if (1 <= is_valid_decimal && is_valid_decimal <= SIGRTMAX) {
> > +    *pnum = is_valid_decimal;
> > +    return 0;
> > +  }
> > +
> > +  /* If str is in RT signal range, get number of of RT signal, save it as an
> > +   * integer. The Issue 8 standard requires */
>                                               ^^^
>                                               Looks truncated?
>
> > +  if (strncmp(str, "RTMIN+", SPACES_TO_N) == 0) {
> > +    j = atoi(&str[SPACES_TO_N]);
>
> Oops... strncmp returns 0, but you didn't check if the next char is
> actually a non-NUL char, or even a digit.  Only digits are valid, so
> atoi should be replaced with strtoul with an extra check for endptr.
>
> > +    /* If number is valid, save it in pnum. */
> > +    if (1 <= j && j <= ((SIGRTMAX - SIGRTMIN)-1)) {
>                                                ^^^
>                                                spaces
>
> We have a special problem here.  i686 Cygwin only supports a single RT
> signal.  For historical reasons, don't ask.
>
> I. e., SIGRTMIN == SIGRTMAX.  This special case should be checked here,
> to make sure the code works with this very special, very non-POSIX
> behaviour.  I think the easiest way to handle that is to skip the
> entire "RTMIN+"/"RTMAX-" code if SIGRTMIN == SIGRTMAX.  There just is
> no such valid value in this case.

Do I interpret this as adding a conditionally compiled block to handle the
special case when SIGRTMIN == SIGRTMAX?

Is it possible for a target OS not to define any real-time signals? If so, it
might be necessary to enclose the encode RTMIN+/RTMAX- code like
this:

#ifdef SIGRTMIN
#if (SIGRTMIN == SIGRTMAX)
   Whatever i686 Cygwin want as output
#else
   RTMIN+/RTMAX- code
#endif
#endif

Things are always more complicated when you have to consider so
many target OSes. :)

>
> > +      *pnum = (SIGRTMIN + j);
> > +      return 0;
> > +    }
> > +    return -1;
> > +  }
> > +
> > +  /* If str is in RT signal range, get number of of RT signal, save it as an
> > +   * integer. */
> > +  if (strncmp(str, "RTMAX-", SPACES_TO_N) == 0) {
> > +    j = atoi(&str[SPACES_TO_N]);
>
> Ditto.
>
> > +    /* If number is valid, save it in pnum. */
> > +    if (1 <= j && j <= ((SIGRTMAX - SIGRTMIN)-1)) {
>                                                ^^^
>                                                spaces
>

--joel

>
> Thanks,
> Corinna
>