Обсуждение: __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@ohmu.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@ohmu.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 greatlybenefit 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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@ohmu.fi> wrote: > 15.01.2015, 21:58, Robert Haas kirjoitti: >> On Wed, Jan 14, 2015 at 5:54 PM, Andres Freund <andres@2ndquadrant.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@ohmu.fi> wrote: >> 15.01.2015, 21:58, Robert Haas kirjoitti: >>> On Wed, Jan 14, 2015 at 5:54 PM, Andres Freund <andres@2ndquadrant.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 */#endifItemPointerData; and void -pg_attribute_noreturn + pg_attribute_noreturnplpgsql_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