On Wed, 12 Nov 2014, [utf-8] Dag-Erling Sm??rgrav wrote:
Bruce Evans <b...@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
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
