svn commit: r274340 - in head/sys: crypto/rijndael dev/random geom/bde

[Bruce Evans]

On Tue, 11 Nov 2014, [utf-8] Dag-Erling Smørgrav wrote:

> Bruce Evans <brde at optusnet.com.au> writes:
>> -Wcast-qual is not a very good warning option since the official way
>> to remove qualifiers in C is to cast them away.  Casting them away is
>> better than using the __DECONST() abomination.  The option exists
>> because it is too easy for sloppy code to cast away const without
>> really intending to or when casting away const is done intentionally
>> but is an error.
>
> I agree that __DECONST() is ugly (not least because it strips all
> qualifiers, not just const, so it should be DEQUAL()),

It is not quite that broken.  __DEQUALIFIER() strips all qualifiers,
but __DECONST() starts by casting to const void *; thus if the initial
type has a volatile qualifier, and -Wcast-qual is configured, and
-Wcast-qual is not broken, then you get a cast-qual warning for
attempting to strip volatile.

> but the
> alternative is worse.  In my experience, the majority of cases where a
> cast discards a qualifier are bugs, with struct iov being one of very
> few legitimate use cases.

That is a (design) bug too.  The pointer type is 'const void *' for
write() and plain 'void *' for read(), but struct iov is older than
const so there aren't separate pointers to keep the types separate in
it.  read() and write() are even older, but changing the API of write()
to add the const didn't risk breaking so much so it was done.  It was
a much larger API/ABI change and breakage to change read() and write()
to use [s]size_t instead of int.  Interestingly, struct iov was changed
signfificantly without fixing this bug -- in Net/2, it contained
'caddr_t iov_base' instead of void *iobase, and 'int iov_len' instead
of 'size_t iov_len'.

The next level of design errors that require the cast is for the str*()
family.  E.g., strchr() takes a 'const char *' and returns a plain
'char *'.  This is like the error for readv(), except strchr() is
lower level than readv().

The level below that is design errors errors in the C type system.
'const' doesn't work right after the first level of indirection,
so it is impossible to declare functions like strtol() and excecv()
with the correct number of const's, and callers of these functions
need hacks to be comitbly broken.

> In the same vein, you could also argue that it is wrong of gcc and clang
> to warn about underparanthesized boolean expressions or about using an
> assignment as a truth value.  Yet these warnings are extremely useful,
> because code that triggers them is often either incorrect or easily
> misinterpreted by a casual reader.

I certainly complain about their warning about "missing" parentheses for
&& vs || and & vs |.  This is like complaining about "missing" parentheses
for * vs +.  All of these have a standard conventional precdedence and no
design errors for this in C (the C design errors for precedence are only
nearby, with & and | having much lower precedence than * and +, so much
lower that they are below comparison operators; OTOH, it is correct for
&& and || to have lower precedence than comparison operators).  When I
maintained local patches for gcc-2.4.5 and gcc-2.7.2 ending when the ports
tree started about 19 years ago, I used to delete check for && vs ||.

This bug is larger in clang, since clang enables the && vs || warnings
at a lower level than gcc, so some of my code where I intentionally
leave out the parentheses exposes the brokenness of the compiler.

> Apple's "goto fail" certificate verification bug was caused by code that
> was perfectly legal and looked fine at first glance but would have been
> caught by -Wunreachable-code.  Unfortunately, turning it on in our tree
> breaks the build in non-trivially-fixable ways because it is triggered
> by const propagation into inline functions.

Turning on warnings finds too many problems in dusty decks.  Compilers
shouldn't put new warnings under only warning flags since this mainly
finds non-bugs in code that is not dusty enough to need compiling with
no warning flags.  -Wunreachable code is fine here since it is new.

I don't know of any solution for killing warnings selectively except for
ugly lint-like markup.

Bruce