Message ID | 20170524155000.GA45792@aloka.lostca.se |
---|---|
State | New, archived |
Headers |
Received: (qmail 129418 invoked by alias); 24 May 2017 15:50:02 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 129398 invoked by uid 89); 24 May 2017 15:50:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=UD:se, H*F:D*se X-HELO: aloka.lostca.se Date: Wed, 24 May 2017 15:50:00 +0000 From: Arjun Shankar <arjun.is@lostca.se> To: libc-alpha@sourceware.org Subject: [PATCH] Use an alias to `ptsname_r' in login/tst-ptsname.c Message-ID: <20170524155000.GA45792@aloka.lostca.se> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.7.1 (2016-10-04) |
Commit Message
Arjun Shankar
May 24, 2017, 3:50 p.m. UTC
When compiled with `-O3', inlining causes GCC to realize that one of the tests calls `ptsname_r' with a NULL buffer and error out when compiling with -Wnonnull. Declare `ptsname_r_alias' and alias it to `ptsname_r' to avoid this. ChangeLog: 2017-05-24 Arjun Shankar <arjun.is@lostca.se> * login/tst-ptsname.c (ptsname_r_alias): New declaration. (do_single_test): Use ptsname_r_alias instead of ptsname_r. --- login/tst-ptsname.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
Comments
On Wed, 24 May 2017, Arjun Shankar wrote: > When compiled with `-O3', inlining causes GCC to realize that one of the > tests calls `ptsname_r' with a NULL buffer and error out when compiling > with -Wnonnull. > > Declare `ptsname_r_alias' and alias it to `ptsname_r' to avoid this. Normally in such cases we'd use the DIAG_* macros from libc-diag.h, with appropriate comments, to disable the specific warning for the code in question.
On 05/24/2017 06:18 PM, Joseph Myers wrote: > On Wed, 24 May 2017, Arjun Shankar wrote: > >> When compiled with `-O3', inlining causes GCC to realize that one of the >> tests calls `ptsname_r' with a NULL buffer and error out when compiling >> with -Wnonnull. >> >> Declare `ptsname_r_alias' and alias it to `ptsname_r' to avoid this. > > Normally in such cases we'd use the DIAG_* macros from libc-diag.h, with > appropriate comments, to disable the specific warning for the code in > question. I think passing NULL to a nonnull parameter is undefined, warning or not. The GCC documentation is pretty clear about that. (I have some doubts about the validity of the NULL comparison inside the implementation of ptsname_r, too.) Alternatively, we could withdraw the test as invalid, and update the manual page. Thanks, Florian
On Wed, May 24, 2017 at 07:42:37PM +0200, Florian Weimer wrote: > I think passing NULL to a nonnull parameter is undefined, warning or > not. The GCC documentation is pretty clear about that. (I have some > doubts about the validity of the NULL comparison inside the > implementation of ptsname_r, too.) > > Alternatively, we could withdraw the test as invalid, and update the > manual page. Removing the sub-test that tests for EINVAL on NULL sounds like a good idea to me as well. About the man-page I just looked at `pthread_create' for comparison: * the `pthread_t *' argument is declared `nonnull' in pthread.h * there is no code in `pthread_create' to check if the argument is NULL * nothing in the man page to indicate what happens when NULL is passed * compiling `pthread_create (NULL, NULL, NULL, NULL)' without `-Wall -Werror' produces a binary that SIGSEGVs * compiling with `-Wall -Werror' errors out the compilation Given this, maybe in the case of `ptsname_r' it is enough to just drop the specific test that tests with NULL, and leave the man-page unchanged? While we are at it, we could also drop the test for NULL in `ptsname_r'. How does that sound for a PATCH v2?
On 05/25/2017 06:05 PM, Arjun Shankar wrote: > Given this, maybe in the case of `ptsname_r' it is enough to just drop > the specific test that tests with NULL, and leave the man-page > unchanged? That's fine as well. I don't know if others want to preserve the EINVAL handling. > While we are at it, we could also drop the test for NULL in > `ptsname_r'. How does that sound for a PATCH v2? That would require updating the manpage, wouldn't it? Thanks, Florian
> > Given this, maybe in the case of `ptsname_r' it is enough to just drop > > the specific test that tests with NULL, and leave the man-page > > unchanged? > > That's fine as well. I don't know if others want to preserve the EINVAL > handling. > > > While we are at it, we could also drop the test for NULL in > > `ptsname_r'. How does that sound for a PATCH v2? > > That would require updating the manpage, wouldn't it? My argument behind both of these suggestions was that `pthread_create' doesn't care about both of these things (i.e. it doesn't check that the `pthread_t' pointer is not NULL; it just writes to it. And it doesn't explicitly mention in the man-page that the pointer is required to be valid and of sufficient size) - and therefore `ptsname_r' doesn't require these things either. Am I reasoning about this correctly?
On Thu, May 25, 2017 at 11:37 PM, Arjun Shankar <arjun.is@lostca.se> wrote: > My argument behind both of these suggestions was that `pthread_create' > doesn't care about both of these things (i.e. it doesn't check that the > `pthread_t' pointer is not NULL; it just writes to it. And it doesn't > explicitly mention in the man-page that the pointer is required to be > valid and of sufficient size) - and therefore `ptsname_r' doesn't > require these things either. > > Am I reasoning about this correctly? What you are saying is not wrong, but the manual page in its current state documents the EINVAL return value. That would have to change. Thanks, Florian
diff --git a/login/tst-ptsname.c b/login/tst-ptsname.c index be8744d..dbc8d19 100644 --- a/login/tst-ptsname.c +++ b/login/tst-ptsname.c @@ -27,11 +27,16 @@ #define DEV_TTY "/dev/tty" #define PTSNAME_EINVAL "./ptsname-einval" +/* When compiled with `-O3', inlining causes GCC to realize that one of the + tests calls `ptsname_r' with a NULL buffer and error out when compiling + with -Wnonnull. To avoid this, we use an alias instead. */ +int ptsname_r_alias (int, char *, size_t) __asm__ ("ptsname_r"); + static int do_single_test (int fd, char *buf, size_t buflen, int expected_err) { - int ret = ptsname_r (fd, buf, buflen); + int ret = ptsname_r_alias (fd, buf, buflen); int err = errno; if (expected_err == 0)