Thread: "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. Someone any clue? Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #========================================= wieck@debis.com (Jan Wieck) #
> 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, Pennsylvania19026
>> 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 readif (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, Pennsylvania19026
> 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, Pennsylvania19026