__attribute__ for non-gcc compilers

Commit db4ec2ffce35 added alignment attributes for 64-bit atomic
variables as required on 32-bit platforms using
__attribute__((aligned(8)). That works fine with GCC, and would work
with Solaris Studio Compiler [1] and IBM XL C [2], but src/include/c.h
defines __attribute__ as an empty macro when not using GCC.
Unfortunately we can't just disable that #define and enable all
__attributes__ for Solaris CC and XLC as we use a bunch of attributes
that are not supported by those compilers and using them unconditionally
would generate a lot of warnings.

Attached a patch that defines custom macros for each attribute and
enables them individually for compilers that support them and never
defines __attribute__.

I have tested this with GCC 4.9.2 on Linux x86-64 and Solaris CC 5.12 on
Sparc (32-bit build); I don't have access to an IBM box with XLC.

/ Oskari

[1] https://docs.oracle.com/cd/E18659_01/html/821-1384/gjzke.html
[2]
http://www-01.ibm.com/support/knowledgecenter/SSGH2K_11.1.0/com.ibm.xlc111.aix.doc/language_ref/var_attrib_aligned.html

On Tue, Jan 13, 2015 at 4:18 PM, Oskari Saarenmaa <os(at)ohmu(dot)fi> wrote:
> Commit db4ec2ffce35 added alignment attributes for 64-bit atomic
> variables as required on 32-bit platforms using
> __attribute__((aligned(8)). That works fine with GCC, and would work
> with Solaris Studio Compiler [1] and IBM XL C [2], but src/include/c.h
> defines __attribute__ as an empty macro when not using GCC.
> Unfortunately we can't just disable that #define and enable all
> __attributes__ for Solaris CC and XLC as we use a bunch of attributes
> that are not supported by those compilers and using them unconditionally
> would generate a lot of warnings.
>
> Attached a patch that defines custom macros for each attribute and
> enables them individually for compilers that support them and never
> defines __attribute__.
>
> I have tested this with GCC 4.9.2 on Linux x86-64 and Solaris CC 5.12 on
> Sparc (32-bit build); I don't have access to an IBM box with XLC.

I guess my first question is whether we want to be relying on
__attribute__((aligned)) in the first place. If we do, then this
seems like a pretty sensible and necessary change.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2015-01-14 15:48:47 -0500, Robert Haas wrote:
> On Tue, Jan 13, 2015 at 4:18 PM, Oskari Saarenmaa <os(at)ohmu(dot)fi> wrote:
> > Commit db4ec2ffce35 added alignment attributes for 64-bit atomic
> > variables as required on 32-bit platforms using
> > __attribute__((aligned(8)). That works fine with GCC, and would work
> > with Solaris Studio Compiler [1] and IBM XL C [2], but src/include/c.h
> > defines __attribute__ as an empty macro when not using GCC.
> > Unfortunately we can't just disable that #define and enable all
> > __attributes__ for Solaris CC and XLC as we use a bunch of attributes
> > that are not supported by those compilers and using them unconditionally
> > would generate a lot of warnings.
> >
> > Attached a patch that defines custom macros for each attribute and
> > enables them individually for compilers that support them and never
> > defines __attribute__.
> >
> > I have tested this with GCC 4.9.2 on Linux x86-64 and Solaris CC 5.12 on
> > Sparc (32-bit build); I don't have access to an IBM box with XLC.
>
> I guess my first question is whether we want to be relying on
> __attribute__((aligned)) in the first place.

I think it's better than the alternatives:

a) Don't support 64bit atomics on any 32bit platform. I think that'd be
sad because there's some places that could greatly benefit from being
able to read/store/manipulate e.g. LSNs atomically.
b) Double the size of 64bit atomics on 32bit platforms, and add
TYPEALIGN() to every access inside the atomics implementation.
c) Require 64 atomics to be aligned appropriately manually in every
place they're embedded. I think that's completely impractical.

The only viable one imo is a)

Since the atomics code is compiler dependant anyway, I don't much of a
problem with relying on compiler specific alignment
instructions. Really, the only problem right now is that __attribute__
is defined to be empty on compilers where it has meaning.

> If we do, then this seems like a pretty sensible and necessary change.

I think it's also an improvment independent from the alignment
code. Having a more widely portable __attribute__((noreturn)),
__attribute__((unused)), __attribute__((format(..))) seems like a good
general improvement. And all of these are supported in other compilers
than gcc.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Jan 14, 2015 at 5:29 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> I think it's better than the alternatives:
>
> a) Don't support 64bit atomics on any 32bit platform. I think that'd be
> sad because there's some places that could greatly benefit from being
> able to read/store/manipulate e.g. LSNs atomically.
> b) Double the size of 64bit atomics on 32bit platforms, and add
> TYPEALIGN() to every access inside the atomics implementation.
> c) Require 64 atomics to be aligned appropriately manually in every
> place they're embedded. I think that's completely impractical.
>
> The only viable one imo is a)

I can't really fault that reasoning, but if __attribute__((align))
only works on some platforms, then you've got silent, subtle breakage
on the ones where it doesn't.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2015-01-14 17:46:39 -0500, Robert Haas wrote:
> On Wed, Jan 14, 2015 at 5:29 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > I think it's better than the alternatives:
> >
> > a) Don't support 64bit atomics on any 32bit platform. I think that'd be
> > sad because there's some places that could greatly benefit from being
> > able to read/store/manipulate e.g. LSNs atomically.
> > b) Double the size of 64bit atomics on 32bit platforms, and add
> > TYPEALIGN() to every access inside the atomics implementation.
> > c) Require 64 atomics to be aligned appropriately manually in every
> > place they're embedded. I think that's completely impractical.
> >
> > The only viable one imo is a)
>
> I can't really fault that reasoning, but if __attribute__((align))
> only works on some platforms, then you've got silent, subtle breakage
> on the ones where it doesn't.

The __attribute__((align()))'s are in compiler specific code sections
anyway - and there's asserts ensuring that the alignment is correct in
all routines where it matters (IIRC). Those are what caught the
problem. C.f.
http://archives.postgresql.org/message-id/20150108204635.GK6299%40alap3.anarazel.de

I think I'd for now simply not define pg_attribute_aligned() on
platforms where it's not supported, instead of defining it empty. If we
need a softer variant we can name it pg_attribute_aligned_if_possible or
something.

Sounds sane?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

15.01.2015, 00:54, Andres Freund kirjoitti:
> I think I'd for now simply not define pg_attribute_aligned() on
> platforms where it's not supported, instead of defining it empty. If we
> need a softer variant we can name it pg_attribute_aligned_if_possible or
> something.

Good point, all attributes that actually change program behavior
(aligned and packed for now) need to error out during compilation if
they're used and they're not supported by the compiler.

Attributes which may improve optimization or just provide more
information for the developers (noreturn, unused, format) can be defined
empty when they're not supported (or are not supported well enough: GCC
< 3 doesn't know about %z in printf format.)

/ Oskari

On Wed, Jan 14, 2015 at 5:54 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> I think I'd for now simply not define pg_attribute_aligned() on
> platforms where it's not supported, instead of defining it empty. If we
> need a softer variant we can name it pg_attribute_aligned_if_possible or
> something.
>
> Sounds sane?

Yes, that sounds like a much better plan.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

15.01.2015, 21:58, Robert Haas kirjoitti:
> On Wed, Jan 14, 2015 at 5:54 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I think I'd for now simply not define pg_attribute_aligned() on
>> platforms where it's not supported, instead of defining it empty. If we
>> need a softer variant we can name it pg_attribute_aligned_if_possible or
>> something.
>>
>> Sounds sane?
>
> Yes, that sounds like a much better plan.

Attached an updated patch rebased on today's git master that never
defines aligned or packed empty.

This is also included in the current commitfest,
https://commitfest.postgresql.org/4/115/

/ Oskari

On 2015-02-17 15:41:45 +0200, Oskari Saarenmaa wrote:
> 15.01.2015, 21:58, Robert Haas kirjoitti:
> > On Wed, Jan 14, 2015 at 5:54 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> I think I'd for now simply not define pg_attribute_aligned() on
> >> platforms where it's not supported, instead of defining it empty. If we
> >> need a softer variant we can name it pg_attribute_aligned_if_possible or
> >> something.
> >>
> >> Sounds sane?
> >
> > Yes, that sounds like a much better plan.
>
> Attached an updated patch rebased on today's git master that never
> defines aligned or packed empty.

Cool, that looks good. Besides allowing other compilers to use the
__attribute__ stuff, it also seems like a readability win to
me. Especially the various printf stuff looks much better to me this
way.

I guess you've tested this on solaris and x86 linux?

Greetings,

Andres Freund

17.02.2015, 15:46, Andres Freund kirjoitti:
> On 2015-02-17 15:41:45 +0200, Oskari Saarenmaa wrote:
>> 15.01.2015, 21:58, Robert Haas kirjoitti:
>>> On Wed, Jan 14, 2015 at 5:54 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>>> I think I'd for now simply not define pg_attribute_aligned() on
>>>> platforms where it's not supported, instead of defining it empty. If we
>>>> need a softer variant we can name it pg_attribute_aligned_if_possible or
>>>> something.
>>>>
>>>> Sounds sane?
>>>
>>> Yes, that sounds like a much better plan.
>>
>> Attached an updated patch rebased on today's git master that never
>> defines aligned or packed empty.
>
> Cool, that looks good. Besides allowing other compilers to use the
> __attribute__ stuff, it also seems like a readability win to
> me. Especially the various printf stuff looks much better to me this
> way.
>
> I guess you've tested this on solaris and x86 linux?

Yeah, tested on x86-64 Linux using gcc version 4.9.2 20150212 (Red Hat
4.9.2-6) and on Solaris 10 using Sun C 5.12 SunOS_sparc.

/ Oskari

On Tue, Feb 17, 2015 at 8:41 AM, Oskari Saarenmaa <os(at)ohmu(dot)fi> wrote:
> 15.01.2015, 21:58, Robert Haas kirjoitti:
>> On Wed, Jan 14, 2015 at 5:54 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> I think I'd for now simply not define pg_attribute_aligned() on
>>> platforms where it's not supported, instead of defining it empty. If we
>>> need a softer variant we can name it pg_attribute_aligned_if_possible or
>>> something.
>>>
>>> Sounds sane?
>>
>> Yes, that sounds like a much better plan.
>
> Attached an updated patch rebased on today's git master that never
> defines aligned or packed empty.
>
> This is also included in the current commitfest,
> https://commitfest.postgresql.org/4/115/

Is this going to play nicely with pgindent?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

23.02.2015, 04:31, Robert Haas kirjoitti:
> On Tue, Feb 17, 2015 at 8:41 AM, Oskari Saarenmaa <os(at)ohmu(dot)fi> wrote:
>> 15.01.2015, 21:58, Robert Haas kirjoitti:
>>> On Wed, Jan 14, 2015 at 5:54 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>>> I think I'd for now simply not define pg_attribute_aligned() on
>>>> platforms where it's not supported, instead of defining it empty. If we
>>>> need a softer variant we can name it pg_attribute_aligned_if_possible or
>>>> something.
>>>>
>>>> Sounds sane?
>>>
>>> Yes, that sounds like a much better plan.
>>
>> Attached an updated patch rebased on today's git master that never
>> defines aligned or packed empty.
>>
>> This is also included in the current commitfest,
>> https://commitfest.postgresql.org/4/115/
>
> Is this going to play nicely with pgindent?

I ran pgindent on the tree with this patch applied (with a few changes,
pgindent modified atomics headers a bit) and the changes looked ok to
me, mostly pgindent just rewrapped lines like this:

-extern void quickdie(SIGNAL_ARGS) pg_attribute_noreturn;
+extern void
+quickdie(SIGNAL_ARGS) pg_attribute_noreturn;

but there were two cases where it produced a bit weird indentation:

#ifdef __arm__
-pg_attribute_packed /* Appropriate whack upside the
head for ARM */
+ pg_attribute_packed /* Appropriate whack upside
the head for ARM */
#endif
ItemPointerData;

and

void
-pg_attribute_noreturn
+ pg_attribute_noreturn
plpgsql_yyerror(const char *message)
{

/ Oskari

Hi,

On 2015-02-17 15:41:45 +0200, Oskari Saarenmaa wrote:
> Attached an updated patch rebased on today's git master that never
> defines aligned or packed empty.
>
> This is also included in the current commitfest,
> https://commitfest.postgresql.org/4/115/

I pushed a slightly modified (mostly moved the new definitions up)
modified version. Sorry for taking so long, and thanks for the help.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services