From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33408 invoked by alias); 29 Jun 2016 14:25:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 33398 invoked by uid 89); 29 Jun 2016 14:25:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=BAYES_00,KAM_ASCII_DIVIDERS,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=handful, glibcs, hint, glibc's X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 29 Jun 2016 14:25:19 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 55A3A3F725; Wed, 29 Jun 2016 14:25:18 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5TEPGRe025326; Wed, 29 Jun 2016 10:25:17 -0400 Subject: Re: [PATCH] Initialize strtok_r's saveptr to NULL To: Manish Goregaokar , gdb-patches@sourceware.org References: From: Pedro Alves Message-ID: Date: Wed, 29 Jun 2016 14:25:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2016-06/txt/msg00515.txt.bz2 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$$$...$ 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 > > gdb/ChangeLog: > * rust-lang.c (rust_get_disr_info): Initialize saveptr to NULL ... add missing period. Thanks, Pedro Alves