From owner-freebsd-fs@FreeBSD.ORG Mon Apr 13 15:37:30 2015 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 383AC921 for ; Mon, 13 Apr 2015 15:37:30 +0000 (UTC) Received: from mail-la0-x22c.google.com (mail-la0-x22c.google.com [IPv6:2a00:1450:4010:c03::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id C9507BCA for ; Mon, 13 Apr 2015 15:37:29 +0000 (UTC) Received: by lagv1 with SMTP id v1so60757208lag.3 for ; Mon, 13 Apr 2015 08:37:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=delphix.com; s=google; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=DQo401mk3SKPrmQDfNM/7VOkswgCeZCNX4XBZCre158=; b=SuM0NtvmiJzvA0wYFyDJrGdYHOrYiCri0Y7kADvGlvuxt9njNM/lvG3jrHu63VnKVF //o9t52doxVsnD9UOhBTM4Z2GQvN7oHDMhgkiaprK3XMrfPucIUQ5IjpVxJs4p26fEnH oHkT4E265xLiG8pRwPrWhUfHC8JQYbQknGHNw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=DQo401mk3SKPrmQDfNM/7VOkswgCeZCNX4XBZCre158=; b=bzUt1rTUI5WsVcVAQKLJqPjIkjzNXYQikErtrDHFjbDvovaaRt6qpkP2mm3VJnHtDi 0WutxVMDwhoD84NVL54bgR+icGZwoqQgphh8bdz5IAMTi8zsdMzL+PPSbjmYnwD2uf/d 0TeL+WkXUBxajnoAnnlObSX7wTZb0YNbgiMcXg8W4ctMLXXpdMEutvPuMP+PTL3hfihy Y2BzP8KZmq7q90Ri/u27uHom6OTlN8WEB1Ieci8/gzuqgCxksIi/9L1NXwoW8jgQGkDG Flv/CygFJdy784xuYMbPSAPQ64L190ZgPXXtih07Q7w44zikq+l1FO5U5/jAtgmqudNA h4nA== X-Gm-Message-State: ALoCoQmnPw90nAryR/elYjBarMCBf0l62/SYYBsGVm/qE2v1PkM38WVcLKhQ+dPrXROaIy89cJBQ MIME-Version: 1.0 X-Received: by 10.112.98.201 with SMTP id ek9mr13857118lbb.68.1428939447624; Mon, 13 Apr 2015 08:37:27 -0700 (PDT) Received: by 10.25.44.130 with HTTP; Mon, 13 Apr 2015 08:37:27 -0700 (PDT) In-Reply-To: <21800.25526.8680.435299@khavrinen.csail.mit.edu> References: <21800.25526.8680.435299@khavrinen.csail.mit.edu> Date: Mon, 13 Apr 2015 08:37:27 -0700 Message-ID: Subject: Re: ZFS compiler warnings From: Matthew Ahrens To: Garrett Wollman Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.18-1 Cc: freebsd-fs X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Apr 2015 15:37:30 -0000 On Fri, Apr 10, 2015 at 4:58 PM, Garrett Wollman wrote: > I notice on my 10.1 kernel builds warnings like the following: > > /usr/src/sys/modules/zfs/../../cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c:2855:12: > warning: comparison of constant -1 with expression of type 'zfs_prop_t' is > always true [-Wtautological-constant-out-of-range-compare] > if (prop != ZPROP_INVAL && !zfs_prop_inheritable(prop)) > ~~~~ ^ ~~~~~~~~~~~ > /usr/src/sys/modules/zfs/../../cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c:3810:11: > warning: comparison of constant -1 with expression of type 'zfs_prop_t' is > always false [-Wtautological-constant-out-of-range-compare] > if (prop == ZPROP_INVAL) { > ~~~~ ^ ~~~~~~~~~~~ > > The warnings indicate that these checks are being optimized away by > clang. The code in head is the same. Does anyone have an idea of how > this should be fixed, in a way that would be acceptable upstream? (Do > we have someone who is the "official" liaison with the OpenZFS > project?) > You change makes sense to me. I'd be happy to see it go upstream in illumos too. --matt > > My guess is that the definition of the zfs_prop_t enum itself needs to > include these two constants explicitly -- but that also has the effect > of changing the type of ZPROP_INVAL, which then means that > zpool_prop_t needs to have its own version of "invalid property" > defined, and that then requires cascading changes in a bunch of other > places. > > The following is a minimal change that fixes the warnings in the > kernel alone (beware whitespace lossage) -- I haven't checked the > userland side: > > Index: sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c > =================================================================== > --- sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c (revision > 281265) > +++ sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c (working > copy) > @@ -332,7 +332,7 @@ > zprop_source_t src = ZPROP_SRC_DEFAULT; > zpool_prop_t prop; > > - if ((prop = zpool_name_to_prop(za.za_name)) == ZPROP_INVAL) > + if ((prop = zpool_name_to_prop(za.za_name)) == > ZPOOL_PROP_INVAL) > continue; > > switch (za.za_integer_length) { > @@ -422,7 +422,7 @@ > zpool_prop_t prop = zpool_name_to_prop(propname); > > switch (prop) { > - case ZPROP_INVAL: > + case ZPOOL_PROP_INVAL: > if (!zpool_prop_feature(propname)) { > error = SET_ERROR(EINVAL); > break; > @@ -662,7 +662,7 @@ > prop == ZPOOL_PROP_READONLY) > continue; > > - if (prop == ZPOOL_PROP_VERSION || prop == ZPROP_INVAL) { > + if (prop == ZPOOL_PROP_VERSION || prop == > ZPOOL_PROP_INVAL) { > uint64_t ver; > > if (prop == ZPOOL_PROP_VERSION) { > @@ -6308,7 +6308,7 @@ > spa_feature_t fid; > > switch (prop = zpool_name_to_prop(nvpair_name(elem))) { > - case ZPROP_INVAL: > + case ZPOOL_PROP_INVAL: > /* > * We checked this earlier in spa_prop_validate(). > */ > Index: sys/cddl/contrib/opensolaris/uts/common/sys/fs/zfs.h > =================================================================== > --- sys/cddl/contrib/opensolaris/uts/common/sys/fs/zfs.h (revision > 281265) > +++ sys/cddl/contrib/opensolaris/uts/common/sys/fs/zfs.h (working > copy) > @@ -152,7 +152,9 @@ > ZFS_PROP_SNAPSHOT_COUNT, > ZFS_PROP_REDUNDANT_METADATA, > ZFS_PROP_PREV_SNAP, > - ZFS_NUM_PROPS > + ZFS_NUM_PROPS, > + ZPROP_INVAL = -1, > + ZPROP_CONT = -2 > } zfs_prop_t; > > typedef enum { > @@ -196,14 +198,18 @@ > ZPOOL_PROP_FREEING, > ZPOOL_PROP_FRAGMENTATION, > ZPOOL_PROP_LEAKED, > - ZPOOL_NUM_PROPS > + ZPOOL_NUM_PROPS, > + ZPOOL_PROP_INVAL = -1, > + ZPOOL_PROP_CONT = -2 > } zpool_prop_t; > > /* Small enough to not hog a whole line of printout in zpool(1M). */ > #define ZPROP_MAX_COMMENT 32 > > +/* > #define ZPROP_CONT -2 > #define ZPROP_INVAL -1 > +*/ > > #define ZPROP_VALUE "value" > #define ZPROP_SOURCE "source" > > There is also one spot in zio_checksum.c that has a similar problem, > but in this case the issue is that it assumes that an out-of-range > value can be bitwise-or'ed with a small enum. (The C standard allows > the compiler to use an eight-bit integer type as the underlying type > of 'enum zio_checksum', so to bitwise-or it with ZIO_CHECKSUM_VERIFY, > defined to be 256, is undefined.) A similar change eliminates this > warning. (The effect in both cases is to convince the compiler to > model the enum with a larger underlying type.) > > -GAWollman > _______________________________________________ > freebsd-fs@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-fs > To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org" >