Thread: 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
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; char data[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: teodor@sigaev.ru
Teodor Sigaev <teodor@sigaev.ru> 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
>>>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: teodor@sigaev.ru
Teodor Sigaev <teodor@sigaev.ru> 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
Ok, I just commited changes, pls, check it on HPPA. -- Teodor Sigaev E-mail: teodor@sigaev.ru