[PATCH 0/1] Implementation of sig2str/str2sig

[PATCH 0/1] Implementation of sig2str/str2sig

[Matt Joyce]

Dear Newlib Maintainers and Community,

I am a GSoC student with the RTEMS project and I would like to submit
this patch for your review. It is an implementation of the sig2str and
str2sig methods as found in the upcoming Issue 8 POSIX standard. Thank
you very much for your time!

Sincerely,

Matt

Matt Joyce (1):
  libc: Added implementations and prototypes for sig2str/str2sig methods

 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

-- 
2.31.1

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

[Matt Joyce]

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 ")
+
+int sig2str(int, char *);
+int str2sig(const char *__restrict, int *__restrict);
+
+#endif /* __GNU_VISIBLE */
+
 #if defined(___AM29K__)
 /* These all need to be defined for ANSI C, but I don't think they are
    meaningful.  */
diff --git a/newlib/libc/signal/sig2str.c b/newlib/libc/signal/sig2str.c
new file mode 100644
index 000000000..04b4bb1be
--- /dev/null
+++ b/newlib/libc/signal/sig2str.c
@@ -0,0 +1,235 @@
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2021 Matthew Joyce
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+/* Defining _GNU_SOURCE to have access to SIG2STR_MAX in signal.h. */
+#define _GNU_SOURCE 
+#include <signal.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#define SPACES_TO_N 6 /* Allows indexing to RT Signal number in str2sig */
+#define NUM_OF_SIGS (sizeof(sig_array) / sizeof(sig_name_and_num))
+
+typedef struct sig_name_and_num {
+  const char *sig_name;
+  const int  sig_num; 
+} sig_name_and_num;
+
+static const sig_name_and_num sig_array[] = {
+    #ifdef SIGHUP
+      { "HUP", SIGHUP},
+    #endif
+    #ifdef SIGINT
+      { "INT", SIGINT },
+    #endif
+    #ifdef SIGQUIT
+      { "QUIT", SIGQUIT },
+    #endif
+    #ifdef SIGILL
+      { "ILL", SIGILL },
+    #endif
+    #ifdef SIGTRAP
+      { "TRAP", SIGTRAP },
+    #endif
+    #ifdef SIGABRT
+      { "ABRT", SIGABRT },
+    #endif
+    #ifdef SIGIOT
+      { "IOT", SIGIOT},
+    #endif
+    #ifdef SIGEMT
+      { "EMT", SIGEMT },
+    #endif
+    #ifdef SIGFPE
+      { "FPE", SIGFPE },
+    #endif
+    #ifdef SIGKILL
+      { "KILL", SIGKILL },
+    #endif
+    #ifdef SIGBUS
+      { "BUS", SIGBUS },
+    #endif
+    #ifdef SIGSEGV
+      { "SEGV", SIGSEGV },
+    #endif
+    #ifdef SIGSYS
+      { "SYS", SIGSYS },
+    #endif
+    #ifdef SIGPIPE
+      { "PIPE", SIGPIPE },
+    #endif
+    #ifdef SIGALRM
+      { "ALRM", SIGALRM },
+    #endif
+    #ifdef SIGTERM
+      { "TERM", SIGTERM },
+    #endif
+    #ifdef SIGURG
+      { "URG", SIGURG },
+    #endif
+    #ifdef SIGSTOP
+      { "STOP", SIGSTOP },
+    #endif
+    #ifdef SIGTSTP
+      { "TSTP", SIGTSTP },
+    #endif
+    #ifdef SIGCONT
+      { "CONT", SIGCONT },
+    #endif
+    #ifdef SIGCHLD
+      { "CHLD", SIGCHLD },
+    #endif
+    #ifdef SIGCLD
+      { "CLD", SIGCLD },
+    #endif
+    #ifdef SIGTTIN
+      { "TTIN", SIGTTIN },
+    #endif
+    #ifdef SIGTTOU
+      { "TTOU", SIGTTOU },
+    #endif
+    #ifdef SIGIO
+      { "IO", SIGIO },
+    #endif
+    #ifdef SIGPOLL
+      { "POLL", SIGPOLL },
+    #endif
+    #ifdef SIGWINCH
+      { "WINCH", SIGWINCH },
+    #endif
+    #ifdef SIGUSR1
+      { "USR1", SIGUSR1 },
+    #endif
+    #ifdef SIGUSR2
+      { "USR2", SIGUSR2 },
+    #endif
+    #ifdef SIGPWR
+      { "PWR", SIGPWR },
+    #endif
+    #ifdef SIGXCPU
+      { "XCPU", SIGXCPU },
+    #endif
+    #ifdef SIGXFSZ
+      { "XFSZ", SIGXFSZ },
+    #endif
+    #ifdef SIGVTALRM
+      { "VTALRM", SIGVTALRM },
+    #endif
+    #ifdef SIGPROF
+      { "PROF", SIGPROF },
+    #endif
+    #ifdef SIGLOST
+      { "LOST", SIGLOST },
+    #endif
+    /* The Issue 8 standard requires that SIGRTMIN and SIGRTMAX be included 
+     * as valid results to be saved from calls to sig2str/str2sig.  */
+    #ifdef SIGRTMIN
+      { "RTMIN", SIGRTMIN },
+    #endif
+    #ifdef SIGRTMAX
+      { "RTMAX", SIGRTMAX }
+    #endif
+}; 
+
+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)) {
+    sprintf(str, "RTMIN+%d", (signum-SIGRTMIN));
+    return 0; 
+  }
+  
+  /* Otherwise, search for signal matching signum in sig_array. If found,
+   * save its string value in str. */ 
+  for (sptr = sig_array; sptr < &sig_array[NUM_OF_SIGS]; sptr++) {
+    if (sptr->sig_num == signum) {
+      strcpy(str, sptr->sig_name);
+      return 0; 
+    } 
+  }   
+
+  /* If signum is not a recognized signal number, store this message in str. */
+  sprintf(str, "Unknown signal %d", signum); 
+  return -1; 
+}
+
+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 */
+  if (strncmp(str, "RTMIN+", SPACES_TO_N) == 0) {
+    j = atoi(&str[SPACES_TO_N]);
+
+    /* If number is valid, save it in pnum. */
+    if (1 <= j && j <= ((SIGRTMAX - SIGRTMIN)-1)) {
+      *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]);
+
+    /* If number is valid, save it in pnum. */
+    if (1 <= j && j <= ((SIGRTMAX - SIGRTMIN)-1)) {
+      *pnum = (SIGRTMAX - j);
+      return 0;  
+    }
+    return -1; 
+  }
+  
+  /*If str is a valid signal name, save its corresponding number in pnum. */
+  for (sptr = sig_array; sptr < &sig_array[NUM_OF_SIGS]; sptr++) {
+    if (strcmp(sptr->sig_name, str) == 0) {
+      *pnum = sptr->sig_num;
+      return 0; 
+    }    
+  }
+  return -1; 
+}
-- 
2.31.1

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

[Corinna Vinschen]

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

> +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).

> +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.

> +      *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


Thanks,
Corinna

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

[Joel Sherrill]

On Mon, Jul 19, 2021 at 4:48 AM Corinna Vinschen <vinschen@redhat.com> 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
>

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

[Corinna Vinschen]

On Jul 19 08:19, Joel Sherrill wrote:
> On Mon, Jul 19, 2021 at 4:48 AM Corinna Vinschen <vinschen@redhat.com> wrote:
> > #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.

Writing a simple test program and attaching it to the patchset would be a
good idea anyway.

> > 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, ..."

But then again...

...on rereading the text it appears the intention was to allow both ways
of expressing the signal, whichever is preferred by the implementation.

Checking gnulib, they use RTMAX-x for the upper half of the range.

The Oracle website expresses it in terms of 8 RT sigs and claims

  For access to the signals in the range SIGRTMIN to SIGRTMAX, the
  first four signals match the strings "RTMIN", "RTMIN+1", "RTMIN+2",
  and "RTMIN+3" and the last four match the strings "RTMAX-3",
  "RTMAX-2", "RTMAX-1", and "RTMAX".

So it looks like there's some kind of consensus to do it that way.
I guess we should follow suit.

> > 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?

Just skip the RT sigs handling code block.

> Is it possible for a target OS not to define any real-time signals?

For bare-metal and embedded targets it's probably a good idea to assume
just that.  Per POSIX, 4 RT sigs are required, but that's often no
concern for small targets, and for i686 Cygwin we simply screwed up.


Corinna

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

[Matthew Joyce]

Hi Corinna,

Thank you very much for the feedback! I'll make these changes as
suggested after work today.

Sincerely,

Matt


On Mon, Jul 19, 2021 at 4:31 PM Corinna Vinschen <vinschen@redhat.com> wrote:
>
> On Jul 19 08:19, Joel Sherrill wrote:
> > On Mon, Jul 19, 2021 at 4:48 AM Corinna Vinschen <vinschen@redhat.com> wrote:
> > > #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.
>
> Writing a simple test program and attaching it to the patchset would be a
> good idea anyway.
>
> > > 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, ..."
>
> But then again...
>
> ...on rereading the text it appears the intention was to allow both ways
> of expressing the signal, whichever is preferred by the implementation.
>
> Checking gnulib, they use RTMAX-x for the upper half of the range.
>
> The Oracle website expresses it in terms of 8 RT sigs and claims
>
>   For access to the signals in the range SIGRTMIN to SIGRTMAX, the
>   first four signals match the strings "RTMIN", "RTMIN+1", "RTMIN+2",
>   and "RTMIN+3" and the last four match the strings "RTMAX-3",
>   "RTMAX-2", "RTMAX-1", and "RTMAX".
>
> So it looks like there's some kind of consensus to do it that way.
> I guess we should follow suit.
>
> > > 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?
>
> Just skip the RT sigs handling code block.
>
> > Is it possible for a target OS not to define any real-time signals?
>
> For bare-metal and embedded targets it's probably a good idea to assume
> just that.  Per POSIX, 4 RT sigs are required, but that's often no
> concern for small targets, and for i686 Cygwin we simply screwed up.
>
>
> Corinna
>

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

[Matthew Joyce]

Hi Corinna,

I've made the changes you recommended but I'm running into a problem
with the definition of SIG2STR_MAX. You suggested:

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

I've tried it both in newlib and natively but I get the error:
" missing binary operator before token '(' "
(In the first line of the block)

At first I thought it might be a problem with using 'sizeof' in the
definition. But I get the same error even if I hard-code the define
values after #if  __STDINT_EXP....

Could you please advise on what I might be doing wrong? Completely
hard coding the value as 16 (the length of the larger string) works
without error.

Thank you very much for your time!

Sincerely,

Matt

On Mon, Jul 19, 2021 at 4:31 PM Corinna Vinschen <vinschen@redhat.com> wrote:
>
> On Jul 19 08:19, Joel Sherrill wrote:
> > On Mon, Jul 19, 2021 at 4:48 AM Corinna Vinschen <vinschen@redhat.com> wrote:
> > > #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.
>
> Writing a simple test program and attaching it to the patchset would be a
> good idea anyway.
>
> > > 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, ..."
>
> But then again...
>
> ...on rereading the text it appears the intention was to allow both ways
> of expressing the signal, whichever is preferred by the implementation.
>
> Checking gnulib, they use RTMAX-x for the upper half of the range.
>
> The Oracle website expresses it in terms of 8 RT sigs and claims
>
>   For access to the signals in the range SIGRTMIN to SIGRTMAX, the
>   first four signals match the strings "RTMIN", "RTMIN+1", "RTMIN+2",
>   and "RTMIN+3" and the last four match the strings "RTMAX-3",
>   "RTMAX-2", "RTMAX-1", and "RTMAX".
>
> So it looks like there's some kind of consensus to do it that way.
> I guess we should follow suit.
>
> > > 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?
>
> Just skip the RT sigs handling code block.
>
> > Is it possible for a target OS not to define any real-time signals?
>
> For bare-metal and embedded targets it's probably a good idea to assume
> just that.  Per POSIX, 4 RT sigs are required, but that's often no
> concern for small targets, and for i686 Cygwin we simply screwed up.
>
>
> Corinna
>

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

[Corinna Vinschen]

Hi Matt,

On Jul 22 07:14, Matthew Joyce wrote:
> Hi Corinna,
> 
> I've made the changes you recommended but I'm running into a problem
> with the definition of SIG2STR_MAX. You suggested:
> 
> #if __STDINT_EXP(INT_MAX) > 0x7fff
> #define SIG2STR_MAX (sizeof ("RTMAX+") + sizeof ("4294967295") - 1)
> #else #define SIG2STR_MAX (sizeof ("RTMAX+") + sizeof ("65535") - 1)

These are actually two lines, right?  It's just a copy/paste glitch,
I guess.

> #endif
> 
> I've tried it both in newlib and natively but I get the error:
> " missing binary operator before token '(' "
> (In the first line of the block)

I hacked a STC:

----------------------------------
#include <stdio.h>
#include <string.h>
#include <stdint.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

void
foo (int sig, char *out)
{
  char buf[SIG2STR_MAX];

  if (sig > SIG2STR_MAX)
    return;
  snprintf (buf, SIG2STR_MAX, "FOO+%d", sig);
  memcpy (out, buf, SIG2STR_MAX);
}
----------------------------------

$ gcc -g -O2 -c -Wall foo.c

No error or warning is generated, so I'm puzzled about your problem.

Would you mind to paste the affected code snippet and the full gcc error
output?


Corinna

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

[Matthew Joyce]

Hi Corinna,

Thanks again for the explanation! I think I had just failed to include stdint.h.

Now that it's working, I'd like to clarify: If SIG2STR_MAX is now 17,
this will impact the message "Unknown signal <signum>" in case the
signum isn't a valid, recognized signal number.

Is it better to change the error message (by taking out the <signum>
part) or make SIG2STR_MAX larger?

Thank you!

Sincerely,

Matt

On Thu, Jul 22, 2021 at 9:55 AM Corinna Vinschen <vinschen@redhat.com> wrote:
>
> Hi Matt,
>
> On Jul 22 07:14, Matthew Joyce wrote:
> > Hi Corinna,
> >
> > I've made the changes you recommended but I'm running into a problem
> > with the definition of SIG2STR_MAX. You suggested:
> >
> > #if __STDINT_EXP(INT_MAX) > 0x7fff
> > #define SIG2STR_MAX (sizeof ("RTMAX+") + sizeof ("4294967295") - 1)
> > #else #define SIG2STR_MAX (sizeof ("RTMAX+") + sizeof ("65535") - 1)
>
> These are actually two lines, right?  It's just a copy/paste glitch,
> I guess.
>
> > #endif
> >
> > I've tried it both in newlib and natively but I get the error:
> > " missing binary operator before token '(' "
> > (In the first line of the block)
>
> I hacked a STC:
>
> ----------------------------------
> #include <stdio.h>
> #include <string.h>
> #include <stdint.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
>
> void
> foo (int sig, char *out)
> {
>   char buf[SIG2STR_MAX];
>
>   if (sig > SIG2STR_MAX)
>     return;
>   snprintf (buf, SIG2STR_MAX, "FOO+%d", sig);
>   memcpy (out, buf, SIG2STR_MAX);
> }
> ----------------------------------
>
> $ gcc -g -O2 -c -Wall foo.c
>
> No error or warning is generated, so I'm puzzled about your problem.
>
> Would you mind to paste the affected code snippet and the full gcc error
> output?
>
>
> Corinna
>

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

[Corinna Vinschen]

On Jul 23 07:44, Matthew Joyce wrote:
> Hi Corinna,
> 
> Thanks again for the explanation! I think I had just failed to include stdint.h.
> 
> Now that it's working, I'd like to clarify: If SIG2STR_MAX is now 17,
> this will impact the message "Unknown signal <signum>" in case the
> signum isn't a valid, recognized signal number.
> 
> Is it better to change the error message (by taking out the <signum>
> part) or make SIG2STR_MAX larger?

Actually, don't write anything into the string in case the signal
doesn't exist.  If the signal is not valid, sig2str is supposed to
returns -1.  It's not supposed to write anything into the buffer in that
case.


Corinna