Обсуждение: BUG #5592: list of integer undefined behaviors
The following bug has been logged online: Bug reference: 5592 Logged by: John Regehr Email address: regehr@cs.utah.edu PostgreSQL version: head 8/1/10 Operating system: OSX Description: list of integer undefined behaviors Details: Below: a list of integer undefined behaviors that occur when running "make check" on yesterday's postgresql on an x86-64 Mac Mini. Here we're using the ANSI C rules for overflow. Of course many/most of these errors are not errors when the -fwrapv rules are in effect. The last error in the list I already reported, just leaving it in for completeness. If more details are needed please let me know. John Regehr <bitmapset.c, (752:8)> : Op: -, Reason : Signed Subtraction Overflow, UNARY OPERATION: right (int32): -2147483648 <int.c, (1002:16)> : Op: -, Reason : Signed Subtraction Overflow, BINARY OPERATION: left (int32): -2147483647 right (int32): 2 <int.c, (1023:16)> : Op: *, Reason : Signed Multiplication Overflow, BINARY OPERATION: left (int32): 2147483647 right (int32): 2 <int.c, (641:16)> : Op: +, Reason : Signed Addition Overflow, BINARY OPERATION: left (int32): 2147483647 right (int32): 2 <int.c, (662:16)> : Op: -, Reason : Signed Subtraction Overflow, BINARY OPERATION: left (int32): -2147483647 right (int32): 2 <int.c, (695:16)> : Op: *, Reason : Signed Multiplication Overflow, BINARY OPERATION: left (int32): 2147483647 right (int32): 2 <int.c, (981:16)> : Op: +, Reason : Signed Addition Overflow, BINARY OPERATION: left (int32): 2147483647 right (int32): 2 <int8.c, (1028:16)> : Op: +, Reason : Signed Addition Overflow, BINARY OPERATION: left (int64): 100 right (int64): 9223372036854775800 <int8.c, (1049:16)> : Op: -, Reason : Signed Subtraction Overflow, BINARY OPERATION: left (int64): -100 right (int64): 9223372036854775800 <int8.c, (104:23)> : Op: *, Reason : Signed Multiplication Overflow, BINARY OPERATION: left (int64): 3908203590239580293 right (int64): 10 <int8.c, (104:28)> : Op: +, Reason : Signed Addition Overflow, BINARY OPERATION: left (int64): 9223372036854775800 right (int64): 9 <int8.c, (1070:16)> : Op: *, Reason : Signed Multiplication Overflow, BINARY OPERATION: left (int64): 100 right (int64): 9223372036854775800 <int8.c, (497:11)> : Op: -, Reason : Signed Subtraction Overflow, UNARY OPERATION: right (int64): -9223372036854775808 <int8.c, (521:16)> : Op: +, Reason : Signed Addition Overflow, BINARY OPERATION: left (int64): 9223372036854775800 right (int64): 9223372036854775800 <int8.c, (542:16)> : Op: -, Reason : Signed Subtraction Overflow, BINARY OPERATION: left (int64): 9223372036854775800 right (int64): -9223372036854775800 <int8.c, (563:16)> : Op: *, Reason : Signed Multiplication Overflow, BINARY OPERATION: left (int64): 4567890123456789 right (int64): 4567890123456789 <int8.c, (623:24)> : Op: -, Reason : Signed Subtraction Overflow, UNARY OPERATION: right (int64): -9223372036854775808 <int8.c, (748:16)> : Op: +, Reason : Signed Addition Overflow, BINARY OPERATION: left (int64): 9223372036854775800 right (int64): 100 <int8.c, (769:16)> : Op: -, Reason : Signed Subtraction Overflow, BINARY OPERATION: left (int64): -9223372036854775800 right (int64): 100 <int8.c, (790:16)> : Op: *, Reason : Signed Multiplication Overflow, BINARY OPERATION: left (int64): 9223372036854775800 right (int64): 100 <int8.c, (844:16)> : Op: +, Reason : Signed Addition Overflow, BINARY OPERATION: left (int64): 100 right (int64): 9223372036854775800 <int8.c, (865:16)> : Op: -, Reason : Signed Subtraction Overflow, BINARY OPERATION: left (int64): -100 right (int64): 9223372036854775800 <int8.c, (886:16)> : Op: *, Reason : Signed Multiplication Overflow, BINARY OPERATION: left (int64): 100 right (int64): 9223372036854775800 <int8.c, (932:16)> : Op: +, Reason : Signed Addition Overflow, BINARY OPERATION: left (int64): 9223372036854775800 right (int64): 100 <int8.c, (953:16)> : Op: -, Reason : Signed Subtraction Overflow, BINARY OPERATION: left (int64): -9223372036854775800 right (int64): 100 <int8.c, (974:16)> : Op: *, Reason : Signed Multiplication Overflow, BINARY OPERATION: left (int64): 9223372036854775800 right (int64): 100 <nabstime.c, (1193:21)> : Op: -, Reason : Signed Subtraction Overflow, BINARY OPERATION: left (int32): 2147483644 right (int32): -2147483648 <nabstime.c, (1194:21)> : Op: -, Reason : Signed Subtraction Overflow, BINARY OPERATION: left (int32): 2147483644 right (int32): -2147483648 <tsquery_util.c, (48:18)> : Op: <<, Reason : Signed Left Shift Error: Right operand is negative or is greater than or equal to the width of the promoted left operand, BINARY OPERATION: left (int32): 1 right (int32): -25
On Mon, Aug 2, 2010 at 7:16 PM, John Regehr <regehr@cs.utah.edu> wrote: > <nabstime.c, (1193:21)> : Op: -, Reason : Signed Subtraction Overflow, > BINARY OPERATION: left (int32): 2147483644 right (int32): -2147483648 > > <nabstime.c, (1194:21)> : Op: -, Reason : Signed Subtraction Overflow, > BINARY OPERATION: left (int32): 2147483644 right (int32): -2147483648 > These seem to imply that tinterval can contain a start point greater than its end point. I'm not familiar with the rep invariant of tinterval well enough to know if that's true or an indication of a bug elsewhere. I suppose it doesn't matter for cmp since it's still assigning an arbitrary position in the range to the interval. -- greg
Greg Stark <gsstark@mit.edu> writes:
> On Mon, Aug 2, 2010 at 7:16 PM, John Regehr <regehr@cs.utah.edu> wrote:
>> <nabstime.c, (1193:21)> : Op: -, Reason : Signed Subtraction Overflow,
>> BINARY OPERATION: left (int32): 2147483644 right (int32): -2147483648
>>
>> <nabstime.c, (1194:21)> : Op: -, Reason : Signed Subtraction Overflow,
>> BINARY OPERATION: left (int32): 2147483644 right (int32): -2147483648
> These seem to imply that tinterval can contain a start point greater
> than its end point.
Just to follow up: all the other ones seem to be non-problems.
The one in bitmapset.c is an intentional trick (see the comment for
the RIGHTMOST_ONE macro), and all the ones in int.c and int8.c have
associated overflow checks. I'm actually a bit surprised that the
regression tests seem to exercise all of those overflow checks ;-)
Like Greg, I'm not sure about the tinterval_cmp_internal cases.
tinterval is pretty much a dead legacy datatype anyway, but probably we
oughta fix it if there are failures showing up in the regression tests.
regards, tom lane
Greg Stark <gsstark@mit.edu> writes:
> On Mon, Aug 2, 2010 at 7:16 PM, John Regehr <regehr@cs.utah.edu> wrote:
>> <nabstime.c, (1193:21)> : Op: -, Reason : Signed Subtraction Overflow,
>> BINARY OPERATION: left (int32): 2147483644 right (int32): -2147483648
>>
>> <nabstime.c, (1194:21)> : Op: -, Reason : Signed Subtraction Overflow,
>> BINARY OPERATION: left (int32): 2147483644 right (int32): -2147483648
> These seem to imply that tinterval can contain a start point greater
> than its end point.
On closer investigation, the problem is just that you can't fit the
difference INT_MAX minus INT_MIN into an int :-(.
The particular values cited appear to be NOEND_ABSTIME and
NOSTART_ABSTIME, ie "infinity" and "-infinity" in the abstime datatype,
so this is coming from tinterval comparisons involving this value
in the regression tests:
INSERT INTO TINTERVAL_TBL (f1)
VALUES ('["-infinity" "infinity"]');
Although this is the worst case, you could easily get overflows from
intervals with ordinary endpoints that are sufficiently far apart.
Since this is a nearly-dead legacy datatype, I can't get excited about
spending a lot of time on it. What I suggest we do is do the difference
calculation in int64 arithmetic instead of int32.
Another complaint that could be leveled against this code is that
infinity and minus infinity behave way too much like ordinary mortal
timestamps. It would be very easy to select finite timestamps
a, b, c such that this code thinks [a b] is a longer interval than
[c infinity], which is surprising to say the least. However, I can't
get excited about fixing that for abstime/tinterval. It might be
something to consider if anyone ever reimplements this type in a way
that isn't going to fall over in 2038.
regards, tom lane
On Tue, Aug 3, 2010 at 3:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Although this is the worst case, you could easily get overflows from > intervals with ordinary endpoints that are sufficiently far apart. Oh, duh, this is pretty obvious in retrospect. > Since this is a nearly-dead legacy datatype, I can't get excited about > spending a lot of time on it. =A0What I suggest we do is do the difference > calculation in int64 arithmetic instead of int32. At some level this is all not really a problem. It just means that the arbitrary ordering of tintervals isn't the obvious ordering. If we change it it would invalidate any indexes on tintervals so it can't be backpatched. I'm not sure whether it makes sense to bother having a slightly more sensible ordering in 9.0+ if it's still going to be wacky in older versions. Also, does it cause any problem that this comparison function will treat large swathes of tintervals as equal? Any tinterval with the same length will compare equal according to this. I suppose it doesn't have the same problem as strings as long as =3D is defined compatibly. --=20 greg
Greg Stark <gsstark@mit.edu> writes:
> On Tue, Aug 3, 2010 at 3:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Since this is a nearly-dead legacy datatype, I can't get excited about
>> spending a lot of time on it. What I suggest we do is do the difference
>> calculation in int64 arithmetic instead of int32.
> At some level this is all not really a problem. It just means that the
> arbitrary ordering of tintervals isn't the obvious ordering. If we
> change it it would invalidate any indexes on tintervals so it can't be
> backpatched. I'm not sure whether it makes sense to bother having a
> slightly more sensible ordering in 9.0+ if it's still going to be
> wacky in older versions.
There is that. Given the lack of complaints from the field, maybe we
should leave it alone, except maybe for adding some more comments to
tinterval_cmp_internal.
> Also, does it cause any problem that this comparison function will
> treat large swathes of tintervals as equal?
Since we don't have hashing support for tinterval, there's nothing
inconsistent about the behavior. Whether it is *useful* is a whole
different discussion.
regards, tom lane
John Regehr <regehr@cs.utah.edu> writes:
>> Just to follow up: all the other ones seem to be non-problems.
> Would you folks be willing to specify which arithmetic operations are
> considered to be safe in the case of overflow? Something simple like an
> "INTEGER_OVERFLOW_OK" comment at the end of the line of code containing
> the operation would suffice. This would let me automatically filter out
> error messages on these lines of code in the future.
That might be doable for individual operations, but I don't think that
(for example) having to label all the users of RIGHTMOST_ONE() would be
very maintainable. Is your code capable of tracking back to a macro
definition?
Also, it would be nicer if we could put the marker comment on an
adjacent line. If it has to be on the same line then there are
formatting problems when the code is wide (and pgindent could break it).
regards, tom lane
John Regehr <regehr@cs.utah.edu> writes:
> Tom, would you be willing to isolate these operations into functions
> that could be marked with a "no_overflow_check" attribute? This would
> be easy for us to deal with, would survive preprocesing cleanly, and
> wouldn't have any performance cost since inliners do a fine job.
Uh, *some* inliners do a fine job. I'm not really willing to depend on
that for performance.
However, most of the cases that seem of interest so far are in fairly
small, stable functions. Would it be reasonable to attach a "checked
for overflow problems" label to these functions as a whole?
regards, tom lane
> Just to follow up: all the other ones seem to be non-problems. Would you folks be willing to specify which arithmetic operations are considered to be safe in the case of overflow? Something simple like an "INTEGER_OVERFLOW_OK" comment at the end of the line of code containing the operation would suffice. This would let me automatically filter out error messages on these lines of code in the future. John Regehr
Tom, would you be willing to isolate these operations into functions that could be marked with a "no_overflow_check" attribute? This would be easy for us to deal with, would survive preprocesing cleanly, and wouldn't have any performance cost since inliners do a fine job. John On 8/3/2010 3:43 PM, Tom Lane wrote: > John Regehr<regehr@cs.utah.edu> writes: >>> Just to follow up: all the other ones seem to be non-problems. > >> Would you folks be willing to specify which arithmetic operations are >> considered to be safe in the case of overflow? Something simple like an >> "INTEGER_OVERFLOW_OK" comment at the end of the line of code containing >> the operation would suffice. This would let me automatically filter out >> error messages on these lines of code in the future. > > That might be doable for individual operations, but I don't think that > (for example) having to label all the users of RIGHTMOST_ONE() would be > very maintainable. Is your code capable of tracking back to a macro > definition? > > Also, it would be nicer if we could put the marker comment on an > adjacent line. If it has to be on the same line then there are > formatting problems when the code is wide (and pgindent could break it). > > regards, tom lane >
On 8/3/2010 4:08 PM, Tom Lane wrote: > However, most of the cases that seem of interest so far are in fairly > small, stable functions. Would it be reasonable to attach a "checked > for overflow problems" label to these functions as a whole? This should work great. I'll get my clang hacker to start working on it. I see that pgsql has support already in place to def out attributes when non-GCC ocmpilers are used. Just to be clear we're talking about putting something like this in your header files: int bms_first_member(Bitmapset *a) __attribute__((no_integer_overflow_checks)); Pick whatever name you like for this attribute, it doesn't matter to us. John