Date: Mon, 22 Jan 2018 20:25:20 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Alexander Motin <mav@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-vendor@freebsd.org Subject: Re: 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 Message-ID: <20180122201244.M1317@besplex.bde.org> In-Reply-To: <201801220448.w0M4mEjB033773@repo.freebsd.org> References: <201801220448.w0M4mEjB033773@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 22 Jan 2018, Alexander Motin wrote: > Log: > 8652 Tautological comparisons with ZPROP_INVAL > > illumos/illumos-gate@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180122201244.M1317>