1152845 - sqlite.c triggers -Wtautological-pointer-compare build warnings for useless null-checks SrcList::a (an array)

Status: RESOLVED WONTFIX

(Toolkit :: Storage, defect)

Description

[Daniel Holbert [:dholbert]]

I get these build warnings, when building with clang 3.6:

FIRST WARNING
=============
Warning: -Wpointer-bool-conversion in $SRCdb/sqlite3/src/sqlite3.c: address of array 'p->a' will always evaluate to 'true'
$SRCdb/sqlite3/src/sqlite3.c:93517:16: warning: address of array 'p->a' will always evaluate to 'true' [-Wpointer-bool-conversion]
    assert( p->a || p->nSrc==0 );
            ~~~^ ~~
/usr/include/assert.h:89:5: note: expanded from macro 'assert'
  ((expr)                                                               \
    ^
}

Longer snippet of source code:
> 93514 SQLITE_PRIVATE void sqlite3SrcListShiftJoinType(SrcList *p){
> 93515   if( p ){
> 93516     int i;
> 93517     assert( p->a || p->nSrc==0 );


SECOND WARNING
=============
Warning: -Wtautological-pointer-compare in $SRCdb/sqlite3/src/sqlite3.c: comparison of array 'pSrc->a' not equal to a null pointer is always true
$SRCdb/sqlite3/src/sqlite3.c:111766:19: warning: comparison of array 'pSrc->a' not equal to a null pointer is always true [-Wtautological-pointer-compare]
    assert( pSrc->a!=0 );
            ~~~~~~^  ~
/usr/include/assert.h:89:5: note: expanded from macro 'assert'
  ((expr)                                                               \
    ^

Longer snippet of source code:
> 111761   SrcList *pSrc;       /* SrcList to be returned */
> [...]
> 111766     assert( pSrc->a!=0 );


In both cases, we're null-checking a SrcList's "a" member-variable, which is an array.  An array can't be null (because it brings its own storage; it's not an arbitrary pointer). Hence, clang spamming these warnings.

Not sure if the assertions can just go away, or if they're trying to assert something subtly different & should be tweaked.  I don't know how we report bugs upstream to sqlite, but I'm starting this out as a Toolkit|Storage bug for now.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

http://www.sqlite.org/src/wiki?name=Bug+Reports

Comment 2

[Daniel Holbert [:dholbert]]

Here's the SrcList struct definition, to show that "a" is indeed an array:
> 11946 struct SrcList {
> 11947   int nSrc;        /* Number of tables or subqueries in the FROM clause */
> 11948   u32 nAlloc;      /* Number of entries allocated in a[] below */
> 11949   struct SrcList_item {
> 11950     Schema *pSchema;  /* Schema to which this item is fixed */
[...]
> 11972     Index *pIndex;    /* Index structure corresponding to zIndex, if any */
> 11973   } a[1];             /* One entry for each identifier on the list */
> 11974 };

Comment 3

[Daniel Holbert [:dholbert]]

Thanks, Ryan! I don't see anything like this bug on the "all tickets" view, so I'll go ahead and email the sqlite-users list as suggested there.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Thanks, Ryan! I don't see anything like this bug on the "all tickets" view,
> so I'll go ahead and email the sqlite-users list as suggested there.

that's the right thing to do, otherwise we can also mail support, but I think it's not needed in this case.

Comment 5

[D. Richard Hipp]

Fixed at https://www.sqlite.org/src/info/83b342a44ffc9ea0

Comment 6

[D. Richard Hipp]

BTW, assert() statements in SQLite ought to be disabled for release builds (by omitting the -DSQLITE_DEBUG option) since there are thousands of assert()s in SQLite, many in inner loops, and the code runs about 2x or 3x faster with the asserts turned off.

Comment 7

[Daniel Holbert [:dholbert]]

Well, that was fast! Thanks! :)

I'll leave this bug open for now, since we're still getting these warnings in mozilla-central; presumably this will be fixed by our next sqlite update.

(In reply to D. Richard Hipp from comment #6)
> BTW, assert() statements in SQLite ought to be disabled for release builds

(Good to know. In my case, this was a debug build. Not sure if we have these enabled for release builds currently, but I hope/suspect we don't.)

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> (Good to know. In my case, this was a debug build. Not sure if we have these
> enabled for release builds currently, but I hope/suspect we don't.)

https://dxr.mozilla.org/mozilla-central/source/db/sqlite3/src/moz.build#55

# Turn on SQLite's assertions in debug builds.
if CONFIG['MOZ_DEBUG']:
    DEFINES['SQLITE_DEBUG'] = 1

Comment 9

[Daniel Holbert [:dholbert]]

Great, sounds like things are working properly then.

Comment 10

[Chris Peterson [:cpeterson]]

We don't care about warnings in third-party code.