"24" < INT_MIN returns TRUE ???

Hi,

did anyone compile latest CVS with v1.31
utils/adt/numutils.c?

Marc activated some range checks in pg_atoi() and now I have
a very interesting behaviour on a Linux box running gcc 2.8.1
glibc-2.

Inside of pg_atoi(), the value is read into a long. Comparing
a small positive long like 24 against INT_MIN returns TRUE -
dunno how. Putting INT_MIN into another long variable and
comparing the two returns the expected FALSE - so what's
going on here? long, int32 and int have all 4 bytes here.

I just reversed out the patch. It was causing initdb to fail!

I don't understand why it fails either, but have sent the report back to
the patch author.

I would love to hear the answer on this one.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@candle.pha.pa.us            |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Inside of pg_atoi(), the value is read into a long. Comparing
a small positive long like 24 against INT_MIN returns TRUE -
dunno how. Putting INT_MIN into another long variable and
comparing the two returns the expected FALSE - so what's
going on here? long, int32 and int have all 4 bytes here.

I believe the problem is that the compiler is deciding that INT_MIN
is of type "unsigned int" or "unsigned long", whereupon the type
promotion rules will cause the comparison to be done in
unsigned-long arithmetic. And indeed, 24 < 0x80000000 in unsigned
arithmetic. When you compare two long variables,
you get the desired behavior of signed long comparison.

Do you have <limits.h>, and if so how does it define INT_MIN?

The default INT_MIN provided at the top of numutils.c is clearly
prone to cause this problem:
#ifndef INT_MIN
#define INT_MIN (-0x80000000L)
#endif
If long is 32 bits then the constant 0x80000000L will be classified
as unsigned long by an ANSI-compliant compiler, whereupon the test
in pg_atoi fails.

The two systems I have here both avoid this problem in <limits.h>,
but I wonder whether you guys have different or no <limits.h>.

I recommend a two-pronged approach to dealing with this bug:

1. The default INT_MIN ought to read
#define INT_MIN (-INT_MAX-1)
so that it is classified as a signed rather than unsigned long.
(This is how both of my systems define it in <limits.h>.)

2. The two tests in pg_atoi ought to read
if (l < (long) INT_MIN)
...
if (l > (long) INT_MAX)

The second change should ensure we get a signed-long comparison
even if <limits.h> has provided a broken definition of INT_MIN.
The first change is not necessary to fix this particular bug
if we also apply the second change, but I think leaving INT_MIN
the way it is is trouble waiting to happen.

Bruce, I cannot check this since my system won't show the failure;
would you un-reverse-out the prior patch, add these changes, and
see if it works for you?

regards, tom lane

I said:

Do you have <limits.h>, and if so how does it define INT_MIN?

Actually, looking closer, it doesn't matter whether you have <limits.h>,
because there is yet a *third* bug in numutils.c:

#ifdef HAVE_LIMITS
#include <limits.h>
#endif

should be

#ifdef HAVE_LIMITS_H
...

because that is how configure and config.h spell the configuration
symbol. Thus, <limits.h> is never included on *any* platform,
and our broken default INT_MIN is always used.

Whoever wrote this code was not having a good day...

regards, tom lane

I said:

Do you have <limits.h>, and if so how does it define INT_MIN?

Actually, looking closer, it doesn't matter whether you have <limits.h>,
because there is yet a *third* bug in numutils.c:

#ifdef HAVE_LIMITS
#include <limits.h>
#endif

should be

#ifdef HAVE_LIMITS_H
...

because that is how configure and config.h spell the configuration
symbol. Thus, <limits.h> is never included on *any* platform,
and our broken default INT_MIN is always used.

Yes, I caught this when you made that comment about the LIMIT test. I
am checking all the other HAVE_ tests.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@candle.pha.pa.us            |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Yes, I caught this when you made that comment about the LIMIT test. I
am checking all the other HAVE_ tests.

OK, patch reapplyed, and change made to #define test, and default
MAX/MIN values. It works fine now.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@candle.pha.pa.us            |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026