Project

General

Profile

Bug #8652

Tautological comparisons with ZPROP_INVAL

Added by Alan Somers about 2 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
zfs - Zettabyte File System
Start date:
2017-09-12
Due date:
% Done:

100%

Estimated time:
Difficulty:
Bite-size
Tags:
needs-triage

Description

Clang and GCC prefer to use unsigned ints to store enums. With Clang, that causes tautological comparison warnings when comparing a zfs_prop_t or zpool_prop_t variable to the macro ZPROP_INVAL. It's likely that error handling code is being silently removed as a result.

Here's an example when using Clang to build libzfs on FreeBSD:
/usr/home/alans/freebsd/head/cddl/contrib/opensolaris/lib/libzfs/common/libzfs_changelist.c:434:24: error: comparison of constant 1 with expression of type 'zfs_prop_t' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
if (clp
>cl_shareprop != ZPROP_INVAL &&
~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
Here's that same error, as shown by GCC 6:
/usr/home/alans/freebsd/head/cddl/contrib/opensolaris/lib/libzfs/common/libzfs_changelist.c:434:24: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
if (clp->cl_shareprop != ZPROP_INVAL &&

I can't reproduce this problem on Illumos simply because its compiler is too old. I think GCC added this warning in version 6. I'm not sure when Clang added this warning.

History

#1

Updated by Alan Somers about 2 years ago

Here's GCC's explanation for how they handle enum signededness. I would guess that the problem exists on Illumos, but it's silent.
https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html#Structures-unions-enumerations-and-bit-fields-implementation

#2

Updated by Igor Kozhukhov about 2 years ago

Alan Somers wrote:

Here's GCC's explanation for how they handle enum signededness. I would guess that the problem exists on Illumos, but it's silent.
https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html#Structures-unions-enumerations-and-bit-fields-implementation

i have fixed it on DilOS and will try upstream it soon

#3

Updated by Alan Somers about 2 years ago

Don't sweat it Igor. I've already fixed it on FreeBSD and I should have a pull request for OpenZFS later today.

#4

Updated by Electric Monk over 1 year ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 4ae5f5f06c6c2d1db8167480f7d9e3b5378ba2f2

commit  4ae5f5f06c6c2d1db8167480f7d9e3b5378ba2f2
Author: Alan Somers <asomers@gmail.com>
Date:   2018-01-18T17:07:03.000Z

    8652 Tautological comparisons with ZPROP_INVAL
    Reviewed by: Matthew Ahrens <mahrens@delphix.com>
    Reviewed by: Igor Kozhukhov <igor@dilos.org>
    Approved by: Gordon Ross <gwr@nexenta.com>

#5

Updated by Prakash Surya over 1 year ago

The fix for this missed a couple lines, and these omissions result in warnings for later GCC versions. Here's the fix that is needed:

diff --git a/usr/src/cmd/zpool/zpool_main.c b/usr/src/cmd/zpool/zpool_main.c
index 3f8bee81c0..a6e878a341 100644
--- a/usr/src/cmd/zpool/zpool_main.c
+++ b/usr/src/cmd/zpool/zpool_main.c
@@ -413,7 +413,7 @@ static int
 add_prop_list(const char *propname, char *propval, nvlist_t **props,
     boolean_t poolprop)
 {
-       zpool_prop_t prop = ZPROP_INVAL;
+       zpool_prop_t prop = ZPOOL_PROP_INVAL;
        zfs_prop_t fprop;
        nvlist_t *proplist;
        const char *normnm;
@@ -431,7 +431,7 @@ add_prop_list(const char *propname, char *propval, nvlist_t **props,
        if (poolprop) {
                const char *vname = zpool_prop_to_name(ZPOOL_PROP_VERSION);

-               if ((prop = zpool_name_to_prop(propname)) == ZPROP_INVAL &&
+               if ((prop = zpool_name_to_prop(propname)) == ZPOOL_PROP_INVAL &&
                    !zpool_prop_feature(propname)) {
                        (void) fprintf(stderr, gettext("property '%s' is " 
                            "not a valid pool property\n"), propname);

#6

Updated by Electric Monk over 1 year ago

git commit d2aa06e8e95fc3abd82008cdd0959cbbdb2d50fb

commit  d2aa06e8e95fc3abd82008cdd0959cbbdb2d50fb
Author: Andrew Stormont <astormont@racktopsystems.com>
Date:   2018-01-22T21:38:05.000Z

    8652 Tautological comparisons with ZPROP_INVAL (followup)
    Reviewed by: Matthew Ahrens <mahrens@delphix.com>
    Reviewed by: Igor Kozhukhov <igor@dilos.org>
    Approved by: Robert Mustacchi <rm@joyent.com>

Also available in: Atom PDF