Closed Bug 1152845 Opened 5 years ago Closed 4 years ago

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

Categories

(Toolkit :: Storage, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox40 --- affected

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Fixed upstream in https://www.sqlite.org/src/info/83b342a44ffc9ea0 ])

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.
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 };
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.
(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.
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.
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.)
Whiteboard: [Fixed upstream in https://www.sqlite.org/src/info/83b342a44ffc9ea0 ]
(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
Great, sounds like things are working properly then.
We don't care about warnings in third-party code.
Status: NEW → RESOLVED
Closed: 4 years ago
Depends on: 1216020
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.