svn commit: r212886 - head/sbin/growfs

[Bruce Evans]

On Mon, 20 Sep 2010, Marcel Moolenaar wrote:

> On Sep 19, 2010, at 11:55 PM, Bruce Evans wrote:
>
>> On Mon, 20 Sep 2010, Marcel Moolenaar wrote:
>>
>>> Log:
>>> Unbreak the build on strong-aligned architectures (arm, ia64).
>>> Casting from (char *) to (struct ufs1_dinode *) changes the
>>> alignment requirement of the pointer and GCC does not know that
>>> the pointer is adequately aligned (due to malloc(3)), and warns
>>> about it. Cast to (void *) first to by-pass the check.
>>
>> A previous version in which the pointer (iobuf) the correct type to
>> begin with was better.  It used to have type void *, but now has type
>> caddr_t (whose existence is a bug), in order to abuse caddr_t by
>> assuming that it is char * to do pointer arithmetic on it in 1 place
>> (iobuf + sblock.fs_cgsize).  The 7 other places where iobuf is used
>> only assume that caddr_t is a pointer (or perhaps a [u]intptr_t).
>>
>> growfs has no other instances of caddr_t.
>
> My first instinct was to retype iobuf as (void *), but saw
> the pointer arithmetic and decided against it. If we had
> a macro that one can use to add X number of bytes to a pointer
> to any, I would have used that. Other macros to add X number
> of short, int, long, could be handy too, but that may not be
> as useful as for bytes.
>
> Would it be useful to add a macro like (don't mind the name):
>
> #define ADDBYTESTOPOINTER(ptr, cnt)	\
> 	((__typeof(ptr))(void *)((uintptr_t)(void *)(ptr) + (cnt)))

As you may know, I like such macros in system headers as much as I
like __DECONST() -- they are good enough for lining /dev/null but only
after suitable heating :-).  They are unportable and make it too easy
to use wrong types.  They are OK in individual applications, but most
applications should have few instances of pointer frobbing so they
don't need macros for it except to highlight its bogusness.

The above implementation has the technical error of casting away any
qualifiers of (ptr).

__DECONST() is OK in that respect (it is good for it to be an error
if it removes more than a const qualifier, but it has the technical
error of not casting uintptr_t to (void *).  If you don't bother with
the cast to (void *), then you could also not bother with the cast to
(const void *), except the latter has the advantage of detecting removal
of more than a const qualifier -- perhaps that was intentional.  Both
depend on the bug that -Wcast-qual doesn't work for casting to uintptr_t.

I think I prefer casting to (char *) than to uintptr_t for adding byte
offsets to pointers.  These give different problems with casting away
qualifiers, as mentioned above.  Perhaps the best way is application-
dependent.  But portable applications cannot use uintptr_t for this
except in negatively useful ifdefs, since uintptr_t might not exist.

IMO, the correct fix for fixing -Wcast-qual errors is to not use
-Wcast-qual.  The Standard method for removing type qualifiers is to
explicitly cast them away, and -Wcast-qual breaks this.  But bogus
casts are still too common, so first you have to find them all.
There should be a -Wbogus-cast option to help find them.  This is a
DWIM option so I don't know how to implement it :-).  It could start
by warning about casts that are null on all target, then warn about
casts that are null on the current target.  There should remain a
cast-qual option that only applies to implicit casts (for assignments
and function calls).

Bruce