Re: databases/sqlite on powerpc

[Taylor R Campbell <campbell+netbsd-tech-pkg%mumbe.net@localhost>]

> Date: Fri, 24 Jun 2022 07:47:35 +0000
> From: David Holland <dholland-pkgtech%netbsd.org@localhost>
> 
> On Thu, Jun 23, 2022 at 08:24:39PM -0400, Greg Troxel wrote:
>  > > Nope, it's UB in the source :-(
> 
> Joerg asked on IRC if it's undefined or implementation-defined; it
> might be implementation-defined,

C99/C11 6.2.6 `Representation of Types' gives constraints for how
signed integer types are represented by strings of bits, which are
what the bitwise operators operate on.

It is possible for the behaviour of bitwise operations on signed
integers to be undefined -- e.g., on a machine with a sign/magnitude
representation in which a bit string that would mean `negative zero'
is a trap representation.

But all of that is academic, because:

>                                  in which case unless powerpc has
> recently developed some very unusual integer properties it's gcc's
> bug.

It may not be a gcc bug; the code has undefined behaviour which gcc
may be exploiting here for optimization.

On powerpc, char is unsigned, so the code

    char *stddt;
    int hash;
    ...
    hash = 0;
    for(j=0; stddt[j]; j++){
      hash = hash*53 + stddt[j];
    }

computes sums and products of nonnegative quantities in (signed) int
arithmetic, in which overflow is undefined behaviour.  gcc may deduce
that the value of hash must lie in the interval [0, INT_MAX] = [0,
0x7fffffff], under which premise the expression `hash & 0x7fffffff'
can be correctly replaced by `hash'.

> Well, yes, except that for code that one cares about the proper
> response is an audit. In this case since it's sqlite2 and properly
> nothing and nobody should still be using it, that's not really
> worthwhile and it's reasonable to apply duct tape.

The variable `hash' was converted to unsigned in sqlite3's lemon in
2013 to avoid signed arithmetic overflow:

https://sqlite.org/src/info/8d399a03de63c159

In other words, he@'s patch is correct and was applied by upstream a
decade ago -- except as `unsigned hash' instead of `unsigned int
hash', and with several other related fixes that we should probably
apply too if anyone cares about building sqlite.