Re: [boost] [config] [atomic] Problem with placement of BOOST_ALIGNMENT - requesting opinions/suggestions

Subject: Re: [boost] [config] [atomic] Problem with placement of BOOST_ALIGNMENT - requesting opinions/suggestions
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2017-02-27 16:00:05

On 02/27/17 16:31, Groke, Paul via Boost wrote:
> While preparing a pull request for my Boost.Atomic extensions for the
> z/OS compiler, I noticed that I have a slight problem there. To get the
> code to work on z/OS, I changed the placement of the BOOST_ALIGNMENT
> macro in atomic/detail/storage_type.hpp from...
>
> BOOST_ALIGNMENT(16) unsigned char data[Size];
>
> to
>
> unsigned char data[Size] BOOST_ALIGNMENT(16);
>
> ...because unfortunately the z compiler doesn't understand the
> __attribute__ ((__aligned__(x))) when it's at the beginning of a
> variable/data member definition. (For the GNU __attribute__
> ((__aligned__(x))) the new location is also the only documented
> location, although GCC doesn't seem to mind.)

This makes the compiler incompatible with the current semantics of 
BOOST_ALIGNMENT. You probably need to revise the changes made in the 
recent Boost.Config PR[1] regarding BOOST_ALIGNMENT. Please, prepare an 
updating PR for Boost.Config.

> While reviewing my changes I now realized that this won't do, because
> BOOST_ALIGNMENT can also be defined as __declspec(align(x)) or
> alignas(x), both of which are legal at the beginning but not at the end
> of a variable/data member definition.
> (I'm not 100% sure about valid positions of alignas (x) - the examples I
> can find use alignas (x) at the beginning, but the compilers we
> currently use - which include MSVC, GCC and Clang, also seem to accept
> it at the end. __declspec(align(x)) definitely is a problem though.)

Note also that BOOST_ALIGNMENT can (and is) also used as a type 
attribute. Does your compiler support that?

> And now I'm wondering what to do about this.
>
> One way to fix this would obviously be to introduce two additional
> macros, e.g. BOOST_ALIGNMENT_DECLSPEC and BOOST_ALIGNMENT_ATTRIBUTE.
> Only one of those would then expand to something non-empty, and both
> would have to be used in variable definitions. The BOOST_ALIGNMENT macro
> would remain as is for type declarations and for compatibility.
> The code in storage_type.hpp would then have to be re-written as
>
> BOOST_ALIGNMENT_DECLSPEC(16) unsigned char data[Size] BOOST_ALIGNMENT_ATTRIBUTE(16);
>
> Which of course is very very ugly.

Yeah, I don't really like the duplication.

> Another option would be a
> BOOST_ALIGNED_VARIABLE(alignment, variable_definition)
> macro. This would be nice in definitions like
>
> BOOST_ALIGNED_VARIABLE(16, unsigned char data[Size]);
>
> but much less nice in definitions like
>
> BOOST_ALIGNED_VARIABLE(16, some_template<foo BOOST_PP_COMMA bar BOOST_PP_COMMA baz>);
>
> And since not all compilers support variadic macros, this is probably
> the best one could do.

I'm not sure BOOST_PP_COMMA would work. I'm not very good at 
preprocessor programming, but doesn't BOOST_PP_COMMA get expanded some 
time before BOOST_ALIGNED_VARIABLE? That would result in the latter 
being invoked with a variable number of arguments.

A typedef could be used as a workaround, although it may be inconvenient 
in some cases.

> The third option I can think of would be to leave things as they are,
> which of course would mean that I can't submit my Boost.Atomic
> implementation for z/OS... hm.
>
> I realize that making things uglier for a single compiler that almost
> nobody uses isn't something many people will cheer for. But then again
> I'd really like to have my Boost.Atomic implementation for z/OS in the
> official Boost releases :)
>
> So I'm calling out to you for suggestions/opinions/comments. Is there
> another way to combine the above mentioned square peg + round hole? And
> which option would you deem favorable/acceptable?

Frankly, I'm not quite happy with either option, although 
BOOST_ALIGNED_VARIABLE seems the lesser evil.

One other idea that I think is worth trying is to see if your compiler 
can cope with alignment attribute in a type definition, in a usual 
placement.

   struct BOOST_ALIGNMENT(16) aligned
   {
      unsigned char data[Size];
   };

If it does, the code can be changed to avoid applying BOOST_ALIGNMENT to 
variables and use it to align types instead.

Although I used BOOST_ALIGNMENT above for shortness, I shouldn't be 
saying BOOST_ALIGNMENT because, as currently documented[2], 
BOOST_ALIGNMENT is not supported by your compiler and thus should expand 
to nothing (with BOOST_NO_ALIGNMENT defined). If your compiler supports 
type alignment in the above syntax, we could introduce a new macro, e.g. 
BOOST_TYPE_ALIGNMENT(n), that would be usable in that context, but not 
in the context of a variable declaration. It can be accompanied with 
BOOST_NO_TYPE_ALIGNMENT, similar to BOOST_NO_ALIGNMENT.

If it doesn't work with types, then you could just sacrifice 128-bit 
atomic ops in Boost.Atomic. No changes to Boost.Atomic should be needed 
for that because the lesser types should already get the necessary 
alignment naturally, and 128-bit storage will be disabled by the current 
preprocessor checks. I think, that would be a good solution with minimal 
impact on the existing code unless, of course, you require 128-bit atomics.

[1]: https://github.com/boostorg/config/pull/122
[2]: 
http://www.boost.org/doc/libs/1_63_0/libs/config/doc/html/boost_config/boost_macro_reference.html#boost_config.boost_macro_reference.macros_that_allow_use_of_c__11_features_with_c__03_compilers