Re: clang -fsanitize=undefined error in ecpg

Peter Eisentraut <peter_e@gmx.net> writes:
> With clang -fsanitize=undefined (clang-3.4), I get the following test failure in ecpg
> (it's the only one in the entire tree):

Hm.  I don't know why you can't reproduce that in the backend, because
when stepping through DecodeDateTime() on the inputselect '19990108foobar'::timestamptz;
I definitely see it shifting 1 left 31 places:

Breakpoint 2, DecodeDateTime (field=<value optimized out>,    ftype=<value optimized out>, nf=2, dtype=0x7ffff6fec168,
 tm=0x7ffff6fec170, fsec=0x7ffff6fec1b4, tzp=0x7ffff6fec16c)   at datetime.c:1193
 
1193                                    type = DecodeTimezoneAbbrev(i, field[i], &val, &valtz);
(gdb) n
1194                                    if (type == UNKNOWN_FIELD)
(gdb) p type
$1 = 31
(gdb) s
1195                                            type = DecodeSpecial(i, field[i], &val);
(gdb) s
DecodeSpecial (field=1, lowtoken=0x7ffff6febf89 "foobar", val=0x7ffff6febf28)   at datetime.c:3018
3018    {
(gdb) fin
Run till exit from #0  DecodeSpecial (field=1,    lowtoken=0x7ffff6febf89 "foobar", val=0x7ffff6febf28) at
datetime.c:3031
DecodeDateTime (field=<value optimized out>, ftype=<value optimized out>,    nf=2, dtype=0x7ffff6fec168,
tm=0x7ffff6fec170,fsec=0x7ffff6fec1b4,    tzp=0x7ffff6fec16c) at datetime.c:1196
 
1196                                    if (type == IGNORE_DTF)
Value returned is $2 = 31
(gdb) s
1199                                    tmask = DTK_M(type);
(gdb) p type
$3 = 31
(gdb) s
1200                                    switch (type)
(gdb) p tmask
$4 = -2147483648

> This patch fixes it:

> -#define DTK_M(t)               (0x01 << (t))
> +#define DTK_M(t)               ((t) == UNKNOWN_FIELD ? 0 : 0x01 << (t))

Don't like that even a little bit.  The intent of the code is perfectly
clear, cf this comment in datetime.h:
* Field types for time decoding.** Can't have more of these than there are bits in an unsigned int* since these are
turnedinto bit masks during parsing and decoding.
 

So I think the correct fix is

-#define DTK_M(t)               (0x01 << (t))
+#define DTK_M(t)               (0x01U << (t))

It looks to me like it doesn't actually matter at the moment, because
anyplace where we apply DTK_M to a value that could be UNKNOWN_FIELD,
we'll immediately after that either return an error or replace the
tmask value with something else.  So the lack of portability of this
construction hasn't mattered.  But we should fix it in a way that won't
create time bombs for future code changes, and producing a zero mask
from a valid field type code would be a time bomb.
        regards, tom lane