Date: Mon, 19 Dec 2011 11:12:04 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dimitry Andric <dim@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r228668 - head/usr.bin/netstat Message-ID: <20111219080853.X877@besplex.bde.org> In-Reply-To: <4EEE136F.5080904@FreeBSD.org> References: <201112172232.pBHMW1Bd079555@svn.freebsd.org> <4EED18B5.8000907@FreeBSD.org> <20111218013905.GA20867@zim.MIT.EDU> <4EEE136F.5080904@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 18 Dec 2011, Dimitry Andric wrote:
> On 2011-12-18 02:39, David Schultz wrote:
> ...
>>>> Log:
>>>> Revert r228650, and work around the clang false positive with printf
>>>> formats in usr.bin/netstat/atalk.c by conditionally adding NO_WFORMAT to
>>>> the Makefile instead.
> ...
>> Have you been keeping track of the other hacks you've been
>> sprinkling throughout the tree to work around clang bugs, e.g.,
>> the one in fsdb? It would be unfortunate if someone else has to
>> waste their time later on figuring out what you did, when we could
>> just as easily have waited a month for the clang bug to be fixed.
>
> Yes, I will revert the fsdb change too, and add a workaround in the
> Makefile. Though after talking with a language lawyer type person, it
> seems that clang was actually right with its original warning.
>
> Apparently, if you use ?: with two shorts, the end result always gets
> promoted to an int, due to the Usual Arithmetic Conversions, so using
> the "%hu" format is not correct. The following small program
> demonstrates this:
An expression starting with shorts indeed gets promoted by ?: (or
almost any binary or unary operator), but that is irrelevant provided
the result of the expression is representable as a short. It also
gets promoted by passing it, and the compiler can't tell the difference
at that point so it shouldn't warn (since this is the point at which
printf() will see it and the purpose of the warning is to check to for
things that will cause problems at the level of printf()).
For the DIP() expression, as for any typical ?: expression, the result
is representable in the same type as the operands if that type is
common, since ?: just selects between the operands. (In most cases,
the operands in DIP() have different types, but in cases where the
ffs1 type has not been expanded, they have the same type, and for
nlink_t this type is also (unsigned) short.) If the expesssion involved
a more complicated operator like addition, this would not be obvious
but might still be true. The compiler could determine this especially
easily for constant expressions. It is important that compilers not
warn for constant and other expressions that they know won't cause any
problems for printf(). Passing sizeof(foo) for a field width is not
quite an example, since the width of sizeof(foo) differs from the width
of what printf expects (that is, sizeof(int)) on some machines, so you
want it warned about always for portable code.
The original non-problem could be checked for in a more refined class
of warnings, starting with:
- -Wbogus-cast[level]: warn if a cast is bogus at the given level.
Some possible levels:
- 0: bde doesn't like it
- 1: compiler maintainer doesn't like it
- 100-200: has no effect because it is:
- 100: the identity conversion in the abstract machine
- 101: the identity conversion in the current machine
- 102: same as the default operator promotion in the abstract machine
- 103: same as the default operator promotion in the current machine
- 104: same as the default parameter promotion in the abstract machine
- 105: same as the default parameter promotion in the current machine
- 106: same as prototype parameter conversion in the abstract machine
- 107: same as prototype parameter conversion in the current machine
- 110-119: like 100-109, but changes the type, but then the next
operation or cast or parameter type changes the type and also
the value back it would have been without any cast at this level,
so that the combination is null. This is useful mainly in
conjunction with parameter-passing, as an extension of 106-107.
We don't want to attach the warning to the final conversion given
by a prototye, since the conversion given by prototypes is
non-bogus in other contexts.
- -Wsubtle-conversion[level]. Sort of the inverse of the above. If you
add a cast to quiet a warning from this, then it should probably
cause a warning from -Wbogus-cast[level]. I haven't thought this
through, but in the present case -Wbogus-cast[114] would warn about
casting down DIP() to u_short, since this has no effect because the
default parameter promotion will turn it back into int.
BTW, the printf format for di_nlink in fsdb is still broken in 2 ways.
nlink_t is bogusly unsigned (uint16_t), but the format is %hd (signed
short).
First way: Using %h at all is broken, since it only works for 16-bit
shorts, and nlink_t is a typedef to avoid hard-coding this.
Second way: sign mismatch. The top bit of nlink_t is supposed to be
unused (LINK_MAX = 32767), so this is relatively harmless, but fsdb
is supposed to be able to work on current file systems, so an
invalid i_nlink might be > LINK_MAX and then it will be misprinted.
An invalid i_nlink of nearly 65535 probably means a "negative" value,
and then printing it as signed is better. Both cases can't be handled,
so the format should match the type.
All of these problems and subtle non-problems can be avoided by not
using %h. Just use the format that matches the arg type. This is
just %d matching int after the default promotions (either inside DIP()
or in parameter passing). This assumes that nlink_t is strictly smaller
than int, which is a weaker (though not strictly weaker) assumption
than the wrong assumption that it is short. Since it is strictly
smaller, it the promotion is non-null and the type loses the signedness.
The promoted value is always non-negative since the original type was
unsigned, and the same as the original value since the conversion was
a promotion. This value can be naturally printed using %d (%u works
too, but doesn't match the parameter types, so there are minor subtleties
to prove that it works). However, printing the value as non-negative is
not bug for bug compatible with the original code, which was essentially:
printf("LINKCNT=%hd", dp->di_nlink);
This essentially abuses %h to corrupt the type from uint16_t to short
(in FreeBSD-1, nlink_t was u_short, so only the sign was corrupted).
If you want to pretend that your uint16_t's are actually signed, then
the right way to do this is to replace them by checking the top bit in
them and then force 2's complement arithmetic:
/* XXX assume nlink_t is uint16_t: */
printf("LINKCNT=%d", (dp->di_nlink & 0x8000 ? -1 : 1) * dp->di_nlink);
Next best is to assume that implementation-defined behaviour is 2's
complement and just cast:
/* XXX assume nlink_t is u_short: */
printf("LINKCNT=%d", (short)dp->di_nlink);
Now with DIP(), we must cast the original di_nlink's, or equivalently
cast DIP() down to u_short first:
/* XXX assume nlink_t is u_short: */
printf("LINKCNT=%d", (short)(u_short)DIP(dp, di_nlink)));
Here the double cast might be warned about by -Wbogus-cast[mumble],
since it has the same effect as directly casting to short, but I left
it in for clarity.
But I think these complications are excessive here, and fsdb should
just use %d on the non-negative DIP().
> #include <stdio.h>
>
> int main(void)
> {
> printf("%zu\n", sizeof(1 > 2 ? (short)1 : (short)2));
> return 0;
> }
>
> It will always print sizeof(int), e.g. 4 on most arches. This is not
> what most programmers expect, I guess, at least I didn't. :)
I've always expected this since I fixed type bugs in my compiler :-).
I tried hundreds of expressions like the above in various compilers
to see what is normal. Another interesting case is cpp expressions.
C eventually standardized on only [u_]long arithmetic (now [u]intmax_t)
in cpp expressions, but my K&R-ish compiler has fully typed cpp
expressions including sizeof(), casts and floating point, so its cpp
expressions give the same results as runtime expressions but different
results from Standard cpp expressions. This was useful for determining
runtime behaviour at compile time.
> Since htons() and ntohs() are implemented in our tree with the __bswap
> macros, which use ?: operators (at least when GNU inline asm and
> __builtin_constant_p are supported), they will in fact return int, not
> uint16_t.
This is very implementation-dependent, and also broken if that is what
they do now:
Broken:
htons(), etc., are now standardized, and any reasonable standard
requires the optional macro versions of them to act as if they were
functions. I don't know if POSIX is reasonable here. In the draft7
2001 version, I could only easily find:
% 7321 The following shall either be declared as functions, defined as macros, or both. If functions are
% 7322 declared, function prototypes shall be provided.
% 7323 uint32_t htonl(uint32_t);
% 7324 uint16_t htons(uint16_t);
% 7325 uint32_t ntohl(uint32_t);
% 7326 uint16_t ntohs(uint16_t);
Thus if they are implemented as functions, they must return a
non-promoted type. Any macro versions should return the same
non-promoted type.
Some recent implementations changed to using inline functions more.
This might have changed the type. There are also the ones with
a constant variant. A misimplementation could easily have the
constant variant returning a different type to the variable
version. And a ?: to select between the constant and variable
versions gives the promoted type unless it is cast back down.
Very implementation-dependent:
Apart from the above, at least i386 used to do __byte_swap_word()
entirely in asm except for a statement-expression to give a
result of type u_short (later u_int16_t). ntohs was defined
as __byte_swap_word in FreeBSD-4 and earlier, and there was
no optimization for the constant case to give an ?: expression
anywhere in this family of functions.
The current implemention uses ?: consistently on i386 at least, so
it consistently gives the broken version for __htons() and ntohs().
There used to be function versions of these interfaces. I got someone
to clean up some of this. The cleanup was incomplete, but removed
most of the function versions in libc. Grepping for ntohs in libc now
shows the following vestiges:
- most arches still have ntohs in a Symbol.map, although the function
doesn't exist. This was apparently missed in the cleanup.
- mips still has ntohs in a Makefile.inc and in a ntohs.S. This was
apparently uncleaned after the cleanup.
> A better fix is to add explicit casts to the __bswap macros, as per
> attached patch, which I'm running through a make universe now. (Note
> that some arches, such as arm and mips already add the explicit casts
> for the expected __bswap return types.) Would that be OK to commit?
It seems to be necessary, and should work since even if anyone wants
to use ntohs in a cpp expression, it already won't work because it
has casts and more in it.
Some of the current mess is because jeff needed ntohs etc. to work
in static initializers. This prevents using the same inline function
version for the constant case as for the variable case (gcc now optimizes
the general version nicely for the constant case).
I've reported most of the style bugs and bugs in the mess before. The
following are most obvious now on i386:
- macro args are bogusly named with a leading underscores. Such
underscores are only needed to protect namespaces in inline functions.
They are just obfuscations in macros.
- __bswap16_const() casts its return value back down to __uint16_t. This
is useless, since __bswap16_const() is only usable in __bswap16() where
the value is immediately promoted back to int. (Note that none of the
__bswapN_const() functions is directly usable, since they all
intentionally omit casting their arg for readability.) This was
apparently an attempt to make the final type __uint16_t.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111219080853.X877>