Date: Fri, 24 Sep 2010 11:26:41 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Marcel Moolenaar <xcllnt@mac.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Marcel Moolenaar <marcel@freebsd.org>, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r212886 - head/sbin/growfs Message-ID: <20100924105219.J780@delplex.bde.org> In-Reply-To: <11C42F9D-7201-4804-8923-430F28E58C57@mac.com> References: <201009200420.o8K4KtKn026514@svn.freebsd.org> <20100920163758.A788@besplex.bde.org> <11C42F9D-7201-4804-8923-430F28E58C57@mac.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100924105219.J780>