Обсуждение: Fix misaligned access of ItemPointerData on ARM

Hello,

Currently there is only one struct that gets packed (via the
__attribute__ keyword that a few compilers support) and it is packed
only on ARM machines -- in order to force it to be exactly 6 bytes long.

But due to how ExecRowMark struct is laid out in memory, the packed
struct ItemPointerData begins at an uneven offset, leading to misaligned
access whenever BlockIdData is set by ItemPointerSetInvalid() (and
likely in some other places, too).

The attached patch is my attempt at fixing that by adding the aligned
attribute (the struct will remain packed).

Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> But due to how ExecRowMark struct is laid out in memory, the packed 
> struct ItemPointerData begins at an uneven offset, leading to misaligned 
> access whenever BlockIdData is set by ItemPointerSetInvalid() (and 
> likely in some other places, too).

But BlockIdData is laid out and accessed as two 16-bit fields, so there
should be no problem.  On what platform exactly do you see a failure?
        regards, tom lane

On 2015-05-21 15:34:00 -0400, Tom Lane wrote:
> Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> > But due to how ExecRowMark struct is laid out in memory, the packed 
> > struct ItemPointerData begins at an uneven offset, leading to misaligned 
> > access whenever BlockIdData is set by ItemPointerSetInvalid() (and 
> > likely in some other places, too).
> 
> But BlockIdData is laid out and accessed as two 16-bit fields, so there
> should be no problem.  On what platform exactly do you see a failure?

It's probably aligned on a byte boundary:

typedef struct ExecRowMark
{Relation    relation;        /* opened and suitably locked relation */Index        rti;            /* its range table
index*/Index        prti;            /* parent range table index, if child */Index        rowmarkId;        /* unique
identifierfor resjunk columns */RowMarkType markType;        /* see enum in nodes/plannodes.h */bool        noWait;
      /* NOWAIT option */ItemPointerData curCtid;    /* ctid of currently locked tuple, if any */
 
} ExecRowMark;

due to the packedness curCtid will quite possibly be stored without any
padding after after noWait.

I wrote:
> But BlockIdData is laid out and accessed as two 16-bit fields, so there
> should be no problem.  On what platform exactly do you see a failure?

Ah, after reading the gcc manual a bit more closely, I get the point.
For some reason I think we assumed that "packed" would not result in
misaligning the struct overall, but it clearly could do so, with possible
efficiency consequences on hardware that doesn't like misaligned accesses.

If the compiler accepts __attribute__((aligned)) then what you've done is
clearly better.  It's not clear to me whether all compilers that accept
"packed" also accept "aligned", but there are enough ARM machines in the
buildfarm that we could hope that we'll find out if this isn't portable.

I wonder whether we should drop the ARM assumption and instead write

#if defined(pg_attribute_packed) && defined(pg_attribute_aligned)
pg_attribute_packed()
pg_attribute_aligned(2)
#endif

so that the annotations are applied on every compiler that accepts them.
        regards, tom lane

On 05/21/2015 04:08 PM, Tom Lane wrote:
> I wrote:
>> But BlockIdData is laid out and accessed as two 16-bit fields, so there
>> should be no problem.  On what platform exactly do you see a failure?
> Ah, after reading the gcc manual a bit more closely, I get the point.
> For some reason I think we assumed that "packed" would not result in
> misaligning the struct overall, but it clearly could do so, with possible
> efficiency consequences on hardware that doesn't like misaligned accesses.
>
> If the compiler accepts __attribute__((aligned)) then what you've done is
> clearly better.  It's not clear to me whether all compilers that accept
> "packed" also accept "aligned", but there are enough ARM machines in the
> buildfarm that we could hope that we'll find out if this isn't portable.
>
> I wonder whether we should drop the ARM assumption and instead write
>
> #if defined(pg_attribute_packed) && defined(pg_attribute_aligned)
> pg_attribute_packed()
> pg_attribute_aligned(2)
> #endif
>
> so that the annotations are applied on every compiler that accepts them.
>
>             


Sounds reasonable.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 05/21/2015 04:08 PM, Tom Lane wrote:
>> I wonder whether we should drop the ARM assumption and instead write
>> 
>> #if defined(pg_attribute_packed) && defined(pg_attribute_aligned)
>> pg_attribute_packed()
>> pg_attribute_aligned(2)
>> #endif
>> 
>> so that the annotations are applied on every compiler that accepts them.

> Sounds reasonable.

We can try it and see if the buildfarm blows up, at least.

I considered also adding a Static_assert about sizeof(ItemIdData),
but I'm afraid that compilers that don't support these pragmas
probably don't support Static_assert either, so it's not clear
that that would catch anything.
        regards, tom lane

On 2015-05-21 16:49:27 -0400, Tom Lane wrote:
> I considered also adding a Static_assert about sizeof(ItemIdData),
> but I'm afraid that compilers that don't support these pragmas
> probably don't support Static_assert either, so it's not clear
> that that would catch anything.

I think your fallback for static assert should pretty much always work?
With an ugly message, but work nonetheless?

Andres

On 05/21/2015 10:08 PM, Tom Lane wrote:
> It's not clear to me whether all compilers that accept "packed" also
> accept "aligned", but there are enough ARM machines in the buildfarm
> that we could hope that we'll find out if this isn't portable.

I think src/include/c.h:614 onward, in its current state, guarantees that?

/* GCC, Sunpro and XLC support aligned, packed and noreturn */
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
#define pg_attribute_noreturn() __attribute__((noreturn))
#define pg_attribute_packed() __attribute__((packed))
#define HAVE_PG_ATTRIBUTE_NORETURN 1
#else

Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> On 05/21/2015 10:08 PM, Tom Lane wrote:
>> It's not clear to me whether all compilers that accept "packed" also
>> accept "aligned", but there are enough ARM machines in the buildfarm
>> that we could hope that we'll find out if this isn't portable.

> I think src/include/c.h:614 onward, in its current state, guarantees that?

Well, c.h *assumes* that.  One of the things I have in mind is that
this should help us find out whether that assumption is valid.
        regards, tom lane

Andres Freund <andres@anarazel.de> writes:
> On 2015-05-21 16:49:27 -0400, Tom Lane wrote:
>> I considered also adding a Static_assert about sizeof(ItemIdData),
>> but I'm afraid that compilers that don't support these pragmas
>> probably don't support Static_assert either, so it's not clear
>> that that would catch anything.

> I think your fallback for static assert should pretty much always work?
> With an ugly message, but work nonetheless?

Good point.  Committed with a static assertion.  We shall now retire
to a safe distance and watch the buildfarm.
        regards, tom lane