Re: [HACKERS] GIST code doesn't build on strict 64-bit machines
Ok, I just commited changes, pls, check it on HPPA. -- Teodor Sigaev E-mail: [EMAIL PROTECTED] ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] GIST code doesn't build on strict 64-bit machines
Tom Lane wrote:
I've just found out the hard way that Postgres doesn't even build on
recent gcc releases for 64-bit HPPA. The reason is that the compiler
now notices and complains about alignment errors that will lead to
core dump at runtime, and GIST has got some. The particular code that
fails to compile is in gist.c:
gistentryinit(((GISTENTRY *) VARDATA(evec))[1],
((GISTENTRY *) VARDATA(evec))[0].key, r, NULL,
(OffsetNumber) 0, ((GISTENTRY *)
VARDATA(evec))[0].bytes, FALSE);
Since VARDATA() is at a 4-byte offset from the start of the datum, this
is trying to overlay a GISTENTRY struct at a 4-byte boundary. When
compiling in 64-bit mode, Datum is 8 bytes, and so the GISTENTRY struct
is not sufficiently well aligned. Unlike Intel machines, the HP chips
*require* 8-byte loads and stores to be 8-byte-aligned.
Hm.
evec is defined as
storage = (char *) palloc(n * sizeof(GISTENTRY) + MAXALIGN(VARHDRSZ));
evec = (bytea *) (storage + MAXALIGN(VARHDRSZ) - VARHDRSZ);
VARDATA is defined as:
#define VARDATA(__PTR) VARATT_DATA(__PTR)
#define VARATT_DATA(PTR) (((varattrib *)(PTR))-va_content.va_data)
and VARHDRSZ is
#define VARHDRSZ((int32) sizeof(int32))
Look follow:
VARATT_SIZEP(evec) = 2 * sizeof(GISTENTRY) + VARHDRSZ;
#define VARATT_SIZEP(_PTR) (((varattrib *)(_PTR))-va_header)
So, if ((varattrib *)evec)-va_content.va_data - (char*)evec == 4 then
((GISTENTRY *) VARDATA(evec))[i] is 8-byte aligned, but evec - no.
But if ((varattrib *)evec)-va_content.va_data - (char*)evec == 8 then
both evec and ((GISTENTRY *) VARDATA(evec))[i] isn't 8-byte aligned.
I don't afraid to say some rubbish :)
I wrote simple test:
#include stdio.h
#include c.h
typedef struct {
int32 len;
chardata[1];
} TST;
int main(int argn, char *argv[]) {
TST t;
TST *ptr = t;
printf(%d\n, (ptr-data - (char*)ptr));
return 0;
}
It prints 4 for my Intel systems and for Alpha system, but I havn't access to
HPUX. If result is equal to 8 on HPUX, then I suppose that replacing to
evec = (bytea *) storage
will resolve our problem (but VARHDRSZ has inconsistent definition).
But if result is 4 then we should use
evec = (bytea *) storage
and replace all VARDATA macro to something like
#define MY_VARDATA(PTR) ( ((char*)PTR) + MAXALIGN(VARHDRSZ) )
But all of this is strage for me, because we already faced to problem with
8-bytes strict aliasing in GiST code, and we had resolved problem on Sun and
Alpha boxes. What was it changed?
I suppose that a correct fix involves doing MAXALIGN(VARDATA(evec)),
but I do not know what places need to change to support this.
Its only union and picksplit user-defined methods in contrib modules.
--
Teodor Sigaev E-mail: [EMAIL PROTECTED]
---(end of broadcast)---
TIP 2: you can get off all lists at once with the unregister command
(send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [HACKERS] GIST code doesn't build on strict 64-bit machines
Teodor Sigaev [EMAIL PROTECTED] writes: But all of this is strage for me, because we already faced to problem with 8-bytes strict aliasing in GiST code, and we had resolved problem on Sun and Alpha boxes. What was it changed? It looks to me like the HP compiler is expecting that the constant offset part of a doubleword load or store instruction should be a multiple of 8. The offset-the-evec hack you're using falls foul of that even though the ultimate runtime address would be legal. I'm not sure whether this is a constraint of the actual HPPA instruction format, or just overly anal compile-time testing. gcc doesn't seem to have a problem, but it's quite possibly not generating the most efficient instruction sequence, either. I suppose that a correct fix involves doing MAXALIGN(VARDATA(evec)), but I do not know what places need to change to support this. Its only union and picksplit user-defined methods in contrib modules. If I recall correctly, we decided to go with the present hack because we found the problem just before a release date and there wasn't time to do it more cleanly. It seems to me that there is time to fix it right for 7.5 ... regards, tom lane ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] GIST code doesn't build on strict 64-bit machines
I suppose that a correct fix involves doing MAXALIGN(VARDATA(evec)),
but I do not know what places need to change to support this.
Its only union and picksplit user-defined methods in contrib modules.
If I recall correctly, we decided to go with the present hack because we
found the problem just before a release date and there wasn't time to do
it more cleanly. It seems to me that there is time to fix it right for
7.5 ...
Yes, you are right.
I suggest to replace bytea by struct
typedef struct {
int32 n; /* number of GISTENTRY */
GISTENTRY vector[1];
} GistEntryVector;
#define GEVHDRSZ (MAXALIGN(sizeof(int32))
so, allocation will be:
evec = palloc( GEVHDRSZ + sizeof(GISTENTRY)*n );
MAXALIGN guarantee that allocated memory will be no less than required (it may
be greater for 4 bytes).
And change interface to user defined structures from
Datum union(bytea *entryvec, int *size)
Datum picksplit(bytea *entryvec, GIST_SPLITVEC *v)
to
Datum union(GistEntryVector *entryvec, int *size)
Datum picksplit(GistEntryVector *entryvec, GIST_SPLITVEC *v)
In this function it's need to use entryvec-n and entryvec-vector
We can do even
Datum union(int32 n, GISTENTRY *entryvec, int *size)
Datum picksplit(int32 n, GISTENTRY *entryvec, GIST_SPLITVEC *v)
It seems to me that first case is clearer. Of course, I change all contrib
modules to new interface.
What do you think?
--
Teodor Sigaev E-mail: [EMAIL PROTECTED]
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [HACKERS] GIST code doesn't build on strict 64-bit machines
Teodor Sigaev [EMAIL PROTECTED] writes:
I suggest to replace bytea by struct
typedef struct {
int32 n; /* number of GISTENTRY */
GISTENTRY vector[1];
} GistEntryVector;
Yes, I was thinking the same thing.
#define GEVHDRSZ (MAXALIGN(sizeof(int32))
so, allocation will be:
evec = palloc( GEVHDRSZ + sizeof(GISTENTRY)*n );
MAXALIGN guarantee that allocated memory will be no less than required (it may
be greater for 4 bytes).
That would work, or you could use offsetof(GistEntryVector, vector[0]).
regards, tom lane
---(end of broadcast)---
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
[HACKERS] GIST code doesn't build on strict 64-bit machines
I've just found out the hard way that Postgres doesn't even build on recent gcc releases for 64-bit HPPA. The reason is that the compiler now notices and complains about alignment errors that will lead to core dump at runtime, and GIST has got some. The particular code that fails to compile is in gist.c: gistentryinit(((GISTENTRY *) VARDATA(evec))[1], ((GISTENTRY *) VARDATA(evec))[0].key, r, NULL, (OffsetNumber) 0, ((GISTENTRY *) VARDATA(evec))[0].bytes, FALSE); Since VARDATA() is at a 4-byte offset from the start of the datum, this is trying to overlay a GISTENTRY struct at a 4-byte boundary. When compiling in 64-bit mode, Datum is 8 bytes, and so the GISTENTRY struct is not sufficiently well aligned. Unlike Intel machines, the HP chips *require* 8-byte loads and stores to be 8-byte-aligned. I suppose that a correct fix involves doing MAXALIGN(VARDATA(evec)), but I do not know what places need to change to support this. regards, tom lane ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
