Re: clang -fsanitize=undefined error in ecpg

Peter Eisentraut <peter_e(at)gmx(dot)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 input
select '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 turned into 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