svn commit: r324178 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/lua

[Bruce Evans]

On Sun, 1 Oct 2017, Ian Lepore wrote:

> On Sun, 2017-10-01 at 22:37 +0200, O. Hartmann wrote:

>>> Author: avg

>>> Log:
>>> \xa0 unbreak kernel builds on sparc64 and powerpc after r324163, ZFS
>>> Channel Programs
>>> \xa0\xa0
>>> \xa0 The custom iscntrl() in ZFS Lua code expects a signed argumnet,
>>> so
>>> \xa0 remove the harmful cast.

[Context lost to mangling of whitespace]

Removing the cast is more fragile than the usual "fix" for warnings
about checking that unsigned values are less than 0.  Robust code must
do such checks in case the type is changed to a correctly signed type,
unless the type is specified as unsigned in an API (then it is the API
that is broken, but this is hard to fix).

Here the behaviour of the function iscntrl(3) is undefined unless the
value is representable as and unsigned char or has the value of the
macro EOF, so iscntl() on a signed character is undefined except when
the value of the character is EOF in which case the result is wrong
iff that character is a control character.  So iscntrl(*s) for char *s
is undefined for about half of all characters when chars are signed.
This is normally "fixed" by casting *s to u_char.  This is broken for
at least the signed char with value EOF if that character is a control
character, and it is unclear what happens for other negative characters,
especially on exotic arches with sizeof(char) == sizeof(int) where
promotion to int doesn't change the size.  The ctype API itself is broken
as designed on these arches (there is no space to keep EOF separate from
characters).

However, the function here is iscntrl(9).  This is only implemented
for cddl and is undodocumented, so no one knows what it does.  Looking
at its internals, it is easy to see that it accepts negative values and
treats them as non-control characters, and that since the check for
negative numbers is the tautologic comparison (C) >= 0 if the arg is
unsigned (as it is on arches with unsigned chars) or just known to
be nonnegative (it might be an unsigned char promoted to int)), there
must be another bug for the warning to not occur in the implementation
too.  This other bug seems to be just that the kernel is not compiled
-Wsystem-headers to unbreak such warnings.  Userland adds -Wsystem-headers
at WARNS >= 1, but kernel makefiles have never added it.  Kernel makefiles
used to use -I- and this should made all headers non-system.  This is
broken now -- -I- was not converted to -Iquote.

> Also, removing the cast won't really be a fix on a platform that has
> default unsigned chars (which arm does, but arm doesn't use gcc; not
> sure about other platforms).

It is really a break on arches with signed char and a different
implementation of iscntrl(), and should have no effect on arches
with unsigned chars, since the arg is alread unsigned then.

gcc has some magic to reduce warnings about tautological comparisons.
Such warnings are usually just harmful, so they shouldn't be printed
unless the compiler can prove that the tautology is independent of
the arch.  The u_char cast allows it to do that.  But when the type is
char, its unsigneness is MD, so the warning should never be printed.
But gcc is apparently not smart enough to do this.  I checked in userland
with -funsigned-char on amd64.  gcc -Wall warns then.  Another bug in
clang is that it doesn't warn then.  clang doesn't even warn about
nnon-forced unsigned char with clang -Wall -Wtautological-compare.
The expression in the test was (c >= 0).  It takes -Wsign-compare to
get a warning about this.

Compilers can prove that values are in a certain range on all arches and
shouldn't warn then either.  The most interesting case is comparing a
loop index against an unsigned variable:

 	int i;

 	for (i = 0; i < 1U; i++)
 		foo();

Compilers shouldn't warn about this, since the int variable is provably
always 0 or 1, and thus cannout e negative, and thus cannot cause any
sign or unsign-extension bugs in the comparison, but ones like clang
-Wsign-compare do.  "Fix"ing this warning by changing i to unsigned
then tends causes actual unsign-extension bugs and further unsigned
poisoning in more complicated code.  The unsigned type for the limit
often occurs naturally as sizeof() due to the bug that sizeof() is
unsigned.  (In K&R1, size_t didn't exist and the type of sizeof()
wasn't specified.  It was presumably int.  This was unimproved by
specifying size_t to be an unsigned type.)

Bruce