Date: Fri, 17 Jul 2015 17:08:27 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pedro Giffuni <pfg@freebsd.org> Cc: Ian Lepore <ian@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r285644 - head/contrib/sqlite3 Message-ID: <20150717162031.C901@besplex.bde.org> In-Reply-To: <55A830E4.6090408@FreeBSD.org> References: <201507162207.t6GM7ECT009955@repo.freebsd.org> <1437085366.1334.367.camel@freebsd.org> <55A830E4.6090408@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 16 Jul 2015, Pedro Giffuni wrote: > On 07/16/15 17:22, Ian Lepore wrote: >> On Thu, 2015-07-16 at 22:07 +0000, Pedro F. Giffuni wrote: >>> Author: pfg >>> Date: Thu Jul 16 22:07:13 2015 >>> New Revision: 285644 >>> URL: https://svnweb.freebsd.org/changeset/base/285644 >>> >>> Log: >>> sqlite: clean a couple of invocations of memcpy(3) >>> Found almost accidentally by our native gcc when enhanced with >>> FORTIFY_SOURCE. >>> Submitted by: Oliver Pinter >>> Sponosored by: Google Inc. GSoC 2015 >>> >>> Modified: >>> head/contrib/sqlite3/sqlite3.c >>> >>> Modified: head/contrib/sqlite3/sqlite3.c >>> ============================================================================== >>> --- head/contrib/sqlite3/sqlite3.c Thu Jul 16 19:40:18 2015 >>> (r285643) >>> +++ head/contrib/sqlite3/sqlite3.c Thu Jul 16 22:07:13 2015 >>> (r285644) >>> @@ -49487,9 +49487,9 @@ static void walIndexWriteHdr(Wal *pWal){ >>> pWal->hdr.isInit = 1; >>> pWal->hdr.iVersion = WALINDEX_MAX_VERSION; >>> walChecksumBytes(1, (u8*)&pWal->hdr, nCksum, 0, pWal->hdr.aCksum); >>> - memcpy((void *)&aHdr[1], (void *)&pWal->hdr, sizeof(WalIndexHdr)); >>> + memcpy((void *)&aHdr[1], (const void *)&pWal->hdr, >>> sizeof(WalIndexHdr)); >>> walShmBarrier(pWal); >>> - memcpy((void *)&aHdr[0], (void *)&pWal->hdr, sizeof(WalIndexHdr)); >>> + memcpy((void *)&aHdr[0], (const void *)&pWal->hdr, >>> sizeof(WalIndexHdr)); >>> } >>> /* >>> >> Setting aside any "unnecessary divergence with upstream" questions for >> the moment, wouldn't the correct fix be to just remove the casting >> completely? Indeed. (Remove all the bogus casts, not just the changed ones.) > The compiler should know, but it is not wrong to have the casts > match the (standard) implementation. I couldn't say which is > "more" correct. It is not even wrong. The casts to void * are only needed to support K&R compilers (ones that aren't actually K&R 1, so that they support void *), or if the program is so buggy as to not make a prototype for memcpy() visible. I think the program doesn't attempt to support K&R. I checked that it includes string.h. The hdr variable is not const, so casting it to void * is not even wrong. If it were const, then the cast to void * still wouldn't be wrong (it would still conform to C, and the prototype would undo it). However, it would remove the const type qualifier, and some compilers warn about this, so it would be an error at a higher level. If the code supported K&R, then your change would be correct for the following reasons: - K&R needs the void * part of the casts - K&R doesn't support const (except it is further from actually being K&R 1 so that it supports both const and void), so const void * looks like a syntax error. However, the support for K&R in <sys/cdefs.h> kills this const so that the K&R case works. - the full cast should be used. Otherwise the cast looks more bogus since it is only half of not even wrong for the non-K&R case. Modern programs have many dependencies on prototypes actually working. E.g., sqlite.c has a measly 257 calls to memcpy(). It bogusly casts to void * in a whole 8 of these. Often the args are already void *, so they don't need need a bogus (null) cast to make them that, but the second arg does need a bogus (non-null) cast to const void * to make it that before the prototype does. Casts that have no effect shouldn't be used since they bloat the source code and obfuscate the casts that are actually needed. Exception: it is good to use explicit downcasts and not depend on the design error that prototypes downcast without warning. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150717162031.C901>