Could turn on -O2 in AIX

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

but, there is eighter an optimizer bug or a code bug in nabstime.c
leading to regression failure in abstime, tinterval and horology.
The rest all passes.
Does anybody see similar behavior ?

IIRC, the same regression tests fail on Linux/Alpha with 7.0.2, even
at -O0. I had assumed it was a 64-bit issue (probably time_t is 64-bit
on that platform), but maybe not. I haven't had time to dig into the
Alpha situation yet; can you poke at it on your machine and narrow down
the problem?

regards, tom lane

but, there is eighter an optimizer bug or a code bug in nabstime.c
leading to regression failure in abstime, tinterval and horology.
The rest all passes.
Does anybody see similar behavior ?

IIRC, the same regression tests fail on Linux/Alpha with 7.0.2, even
at -O0. I had assumed it was a 64-bit issue (probably time_t
is 64-bit on that platform), but maybe not. I haven't had time to dig into the
Alpha situation yet; can you poke at it on your machine and
narrow down the problem?

The macro AbsoluteTimeIsReal does not work somehow.

Andreas

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

The macro AbsoluteTimeIsReal does not work somehow.

Hm. That expands to

(((int) time) < ((int) 0x7FFFFFFC) && \
((int) time) > ((int) 0x80000001))

On a machine where int is 32 bits, the second constant *ought* to be
treated as -2147483647, but I wonder if your compiler is doing something
bizarre with it --- say, forcing the comparison to be done as unsigned
rather than signed.

regards, tom lane

The macro AbsoluteTimeIsReal does not work somehow.

Hm. That expands to

(((int) time) < ((int) 0x7FFFFFFC) && \
((int) time) > ((int) 0x80000001))

There is a special case in nabstime.h for AIX, which imho
got swapped. The normal case for me would be INT_MIN
and not the 0x80000001.
There is a comment that I don't understand at all given the below
source code:

/*
* AIX considers 2147483648 == -2147483648 (since they have the same bit
* representation) but uses a different sign sense in a comparison to
* these integer constants depending on whether the constant is signed
* or not!
*/
#if defined (_AIX)
#define NOSTART_ABSTIME ((AbsoluteTime) INT_MIN)
#else
#define NOSTART_ABSTIME ((AbsoluteTime) 0x80000001) /* -2147483647 (- 2^31) */
#endif

But that is unfortunately not the problem. Looks like yet another broken compiler to me :-(

Andreas

There is a special case in nabstime.h for AIX, which imho
got swapped. The normal case for me would be INT_MIN
and not the 0x80000001.
There is a comment that I don't understand at all given the below
source code:

/*
* AIX considers 2147483648 == -2147483648 (since they have
the same bit
* representation) but uses a different sign sense in a comparison to
* these integer constants depending on whether the constant is signed
* or not!
*/
#if defined (_AIX)
#define NOSTART_ABSTIME ((AbsoluteTime) INT_MIN)
#else
#define NOSTART_ABSTIME ((AbsoluteTime) 0x80000001)
/* -2147483647 (- 2^31) */
#endif

But that is unfortunately not the problem. Looks like yet
another broken compiler to me :-(

Ok, the comparison ((int) time) > ((int) 0x80000001) is the problem.
Reading the comment again and again, I have come to the conclusion,
that the intent was originally to avoid INT_MIN on AIX.

My solution would be to use INT_MIN for all ports, which has the advantage
that the above problematic comparison can be converted to !=,
since no integer will be smaller than INT_MIN.

Please apply the patch and all works well.

Thanks
Andreas

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

But that is unfortunately not the problem. Looks like yet
another broken compiler to me :-(

Ok, the comparison ((int) time) > ((int) 0x80000001) is the problem.
Reading the comment again and again, I have come to the conclusion,
that the intent was originally to avoid INT_MIN on AIX.

No, I think the other way round. Digging into Postgres 4.2, I find

#if defined(PORTNAME_aix)
/*
* AIX considers 2147483648 == -2147483648 (since they have the same bit
* representation) but uses a different sign sense in a comparison to
* these integer constants depending on whether the constant is signed
* or not!
*/
#include <values.h>
#define NOSTART_ABSTIME ((AbsoluteTime) HIBITI) /* - 2^31 */
#else
#define NOSTART_ABSTIME ((AbsoluteTime) 2147483648) /* - 2^31 */
#endif /* PORTNAME_aix */

where HIBITI must come from a system header, because it doesn't appear
anywhere else in Postgres 4.2. But I'm betting it was a representation
of 0x80000000. By the time of our oldest CVS sources, this had
metamorphosed into

#if defined(PORTNAME_aix)
/*
* AIX considers 2147483648 == -2147483648 (since they have the same bit
* representation) but uses a different sign sense in a comparison to
* these integer constants depending on whether the constant is signed
* or not!
*/
#include <values.h>
/*#define NOSTART_ABSTIME ((AbsoluteTime) HIBITI) */ /* - 2^31 */
#define NOSTART_ABSTIME ((AbsoluteTime) INT_MIN)
#else
/*#define NOSTART_ABSTIME ((AbsoluteTime) 2147483648)*/ /* - 2^31 */
#define NOSTART_ABSTIME ((AbsoluteTime) -2147483647) /* - 2^31 */
#endif /* PORTNAME_aix */

Hard to tell how we got from point A to point B, but it seems
crystal-clear that the *original* author intended to use 0x80000000
on all platforms.

My solution would be to use INT_MIN for all ports, which has the advantage
that the above problematic comparison can be converted to !=,
since no integer will be smaller than INT_MIN.

I agree. When I was looking at this code this morning, I was wondering
what INT_MIN was supposed to represent anyway, if NOSTART_ABSTIME is
INT_MIN + 1. I think someone messed this up between 4.2 and Postgres95.

Thomas, any objection to this plan?

regards, tom lane

My solution would be to use INT_MIN for all ports, which has the advantage
that the above problematic comparison can be converted to !=,
since no integer will be smaller than INT_MIN.

I agree. When I was looking at this code this morning, I was wondering
what INT_MIN was supposed to represent anyway, if NOSTART_ABSTIME is
INT_MIN + 1. I think someone messed this up between 4.2 and Postgres95.

Has there been any consensus yet ? If yes, could you apply my patch please ?
Or should I ask Bruce, for his "faster than his shadow" patch services ?

Thanks
Andreas

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

I agree. When I was looking at this code this morning, I was wondering
what INT_MIN was supposed to represent anyway, if NOSTART_ABSTIME is
INT_MIN + 1. I think someone messed this up between 4.2 and Postgres95.

Has there been any consensus yet ? If yes, could you apply my patch please ?

I have it on my to-do list, but I was waiting to see if Thomas had an
objection (since he knows more about the datetime types than the rest
of us). He's been at Comdex the last few days, which probably explains
the delay.

regards, tom lane

My solution would be to use INT_MIN for all ports, which has the advantage
that the above problematic comparison can be converted to !=,
since no integer will be smaller than INT_MIN.

I agree. When I was looking at this code this morning, I was wondering
what INT_MIN was supposed to represent anyway, if NOSTART_ABSTIME is
INT_MIN + 1. I think someone messed this up between 4.2 and Postgres95.

Thomas, any objection to this plan?

I went ahead and committed this change, since Thomas hasn't weighed in
with an objection ...

regards, tom lane

Thomas Lockhart <lockhart@alumni.caltech.edu> writes:

Is the original issue support for 0x10... as the smallest integer, as
opposed to -MAX_INT? As long as we continue to map the "reserved values"
to the upper and lower range of allowed values so they are unlikely to
appear under normal circumstances, the change should be OK.

I think that the original problem was that Andreas was seeing a compiler
codegen bug on AIX, having to do with the comparison
foo > INT_MIN
generated by the AbsoluteTimeIsReal macro. I think he was seeing that
the compiler insisted on generating an unsigned compare, explicit casts
to signed datatypes notwithstanding :-(.

The proposed fix was to recode the macro's test as foo != INT_MIN,
thereby avoiding the issue of whether the comparison is signed or not.
To do that, we needed to make NOSTART_ABSTIME be defined as INT_MIN
on all platforms, not only AIX. That seemed like a good general-purpose
approach to me anyway, since the intended meaning of 0x80000000 was very
unclear otherwise.

regards, tom lane

PS: I'm quite sure that I'd explicitly cc'd you on the prior discussion.
If you didn't see it, then you've lost personal mail, not only pghackers
traffic...

My solution would be to use INT_MIN for all ports, which has the advantage
that the above problematic comparison can be converted to !=,
since no integer will be smaller than INT_MIN.

I agree. When I was looking at this code this morning, I was wondering
what INT_MIN was supposed to represent anyway, if NOSTART_ABSTIME is
INT_MIN + 1. I think someone messed this up between 4.2 and Postgres95.
Thomas, any objection to this plan?

I went ahead and committed this change, since Thomas hasn't weighed in
with an objection ...

Hmm. I've been seeing a lot of followup messages to threads I don't
recall getting. Presumably all related to a combination of getting
auto-unsubscribed to -hackers :(, traveling and getting mail on my
laptop, and converting service at home over to DSL.

Hopefully this will all settle down here in the next few days.

Is the original issue support for 0x10... as the smallest integer, as
opposed to -MAX_INT? As long as we continue to map the "reserved values"
to the upper and lower range of allowed values so they are unlikely to
appear under normal circumstances, the change should be OK.

- Thomas

Tom, can you remind me where we left this?

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

But that is unfortunately not the problem. Looks like yet
another broken compiler to me :-(

Ok, the comparison ((int) time) > ((int) 0x80000001) is the problem.
Reading the comment again and again, I have come to the conclusion,
that the intent was originally to avoid INT_MIN on AIX.

No, I think the other way round. Digging into Postgres 4.2, I find

#if defined(PORTNAME_aix)
/*
* AIX considers 2147483648 == -2147483648 (since they have the same bit
* representation) but uses a different sign sense in a comparison to
* these integer constants depending on whether the constant is signed
* or not!
*/
#include <values.h>
#define NOSTART_ABSTIME ((AbsoluteTime) HIBITI) /* - 2^31 */
#else
#define NOSTART_ABSTIME ((AbsoluteTime) 2147483648) /* - 2^31 */
#endif /* PORTNAME_aix */

where HIBITI must come from a system header, because it doesn't appear
anywhere else in Postgres 4.2. But I'm betting it was a representation
of 0x80000000. By the time of our oldest CVS sources, this had
metamorphosed into

#if defined(PORTNAME_aix)
/*
* AIX considers 2147483648 == -2147483648 (since they have the same bit
* representation) but uses a different sign sense in a comparison to
* these integer constants depending on whether the constant is signed
* or not!
*/
#include <values.h>
/*#define NOSTART_ABSTIME ((AbsoluteTime) HIBITI) */ /* - 2^31 */
#define NOSTART_ABSTIME ((AbsoluteTime) INT_MIN)
#else
/*#define NOSTART_ABSTIME ((AbsoluteTime) 2147483648)*/ /* - 2^31 */
#define NOSTART_ABSTIME ((AbsoluteTime) -2147483647) /* - 2^31 */
#endif /* PORTNAME_aix */

Hard to tell how we got from point A to point B, but it seems
crystal-clear that the *original* author intended to use 0x80000000
on all platforms.

My solution would be to use INT_MIN for all ports, which has the advantage
that the above problematic comparison can be converted to !=,
since no integer will be smaller than INT_MIN.

I agree. When I was looking at this code this morning, I was wondering
what INT_MIN was supposed to represent anyway, if NOSTART_ABSTIME is
INT_MIN + 1. I think someone messed this up between 4.2 and Postgres95.

Thomas, any objection to this plan?

regards, tom lane

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@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

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom, can you remind me where we left this?

It's done ...

regards, tom lane