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

[Bruce Evans]

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

> Bruce Evans <brde at optusnet.com.au> writes:
>
>> On Tue, 11 Nov 2014, [utf-8] Dag-Erling Smørgrav wrote:
>>> ...
>>> 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.
>
> Yes, I hate struct iov, but what is the alternative?  An anonymous union
> inside struct iov so we have a non-const pointer for readv() and a const
> pointer for writev()?

For userland, perhaps an anonymous union would work now, but old versions
should have used separate structs.  In the kernel (implementation), just
drop the const qualifier.  This is not quite right, but note that we
already do it for most user pointers starting with pathames and read().
This is done using type puns in syscalls.master for most pathnames.  It
is not done for some newer syscalls including chflags(), and namei() is
apparently ready for this, in contradiction to the comments in
syscalls.master saying that things are not ready (ni_dirp is const char *,
and since that works the const poisoning must have been pushed to all
lower levels).  For write(), it is done bogusly using iov:
- write() isn't type punned in syscalls.master, although that would be
   the simplest hack
- the const void * pointer is assigned to iov_base using a home made
   version of __DECONST().

>> 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().
>
> This is trivially fixable by defining it as a macro instead.  However,
> there is probably code out there that uses &strchr for some purpose or
> other, and any autoconf script that tests for the existence of strchr
> will break unless it uses AC_CHECK_DECL instead of AC_CHECK_FUNC (which
> is non-idiomatic but not wrong, as AC_CHECK_DECL checks for both).

Er, the problem is in the strchr() implementation where no macro is
needed, and in the API breaking type safety.

strchr may be a macro, but is required to be backed by a function.  It
must be parenthesized before applying & to it to get the function.

>> 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.
>
> Tell me about it.  It's a constant annoyance in PAM:
>
>  int pam_get_item(const pam_handle_t *, int, const void **);

Possibly the API just shouldn't use const void **, since the type
system cannot handle it.

>> I certainly complain about their warning about "missing" parentheses for
>> && vs || and & vs |.  This is like complaining about "missing" parentheses
>> for * vs +.
>
> These warnings are there for the same reason: frequent mistakes in both
> reading and writing complex boolean expressions.

I like writing especially complex boolean expressions and never make these
mistakes :-).

>> 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;
>
> That never fails to piss me off.  90% of the time I check operator(7) is
> to verify that I got & and | right.  IMHO, foo & bar == 0 should mean
> (foo & bar) == 0, not foo & (bar == 0) - although there are a few cases
> where the latter is useful: foo & bar is equivalent to foo || bar but
> without shortcut evaluation.  I sometimes use this construct in unit
> tests.

Sure, but that is the main design error in C precedence.  The compiler
should warn about this but not about foo & bar | baz.  Adding parentheses
for the latter makes it harder (for humans) to parse the important parentheses
for (foo & bar) == 0.

Non-bitwise logical expressions are more interesting.  Now you want == to
bind tighter than ||, so that you can write foo == 0 || bar == 0, and
compilers mercifully don't warn for this, but they do warn for
foo && bar || baz even though it is especially unambiguous since it has
no comparision operators in it.

>>> 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.
>
> This particular case is not a "dusty deck".  Try something like this -
> off the top of my head and somewhat contrived, but I think it is an
> adequate demonstration of the problem:
>
>        /*
>         * Return the (lexically) lesser of two strings.  If one of
>         * the arguments is NULL, return the other.
>         */
>        static inline char *
>        strmin(char *s1, char *s2)
>        {
>
> /* a */         if (s1 == NULL)
>                        return (s2);
> /* b */         if (s2 == NULL)
>                        return (s1);
> /* c */         return (strcmp(s1, s2) <= 0 ? s1 : s2);
>        }
>
> Wherever you use strmin(), if gcc is able to determine that either s1 or
> s2 is NULL, you will get an "unreachable code" warning at point b or c,
> and possibly a bonus "condition is always true" warning.
>
> This is what happens when I try a gcc buildworld with -Wunreachable-code
> added at WARNS >= 3:

Does it also print "undefined behaviour: strcmp() with a null pointer" if
you omit the arg checking and it determines that one of the args is null.
That would be a much more useful diagnostic, but if it does that then it
should provide a way to avoid the undefined behaviour without getting a
different diagnostic.

I also doing like the error for:

int
foo(bar_t b)
{
 	if (b < 0)
 		return (EINVAL);
 	...
}

where bar_t happens to an unsigned type so the error checking is null.
The compiler warns, and the "fix" is to make the code less robust by
removing the error checking.

> This seems to have been fixed somewhere between 4.2 and 4.8.  Clang does
> not complain, but I don't know whether that is because it's smarter than
> GCC 4.2 or because -Wunreachable-code is unimplemented.  There is no
> documentation of Clang's -W options.

Maybe they are documented on the net, but clang almost no documentation
of anything in FreeBSD installations of it.

Bruce