Message ID | CAFOnWknprKQpWy3TU1x7cZZ7n+=nJyw7nhHme9KtLxiv8GasbQ@mail.gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 27569 invoked by alias); 29 Jun 2016 12:45:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 27523 invoked by uid 89); 29 Jun 2016 12:45:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=palves X-HELO: mail-wm0-f44.google.com Received: from mail-wm0-f44.google.com (HELO mail-wm0-f44.google.com) (74.125.82.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 29 Jun 2016 12:45:20 +0000 Received: by mail-wm0-f44.google.com with SMTP id v199so179381137wmv.0 for <gdb-patches@sourceware.org>; Wed, 29 Jun 2016 05:45:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=RxxN+iXndbZkPNvEMuSH4E3bglG+uTvVM4V/xPiaDRc=; b=fhaNpUbwxrHz9igHxPS1lygyiTxdfPwhOd7VCoezgrqlEFqCrKTWEeLaHSptd+lvq+ K8VAjWy+cDS0fkRRmWp/Kb4Th/mYr0VwLarUj1+Ag6+go0wDK9xlF6/zndOgt8jpUNkh AOkK5BP/esQVssQz82CjzKkEjulpf9v8C69SYJM1Fg1u1JA2gcC4giME653Bgj/JEy/d //R6SzzjXdblIJL4FP8WxrqpteGhgcaytzYHp2jZOnT044t1dvHxCs80tTY4uZjBITwy MU4gPJuFsBU8B9TuRsbq4xV/ioNchzgz9FfO+Dr7HPDNxCn+PtfKnQ4wWL+/Tvs95c9q jhBg== X-Gm-Message-State: ALyK8tIr1wJJU/gXsYO9T4TbpZki8waVTK69csVqjJm28TN23n/7H1aC2GXISrOuUVrzOk3VwJ7qGyTnMREGnfSA X-Received: by 10.194.200.164 with SMTP id jt4mr8482301wjc.18.1467204317041; Wed, 29 Jun 2016 05:45:17 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.36.215 with HTTP; Wed, 29 Jun 2016 05:45:16 -0700 (PDT) From: Manish Goregaokar <manish@mozilla.com> Date: Wed, 29 Jun 2016 18:15:16 +0530 Message-ID: <CAFOnWknprKQpWy3TU1x7cZZ7n+=nJyw7nhHme9KtLxiv8GasbQ@mail.gmail.com> Subject: [PATCH] Initialize strtok_r's saveptr to NULL To: gdb-patches@sourceware.org, Pedro Alves <palves@redhat.com> Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes |
Commit Message
Manish Goregaokar
June 29, 2016, 12:45 p.m. UTC
Accidentally sent this directly to palves instead of to the list. In the review of the previous patch it was mentioned that we shouldn't need to initialize this, but it seems like elsewhere in the codebase we initialize the saveptr of strtok_r to NULL too. ---- This fixes a build warning. 2016-06-29 Manish Goregaokar <manish@mozilla.com> gdb/ChangeLog: * rust-lang.c (rust_get_disr_info): Initialize saveptr to NULL --- gdb/rust-lang.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) LONGEST value;
Comments
On 06/29/2016 01:45 PM, Manish Goregaokar wrote: > Accidentally sent this directly to palves instead of to the list. > > > In the review of the previous patch it was mentioned that we shouldn't > need to initialize this, > but it seems like elsewhere in the codebase we initialize the saveptr > of strtok_r to NULL too. No, strtok_r is only used in a handful of places, and not all initialize it. E.g., linux-tdep.c: if (data != NULL) { struct cleanup *cleanup = make_cleanup (xfree, data); char *line, *t; line = strtok_r (data, "\n", &t); while (line != NULL) { > > > ---- > > This fixes a build warning. The buildbot that failed is the one that builds gdb in C mode I can reproduce this here too, with --enable-build-with-cxx=no. gcc 7 shows a bit more detail, though it's still not very clear: /home/pedro/gdb/mygit/src/gdb/rust-lang.c: In function ‘rust_get_disr_info.isra.5’: /home/pedro/gdb/mygit/src/gdb/rust-lang.c:173:15: error: ‘__s’ may be used uninitialized in this function [-Werror=maybe-uninitialized] ret.name = concat (TYPE_NAME (type), "::", token, (char *) NULL); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors The mention of "__s" is a hint -- I think it comes from an expansion of glibc's inline strtok_r, in /usr/include/bits/string2.h: __STRING_INLINE char *__strtok_r_1c (char *__s, char __sep, char **__nextp); __STRING_INLINE char * __strtok_r_1c (char *__s, char __sep, char **__nextp) { char *__result; if (__s == NULL) __s = *__nextp; ... So if on the first call to strtok_r, "tail" is NULL, __s here is NULL and "token" becomes the uninitialized "savedptr". So the problem is that gcc doesn't understand that in: name = xstrdup (TYPE_FIELD_NAME (type, 0)); cleanup = make_cleanup (xfree, name); tail = name + strlen (RUST_ENUM_PREFIX); /* The location of the value that doubles as a discriminant is stored in the name of the field, as RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname> where the fieldnos are the indices of the fields that should be traversed in order to find the field (which may be several fields deep) and the variantname is the name of the variant of the case when the field is zero. */ for (token = strtok_r (tail, "$", &saveptr); "tail" can _never_ be NULL. Adding: gdb_assert (tail != NULL); just before the strtok_r makes the warning go away, which proves that that's indeed the problem. If "tail" could ever be NULL here, then the warning would be revealing a bug -- exactly the sort of bug that I was hoping a warning would catch. But it looks like it's revealing a gcc bug instead... xstrdup is marked with __attribute__ ((__returns_nonnull__)), so gcc knows "name" is never NULL. Hacking the "tail" initialization like this: - tail = name + strlen (RUST_ENUM_PREFIX); + tail = name; Makes the warning go away. Changing the strlen to a sizeof, to make sure gcc understands this is a constant offset does not help. Even writing it as tail = &name[sizeof (RUST_ENUM_PREFIX) - 1]; does not help either. It looks like a gcc bug to me that gcc doesn't propagate the non-nullness to "tail" (while g++ does). Oh well... It'd be good to include a bit more detail in the commit log, about at least which warning triggered. E.g., something like this: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Building gdb with --enable-build-with-cxx=no trips on a warning: ../../binutils-gdb/gdb/rust-lang.c:173:15: error: saveptr may be used uninitialized in this function [-Werror=maybe-uninitialized] ret.name = concat (TYPE_NAME (type), "::", token, (char *) NULL); The problem is that gcc doesn't understand that "tail" can never be NULL in the call to strtok_r: name = xstrdup (TYPE_FIELD_NAME (type, 0)); cleanup = make_cleanup (xfree, name); tail = name + strlen (RUST_ENUM_PREFIX); ... for (token = strtok_r (tail, "$", &saveptr); Fix this by always initializing saveptr. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ OK with that change, and ... > 2016-06-29 Manish Goregaokar <manish@mozilla.com> > > gdb/ChangeLog: > * rust-lang.c (rust_get_disr_info): Initialize saveptr to NULL ... add missing period. Thanks, Pedro Alves
Fixed and pushed. Is this something we can fix in gcc? Pointer overflow is UB IIRC so <nonnullptr>+<positive thing> should never be null. Not sure if the compiler knows that this is positive though. -Manish On Wed, Jun 29, 2016 at 7:55 PM, Pedro Alves <palves@redhat.com> wrote: > On 06/29/2016 01:45 PM, Manish Goregaokar wrote: >> Accidentally sent this directly to palves instead of to the list. >> >> >> In the review of the previous patch it was mentioned that we shouldn't >> need to initialize this, >> but it seems like elsewhere in the codebase we initialize the saveptr >> of strtok_r to NULL too. > > No, strtok_r is only used in a handful of places, and not all > initialize it. E.g., linux-tdep.c: > > if (data != NULL) > { > struct cleanup *cleanup = make_cleanup (xfree, data); > char *line, *t; > > line = strtok_r (data, "\n", &t); > while (line != NULL) > { > >> >> >> ---- >> >> This fixes a build warning. > > > The buildbot that failed is the one that builds gdb in C mode > I can reproduce this here too, with --enable-build-with-cxx=no. > > gcc 7 shows a bit more detail, though it's still not very clear: > > /home/pedro/gdb/mygit/src/gdb/rust-lang.c: In function ‘rust_get_disr_info.isra.5’: > /home/pedro/gdb/mygit/src/gdb/rust-lang.c:173:15: error: ‘__s’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > ret.name = concat (TYPE_NAME (type), "::", token, (char *) NULL); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: some warnings being treated as errors > > > The mention of "__s" is a hint -- I think it comes from an > expansion of glibc's inline strtok_r, in /usr/include/bits/string2.h: > > __STRING_INLINE char *__strtok_r_1c (char *__s, char __sep, char **__nextp); > __STRING_INLINE char * > __strtok_r_1c (char *__s, char __sep, char **__nextp) > { > char *__result; > if (__s == NULL) > __s = *__nextp; > ... > > > So if on the first call to strtok_r, "tail" is NULL, __s here is NULL > and "token" becomes the uninitialized "savedptr". > > So the problem is that gcc doesn't understand that in: > > name = xstrdup (TYPE_FIELD_NAME (type, 0)); > cleanup = make_cleanup (xfree, name); > tail = name + strlen (RUST_ENUM_PREFIX); > > /* The location of the value that doubles as a discriminant is > stored in the name of the field, as > RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname> > where the fieldnos are the indices of the fields that should be > traversed in order to find the field (which may be several fields deep) > and the variantname is the name of the variant of the case when the > field is zero. */ > for (token = strtok_r (tail, "$", &saveptr); > > "tail" can _never_ be NULL. Adding: > > gdb_assert (tail != NULL); > > just before the strtok_r makes the warning go away, which proves > that that's indeed the problem. > > If "tail" could ever be NULL here, then the warning would be > revealing a bug -- exactly the sort of bug that I was hoping a warning > would catch. But it looks like it's revealing a gcc bug instead... > > xstrdup is marked with __attribute__ ((__returns_nonnull__)), > so gcc knows "name" is never NULL. Hacking the "tail" initialization > like this: > > - tail = name + strlen (RUST_ENUM_PREFIX); > + tail = name; > > Makes the warning go away. > > Changing the strlen to a sizeof, to make sure gcc understands this is > a constant offset does not help. Even writing it as > > tail = &name[sizeof (RUST_ENUM_PREFIX) - 1]; > > does not help either. It looks like a gcc bug to me that gcc > doesn't propagate the non-nullness to "tail" (while g++ does). > > Oh well... > > It'd be good to include a bit more detail in the commit log, about > at least which warning triggered. E.g., something like this: > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > Building gdb with --enable-build-with-cxx=no trips on a warning: > > ../../binutils-gdb/gdb/rust-lang.c:173:15: error: saveptr may be used > uninitialized in this function [-Werror=maybe-uninitialized] > ret.name = concat (TYPE_NAME (type), "::", token, (char *) NULL); > > The problem is that gcc doesn't understand that "tail" can never be > NULL in the call to strtok_r: > > name = xstrdup (TYPE_FIELD_NAME (type, 0)); > cleanup = make_cleanup (xfree, name); > tail = name + strlen (RUST_ENUM_PREFIX); > ... > for (token = strtok_r (tail, "$", &saveptr); > > Fix this by always initializing saveptr. > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > OK with that change, and ... > >> 2016-06-29 Manish Goregaokar <manish@mozilla.com> >> >> gdb/ChangeLog: >> * rust-lang.c (rust_get_disr_info): Initialize saveptr to NULL > > ... add missing period. > > Thanks, > Pedro Alves >
On 06/29/2016 03:42 PM, Manish Goregaokar wrote: > Fixed and pushed. Thanks. Please don't put the "gdb/ChangeLog:" lines in the ChangeLog file though. :-) > > Is this something we can fix in gcc? Pointer overflow is UB IIRC so > <nonnullptr>+<positive thing> should never be null. Not sure if the > compiler knows that this is positive though. I'd hope so. Was already working on a minimal reproducer. :-) Filed a gcc bug now: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71699 Thanks, Pedro Alves
Yipes, sorry about that. Copy-paste error :) -Manish On Wed, Jun 29, 2016 at 8:42 PM, Pedro Alves <palves@redhat.com> wrote: > On 06/29/2016 03:42 PM, Manish Goregaokar wrote: >> Fixed and pushed. > > Thanks. Please don't put the "gdb/ChangeLog:" lines in the > ChangeLog file though. :-) > >> >> Is this something we can fix in gcc? Pointer overflow is UB IIRC so >> <nonnullptr>+<positive thing> should never be null. Not sure if the >> compiler knows that this is positive though. > > I'd hope so. Was already working on a minimal reproducer. :-) > Filed a gcc bug now: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71699 > > Thanks, > Pedro Alves >
On 06/29/2016 03:25 PM, Pedro Alves wrote: > gcc 7 shows a bit more detail, though it's still not very clear: > > /home/pedro/gdb/mygit/src/gdb/rust-lang.c: In function ‘rust_get_disr_info.isra.5’: > /home/pedro/gdb/mygit/src/gdb/rust-lang.c:173:15: error: ‘__s’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > ret.name = concat (TYPE_NAME (type), "::", token, (char *) NULL); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: some warnings being treated as errors > > > The mention of "__s" is a hint. Filed this one as: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71701 Thanks, Pedro Alves
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c index c01687a..1849349 100644 --- a/gdb/rust-lang.c +++ b/gdb/rust-lang.c @@ -121,7 +121,7 @@ rust_get_disr_info (struct type *type, const gdb_byte *valaddr, if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX, strlen (RUST_ENUM_PREFIX)) == 0) { - char *tail, *token, *name, *saveptr; + char *tail, *token, *name, *saveptr = NULL; unsigned long fieldno; struct type *member_type;