svn commit: r209193 - head/sys/dev/sound/pcm

[Bruce Evans]

On Tue, 15 Jun 2010, Andriy Gapon wrote:

> on 15/06/2010 11:03 Kostik Belousov said the following:
>> On Tue, Jun 15, 2010 at 07:06:54AM +0000, Andriy Gapon wrote:
>>> Log:
>>>   sound/pcm: use non-const string as a value with SYSCTL_STRING
>>>
>>>   Although the sysctls are marked with CTLFLAG_RD and the values will stay
>>>   immutable, current sysctl implementation stores value pointer in
>>>   void* type, which means that const qualifier is discarded anyway
>>>   and some newer compilers complaint about that.
>>>   We can't use de-const trick in sysctl implementation, because in that
>>>   case we could miss an opposite situation where a const value is used
>>>   with CTLFLAG_RW sysctl.

__DECONST() and __DEVOLATILE can almost never be used, because they just
hide bugs like the above or API bugs in most cases, and I would complain
about their use in all cases.

>>>
>>>   Complaint from:	gcc 4.4, clang
>>>   MFC after:	2 weeks
>> This is arguably the change for worse then better.
>
> Arguably - yes, practically - I am not sure.
> See almost every other instance of SYSCTL_STRING usage, kern_mib.c most prominently.
>
>> You could add SYSCTL_STRING_CONST or the like instead.
>
> But we already have CTLFLAG_RD vs CTLFLAG_RW...
> Perhaps, we could have a union of void* and const void* and then assign value to
> the appropriate member based on the flags.  But I am not sure if the benefit
> would be worth the effort.

More importantly the "void *" causes many type mismatches to be missed.
E.g., SYSCTL_INT() should only accept variables of type int, but the
variable type decays to "void *" almost before it can be checked, and
SYSCTL_INT() depends on this decay (for the assignment).

I couldn't see a good way to fix this.  A bad way might be to use an
enormous union with all supported types and type qualifiers.
SYSCTL_INT() can then assign to the unqualified int type.  But this
only fixes the easier cases where the macro is used.  Many cases use
a SYSCTL_PROC() with a sysctl_handle_foo() call.  All these calls take
args of SYSCTL_HANDLER_ARGS.  This gives the same types for all the
calls and one of the types is "void *" which defeats type safety in
the same way as the assignment to the "void *" in the macros.  We now
have flags for some unsigned types (CTLTYPE_UQUAD is still missing),
but we don't have sysctl_handle_uint() and depend on this decay for
for sysctl_handle_int() to work on u_ints.  But this type punning is
a feature.  At least the macro version can do a static type check
by doing an assignment without a conversion, and then assign to a
"void *" or a slightly more specialized type as now, to avoid
replicating the handler functions.

Bruce