From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f172.google.com (mail-lj1-f172.google.com [209.85.208.172]) by sourceware.org (Postfix) with ESMTPS id F23D2394FC1C for ; Mon, 19 Jul 2021 13:19:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F23D2394FC1C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rtems.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lj1-f172.google.com with SMTP id t20so10499802ljd.2 for ; Mon, 19 Jul 2021 06:19:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:reply-to :from:date:message-id:subject:to:content-transfer-encoding; bh=GhsFfThKkUMX4dP3eSB/NCpy3+bJIJzzvvbPfGtYH4A=; b=fZQsn9q8WMPBQUZZg5RoTdnOdzam8qYPt4P5hsZgZLc42KWr540o3GPTCAh6PM3WSL V5Juc9GB8Kuyoz5ocMvCHa1MkO8d+e58HWjqPtdP87ALM82MDW+WsVa4lgvD/Hmcayw6 BBazuz0nThS3ujTjm8LWvy7AaYxwT9CF+znmkCiyKCnI2ni8FcegxjRaJGpJJanOobVD L1j4N1ghQpxepMEasKaSeyWZwLA1q98KIUdwntuaybo0A6Ca4R0c8PN2OPsRwnRqBCIN Wq5YNHj8N9au7ohM9RlhYsV5tDcjgn8eNDps/f23ly1FjWakAmOyXEAyfEOhOQ5EZeqZ Lf1A== X-Gm-Message-State: AOAM532grJyiTT6LoUQDpNReOnWH7QSudE3LBZmtfNu1uvSwt1fn+K/O A9USKNZIcTAnua4C+Qv3F9i/bIoTpmMB4w== X-Google-Smtp-Source: ABdhPJxjJjdX9qhaCUC2I2GDnfjD/gPfrfegpJwGIaMvk9AEzNlCIZCCEr5K9maJvwn/L2QdjIjUkQ== X-Received: by 2002:a2e:2a42:: with SMTP id q63mr6969699ljq.460.1626700754533; Mon, 19 Jul 2021 06:19:14 -0700 (PDT) Received: from mail-lf1-f42.google.com (mail-lf1-f42.google.com. [209.85.167.42]) by smtp.gmail.com with ESMTPSA id r15sm2105661ljk.92.2021.07.19.06.19.13 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 19 Jul 2021 06:19:13 -0700 (PDT) Received: by mail-lf1-f42.google.com with SMTP id x25so30076303lfu.13 for ; Mon, 19 Jul 2021 06:19:13 -0700 (PDT) X-Received: by 2002:a05:6512:374f:: with SMTP id a15mr17721354lfs.208.1626700753252; Mon, 19 Jul 2021 06:19:13 -0700 (PDT) MIME-Version: 1.0 References: <20210717101038.3283-1-mfjoyce2004@gmail.com> <20210717101038.3283-2-mfjoyce2004@gmail.com> In-Reply-To: Reply-To: joel@rtems.org From: Joel Sherrill Date: Mon, 19 Jul 2021 08:19:01 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 1/1] libc: Added implementations and prototypes for To: Newlib , Matt Joyce Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3037.8 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: newlib@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Newlib mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Jul 2021 13:19:18 -0000 On Mon, Jul 19, 2021 at 4:48 AM Corinna Vinschen wrot= e: > > 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 ord= er > > 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 >=3D 199309 */ > > > > +#if __GNU_VISIBLE > > + > > +/* POSIX Issue 8 adds sig2str() and str2sig(). */ > > + > > +/* This allows for the max length of the error message and longest int= eger. */ > > +#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 woul= d > 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 stand= ard > > + * gives the option of defining the saved str value as either "RTMIN+= n" or > > + * "RTMAX-m".*/ > > + if ((SIGRTMIN + 1) <=3D signum && signum <=3D (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 mis= read "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=E2=88=921 inclusive, ..." > > > +int > > +str2sig(const char *restrict str, int *restrict pnum) > > +{ > > + int j =3D 0; > > + const sig_name_and_num *sptr; > > + char dest[SIG2STR_MAX]; > > + int is_valid_decimal; > > + is_valid_decimal =3D atoi(str); > > + > > + /* If str is a representation of a decimal value, save its integer v= alue > > + * in pnum. */ > > + if (1 <=3D is_valid_decimal && is_valid_decimal <=3D SIGRTMAX) { > > + *pnum =3D 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) =3D=3D 0) { > > + j =3D 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 <=3D j && j <=3D ((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 =3D=3D SIGRTMAX. This special case should be checked her= e, > 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 =3D=3D 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 =3D=3D 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 =3D=3D 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 =3D (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) =3D=3D 0) { > > + j =3D atoi(&str[SPACES_TO_N]); > > Ditto. > > > + /* If number is valid, save it in pnum. */ > > + if (1 <=3D j && j <=3D ((SIGRTMAX - SIGRTMIN)-1)) { > ^^^ > spaces > --joel > > Thanks, > Corinna >