Date: Sat, 18 Jul 2015 16:26:17 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pedro Giffuni <pfg@freebsd.org> Cc: Peter Jeremy <peter@rulingia.com>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r285644 - head/contrib/sqlite3 Message-ID: <20150718155507.J876@besplex.bde.org> In-Reply-To: <55A98763.1030705@FreeBSD.org> References: <201507162207.t6GM7ECT009955@repo.freebsd.org> <20150717222631.GD36150@server.rulingia.com> <55A98763.1030705@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 17 Jul 2015, Pedro Giffuni wrote: > On 07/17/15 17:26, Peter Jeremy wrote: >> On 2015-Jul-16 22:07:14 +0000, "Pedro F. Giffuni" <pfg@FreeBSD.org> wrote: >>> Log: >> ... >>> sqlite: clean a couple of invocations of memcpy(3) >>> Found almost accidentally by our native gcc when enhanced with >>> FORTIFY_SOURCE. >> ... >>> - 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)); >> If the compiler complained about that, the compiler is broken. > > The change was rather collateral (read cosmetical) to the real warning > which remains unfixed. > > The compiler warning is this: > ... > ===> lib/libsqlite3 (obj,depend,all,install) > cc1: warnings being treated as errors > /scratch/tmp/pfg/head/lib/libsqlite3/../../contrib/sqlite3/sqlite3.c: In > function 'walIndexWriteHdr': > /scratch/tmp/pfg/head/lib/libsqlite3/../../contrib/sqlite3/sqlite3.c:49490: > warning: passing argument 1 of 'memcpy' discards qualifiers from pointer > target type > /scratch/tmp/pfg/head/lib/libsqlite3/../../contrib/sqlite3/sqlite3.c:49492: > warning: passing argument 1 of 'memcpy' discards qualifiers from pointer > target type > --- sqlite3.So --- > *** [sqlite3.So] Error code 1 > ... > > make[6]: stopped in /scratch/tmp/pfg/head/lib/libsqlite3 > > It only happens when using the experimental FORTIFY_SOURCE support [1], > and it only happens with gcc (the base one is the only tested). > > Yes, I am suspecting a compiler bug but gcc has been really useful to find > bugs in fortify_source. gcc is correct. This has nothing to do with 'const'. aHdr has a volatile qualifier. >> 'const' >> is a promise to the caller that the given parameter will not be modified >> by the callee. There's no requirement that the passed argument be const. >> As bde commented, the casts are all spurious. There is a requirement that the parameter doesn't point to volatile storage even if it has a volatile qualifier. memcpy() doesn't support volatile storage. This allows it to be more efficient by possibly doing the stores out of order or more than once. Someone broke the warnings about the prototype discarding the volatile qualifier by adding casts to discard the qualifier explicitly. This is the correct way to turn volatile storage into nonvolatile storage iff the storage is actually nonvolatile for the lifetime of the cast. It is unclear how the storage becomes transiently non-volatile here. But casting away qualifiers is usually an error, so we ask the compiler to warn about it using -Wcast-qual. -Wcast-qual is broken in clang, so bugs found by it show up more after switching to gcc. clang has ways to enable this warning, but the standard way doesn't work. Casting away volatile is more likely to be just a bug than casting away const, so it is quite reasonable for the compiler to warn about it unconditionally or at least under -Wall. Almost all of the casts of memcpy()'s args in sqlite3.c are for or near the casts of the volatile foo *aHdr. The old ones are all to void *. This kills all qualifiers, but there are only volatile qualifiers in sight. Someone also sprinkled them on the nearby second arg of &hdr. This seems to be not even wrong (there are no qualifiers in sight for hdr). Sometimes aHdr is the second arg of memcpy(). This cannot be volatile either, so volatile is cast away using void *. The complete set of conversions for this is: aHdr is volatile foo * for some reason the cast changes it to void * the prototype changes it to const void *. The best way to express the magic removal of volatile probably to cast to foo *, not to void *. Let the prototype do all of the non-magic conversions. Casting to void * does 1 magic conversion but only half of the non-magic conversions that would be done by the prototype (the other one is adding a const qualifier). Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150718155507.J876>