svn commit: r285644 - head/contrib/sqlite3

[Bruce Evans]

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 at 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