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

[Dag-Erling Smørgrav]

Bruce Evans <brde at optusnet.com.au> writes:

> 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.

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()?

> 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).

> 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 **);

> 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.

> 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.

> > 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:

cc1: warnings being treated as errors
/home/des/freebsd/base/head/lib/libthr/thread/thr_affinity.c: In function '_pthr
ead_setaffinity_np':
/home/des/freebsd/base/head/lib/libthr/thread/thr_umtx.h:139: warning: will never be executed

because _thr_umutex_unlock2() is called with defer == NULL at that
point.

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.

DES
-- 
Dag-Erling Smørgrav - des at des.no