Re: BUG #5590: undefined shift behavior

I wrote:
> "John Regehr" <regehr(at)cs(dot)utah(dot)edu> writes:
>> During a "make check" the left-shift operator at tsquery_util.c 48:18 is
>> passed a negative right-hand argument a number of times.

> Hmm. valcrc is declared as signed int32, so depending on what your
> compiler thinks the semantics of % is, this clearly can potentially
> happen. I notice the same problem in makeTSQuerySign() in tsquery_op.c.

On further investigation, the reason makeTSQuerySign didn't show up in
John's test is that it's coded like this:

sign |= ((TSQuerySign) 1) << (ptr->qoperand.valcrc % TSQS_SIGLEN);

where TSQS_SIGLEN is a macro that involves sizeof(TSQuerySign), and
therefore will produce an unsigned result (on most machines anyway).
So the % is done in unsigned arithmetic and there's no problem.
I think it'd still be a good idea to cast valcrc to unsigned int
explicitly here, but this is just for future-proofing not because
there's a real bug ATM. Which is a good thing, because TSQuerySign
does go to disk so we'd have a backwards-compatibility mess if we
had to change this computation.

The problem in QT2QTN() is real, on the other hand, because it's coded
as

node->sign = 1 << (in->qoperand.valcrc % 32);

so the modulo is signed. Some investigation shows that Intel machines
seem to give the right answer anyway, which has masked the problem for
most people. But I find that on PPC, the result of the shift is *zero*
if valcrc is negative --- haven't checked, but I wonder if that hardware
interprets a negative left shift as a right shift. So on PPC the "sign"
comes out as zero about half the time.

As best I can tell at the moment, this only results in some inefficiency
(because the Bloom filters don't work as intended) in some not-too-
heavily-used operations; and the QTNode structures aren't written to
disk so there's no compatibility issue from fixing the computation.

To sum up: we should insert these casts in HEAD but at the moment I
don't see a strong reason to back-patch, nor any indication that we'd
be creating a need for a forced initdb.

regards, tom lane