Date: Fri, 14 Nov 2014 14:01:47 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: =?utf-8?Q?Dag-Erling_Sm=C3=B8rgrav?= <des@des.no> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, "Bjoern A. Zeeb" <bz@freebsd.org>, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r274340 - in head/sys: crypto/rijndael dev/random geom/bde Message-ID: <20141114131512.C1552@besplex.bde.org> In-Reply-To: <86h9y44fkp.fsf@nine.des.no> 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> <86h9y44fkp.fsf@nine.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 12 Nov 2014, [utf-8] Dag-Erling Sm=C3=B8rgrav wrote:
> Bruce Evans <brde@optusnet.com.au> writes:
>
>> On Tue, 11 Nov 2014, [utf-8] Dag-Erling Sm=C3=B8rgrav 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" parenthes=
es
>> 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 =3D=3D 0 should mean
> (foo & bar) =3D=3D 0, not foo & (bar =3D=3D 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 parenthe=
ses
for (foo & bar) =3D=3D 0.
Non-bitwise logical expressions are more interesting. Now you want =3D=3D =
to
bind tighter than ||, so that you can write foo =3D=3D 0 || bar =3D=3D 0, a=
nd
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 tha=
t
>>> 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 =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:
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)
{
=09if (b < 0)
=09=09return (EINVAL);
=09...
}
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
From owner-svn-src-all@FreeBSD.ORG Fri Nov 14 04:26:27 2014
Return-Path: <owner-svn-src-all@FreeBSD.ORG>
Delivered-To: svn-src-all@freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org
[IPv6:2001:1900:2254:206a::19:1])
(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
(No client certificate requested)
by hub.freebsd.org (Postfix) with ESMTPS id 0F0DB50A;
Fri, 14 Nov 2014 04:26:27 +0000 (UTC)
Received: from svn.freebsd.org (svn.freebsd.org
[IPv6:2001:1900:2254:2068::e6a:0])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client did not present a certificate)
by mx1.freebsd.org (Postfix) with ESMTPS id F01E19A0;
Fri, 14 Nov 2014 04:26:26 +0000 (UTC)
Received: from svn.freebsd.org ([127.0.1.70])
by svn.freebsd.org (8.14.9/8.14.9) with ESMTP id sAE4QQj2085797;
Fri, 14 Nov 2014 04:26:26 GMT (envelope-from adrian@FreeBSD.org)
Received: (from adrian@localhost)
by svn.freebsd.org (8.14.9/8.14.9/Submit) id sAE4QQYN085796;
Fri, 14 Nov 2014 04:26:26 GMT (envelope-from adrian@FreeBSD.org)
Message-Id: <201411140426.sAE4QQYN085796@svn.freebsd.org>
X-Authentication-Warning: svn.freebsd.org: adrian set sender to
adrian@FreeBSD.org using -f
From: Adrian Chadd <adrian@FreeBSD.org>
Date: Fri, 14 Nov 2014 04:26:26 +0000 (UTC)
To: src-committers@freebsd.org, svn-src-all@freebsd.org,
svn-src-head@freebsd.org
Subject: svn commit: r274493 - head/sys/dev/ath
X-SVN-Group: head
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-BeenThere: svn-src-all@freebsd.org
X-Mailman-Version: 2.1.18-1
Precedence: list
List-Id: "SVN commit messages for the entire src tree \(except for "
user" and " projects" \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/options/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all/>;
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Fri, 14 Nov 2014 04:26:27 -0000
Author: adrian
Date: Fri Nov 14 04:26:26 2014
New Revision: 274493
URL: https://svnweb.freebsd.org/changeset/base/274493
Log:
Migrate the callouts from using mutex locks to being mpsafe with
the locks being held by the callers.
Kill callout_drain() and use callout_stop().
Modified:
head/sys/dev/ath/if_ath.c
Modified: head/sys/dev/ath/if_ath.c
==============================================================================
--- head/sys/dev/ath/if_ath.c Fri Nov 14 00:25:10 2014 (r274492)
+++ head/sys/dev/ath/if_ath.c Fri Nov 14 04:26:26 2014 (r274493)
@@ -669,8 +669,8 @@ ath_attach(u_int16_t devid, struct ath_s
goto bad;
}
- callout_init_mtx(&sc->sc_cal_ch, &sc->sc_mtx, 0);
- callout_init_mtx(&sc->sc_wd_ch, &sc->sc_mtx, 0);
+ callout_init(&sc->sc_cal_ch, 1); /* MPSAFE */
+ callout_init(&sc->sc_wd_ch, 1); /* MPSAFE */
ATH_TXBUF_LOCK_INIT(sc);
@@ -1812,7 +1812,10 @@ ath_suspend(struct ath_softc *sc)
*/
ath_hal_intrset(sc->sc_ah, 0);
taskqueue_block(sc->sc_tq);
- callout_drain(&sc->sc_cal_ch);
+
+ ATH_LOCK(sc);
+ callout_stop(&sc->sc_cal_ch);
+ ATH_UNLOCK(sc);
/*
* XXX ensure sc_invalid is 1
@@ -5599,6 +5602,10 @@ ath_calibrate(void *arg)
HAL_BOOL aniCal, shortCal = AH_FALSE;
int nextcal;
+ ATH_UNLOCK_ASSERT(sc);
+
+ ATH_LOCK(sc);
+
/*
* Force the hardware awake for ANI work.
*/
@@ -5638,6 +5645,7 @@ ath_calibrate(void *arg)
taskqueue_enqueue(sc->sc_tq, &sc->sc_resettask);
callout_reset(&sc->sc_cal_ch, 1, ath_calibrate, sc);
ath_power_restore_power_state(sc);
+ ATH_UNLOCK(sc);
return;
}
/*
@@ -5713,6 +5721,7 @@ restart:
* Restore power state now that we're done.
*/
ath_power_restore_power_state(sc);
+ ATH_UNLOCK(sc);
}
static void
@@ -5888,12 +5897,17 @@ ath_newstate(struct ieee80211vap *vap, e
* Now, wake the thing up.
*/
ath_power_set_power_state(sc, HAL_PM_AWAKE);
+
+ /*
+ * And stop the calibration callout whilst we have
+ * ATH_LOCK held.
+ */
+ callout_stop(&sc->sc_cal_ch);
ATH_UNLOCK(sc);
if (ostate == IEEE80211_S_CSA && nstate == IEEE80211_S_RUN)
csa_run_transition = 1;
- callout_drain(&sc->sc_cal_ch);
ath_hal_setledstate(ah, leds[nstate]); /* set LED */
if (nstate == IEEE80211_S_SCAN) {
@@ -6084,7 +6098,6 @@ ath_newstate(struct ieee80211vap *vap, e
ATH_LOCK(sc);
ath_power_setselfgen(sc, HAL_PM_AWAKE);
ath_power_setpower(sc, HAL_PM_AWAKE);
- ATH_UNLOCK(sc);
/*
* Finally, start any timers and the task q thread
@@ -6097,6 +6110,7 @@ ath_newstate(struct ieee80211vap *vap, e
DPRINTF(sc, ATH_DEBUG_CALIBRATE,
"%s: calibration disabled\n", __func__);
}
+ ATH_UNLOCK(sc);
taskqueue_unblock(sc->sc_tq);
} else if (nstate == IEEE80211_S_INIT) {
@@ -6457,13 +6471,13 @@ ath_watchdog(void *arg)
struct ath_softc *sc = arg;
int do_reset = 0;
+ ATH_LOCK(sc);
+
if (sc->sc_wd_timer != 0 && --sc->sc_wd_timer == 0) {
struct ifnet *ifp = sc->sc_ifp;
uint32_t hangs;
- ATH_LOCK(sc);
ath_power_set_power_state(sc, HAL_PM_AWAKE);
- ATH_UNLOCK(sc);
if (ath_hal_gethangstate(sc->sc_ah, 0xffff, &hangs) &&
hangs != 0) {
@@ -6475,9 +6489,7 @@ ath_watchdog(void *arg)
if_inc_counter(ifp, IFCOUNTER_OERRORS, 1);
sc->sc_stats.ast_watchdog++;
- ATH_LOCK(sc);
ath_power_restore_power_state(sc);
- ATH_UNLOCK(sc);
}
/*
@@ -6491,6 +6503,8 @@ ath_watchdog(void *arg)
}
callout_schedule(&sc->sc_wd_ch, hz);
+
+ ATH_UNLOCK(sc);
}
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141114131512.C1552>