svn commit: r328251 - vendor-sys/illumos/dist/uts/common/fs/zfs vendor-sys/illumos/dist/uts/common/sys/fs vendor/illumos/dist/cmd/zpool vendor/illumos/dist/lib/libzfs/common

[Bruce Evans]

On Mon, 22 Jan 2018, Alexander Motin wrote:

> Log:
>  8652 Tautological comparisons with ZPROP_INVAL
>
>  illumos/illumos-gate at 4ae5f5f06c6c2d1db8167480f7d9e3b5378ba2f2
>
>  https://www.illumos.org/issues/8652:
>  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.

This shows another reason to never use enums.  emum constants have type int,
but enum variables have an implementation-defined type capable of
respresenting the values of all of the members of the enumeration.

Inexperienced programmers use unsigned types to implement unsign extension
bugs and bugs like this.  If you use basic types or even your own typedefs,
then unsigned types are easy to avoid.  APIs tend to have too many unsigned
types, but only especially badly designed ones allow the signedness to
vary with the implementation or don't document what it is.

The implementation's definition of this is hard to find.  gcc.info has a
second that clearly documents that it is required to document this
(C99 6.7.2.2), but I couldn't find where it actually documents this.
clang has no useful installed documentation.

The type of an enum can be forced to be signed by putting a negative
enum value in it.  ZPROP_INVAL = -1 as an enum value would do this.  This
wouldn't work with a generic INVAL value used with different enums.  This
is the fix used here -- another recent commit added ZPOOL_PROP_INVAL as
an enum value and this commit uses it.  I think this changes the type of
the enum from u_int to int, so it risks sign extension bugs instead of
unsign extension bugs.

Unsigned types might be used to pack 256 enum values in 8 bits, but
gcc and clang on x86 use uint32_t for even 1-bit enums like
"enum foo { ZERO, ONE, };", just like inexperienced programmers.

gcc and clang have an option -fshort-enums which makes unsigned
types give better packing in a few cases --
"enum foo { N = 256 };" can be fitted in 1 byte, and
"enum foo { N = 65536 };" can be fitted in 2 byte.  With 32-bit ints,
uint32_t enums just give the unsign extension bugs since enum values
have type int so "enum foo { N = 0xffffffff };" is invalid.

A similar class of unsign extension bugs augmented by wrong fixes is
when you have an opaque type with careful range checking of the form:

 	opaque_t v = foo();

 	if (v < MIN || v > MAX)
 		error();

First v is misimplemented an unsigned type.  When MIN == 0, (v < MIN)
becomes tautologous.  Then the warning is "fixed" by removing this check.
Then when v is correctly implemented by fixing its type in this implemenation
or never breaking it in other implementations, the code is broken.  Changing
a single typedef or enum in a header file can uncover many latent sign
extension, unsign extension, spurious warning, or missing range checking bugs.

Bruce