Bug #8652
closed
Tautological comparisons with ZPROP_INVAL
100%
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]>cl_shareprop != ZPROP_INVAL &&
if (clp
~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
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.
Updated by Alan Somers almost 5 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
Updated by Igor Kozhukhov almost 5 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
Updated by Alan Somers almost 5 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.
Updated by Electric Monk over 4 years 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>
Updated by Prakash Surya over 4 years 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);
Updated by Electric Monk over 4 years 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>