Date: Wed, 12 Nov 2014 16:13:42 +0100 From: =?utf-8?Q?Dag-Erling_Sm=C3=B8rgrav?= <des@des.no> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, "Bjoern A. Zeeb" <bz@freebsd.org>, src-committers@freebsd.org Subject: Re: svn commit: r274340 - in head/sys: crypto/rijndael dev/random geom/bde Message-ID: <86h9y44fkp.fsf@nine.des.no> In-Reply-To: <20141112100207.Q1068@besplex.bde.org> (Bruce Evans's message of "Wed, 12 Nov 2014 10:51:48 %2B1100 (EST)") References: <201411100944.sAA9icnN061962@svn.freebsd.org> <3C962D07-3AAF-42EA-9D3E-D8F6D9A812B0@FreeBSD.org> <86sihq5a2v.fsf@nine.des.no> <20141111223756.F3519@besplex.bde.org> <86oasd6dad.fsf@nine.des.no> <20141112100207.Q1068@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Bruce Evans <brde@optusnet.com.au> writes:
> On Tue, 11 Nov 2014, [utf-8] Dag-Erling Sm=C3=B8rgrav wrote:
>
>> Bruce Evans <brde@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 =3D=3D 0 should mean
(foo & bar) =3D=3D 0, not foo & (bar =3D=3D 0) - although there are a few c=
ases
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)
{
=20=20=20=20=20=20=20=20
/* a */ if (s1 =3D=3D NULL)
return (s2);
/* b */ if (s2 =3D=3D NULL)
return (s1);
/* c */ return (strcmp(s1, s2) <=3D 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 >=3D 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 =3D=3D 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
--=20
Dag-Erling Sm=C3=B8rgrav - des@des.no
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?86h9y44fkp.fsf>