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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
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.)

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.)

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.

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.

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?

Thank you for taking the time to read this wall of text,
Cheers,
Paul Groke

The contents of this e-mail are intended for the named addressee only. It contains information that may be confidential. Unless you are the named addressee or an authorized designee, you may not copy or use it, or disclose it to anyone else. If you received it in error please notify us immediately and then destroy it. Dynatrace Austria GmbH (registration number FN 91482h) is a company registered in Linz whose registered office is at 4040 Linz, Austria, Freistaedterstrasse 313

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
Groke, Paul wrote:
...
> One way to fix this would obviously be to introduce two additional macros,
> e.g. BOOST_ALIGNMENT_DECLSPEC and BOOST_ALIGNMENT_ATTRIBUTE.

...
> Another option would be a BOOST_ALIGNED_VARIABLE(alignment,
> variable_definition) macro.

...
> 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.

The obvious fourth option is to just #ifdef here for z/OS.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 02/27/17 17:07, Peter Dimov via Boost wrote:

> Groke, Paul wrote:
> ...
>> One way to fix this would obviously be to introduce two additional
>> macros, e.g. BOOST_ALIGNMENT_DECLSPEC and BOOST_ALIGNMENT_ATTRIBUTE.
>
> ...
>> Another option would be a BOOST_ALIGNED_VARIABLE(alignment,
>> variable_definition) macro.
>
> ...
>> 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.
>
> The obvious fourth option is to just #ifdef here for z/OS.

BOOST_ALIGNMENT is used in quite a few places, including outside
Boost.Atomic. Adding preprocessor checks everywhere would be messy.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
Andrey Semashev wrote:
> > The obvious fourth option is to just #ifdef here for z/OS.
>
> BOOST_ALIGNMENT is used in quite a few places, including outside
> Boost.Atomic. Adding preprocessor checks everywhere would be messy.

Sure, but that's only going to be a problem if we want those places to
compile on z/OS as well.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
Andrey Semashev wrote:
> 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.

Yes, I will. Should I undo the "make BOOST_ALIGNMENT definable in the
compiler header" part (the "escape") as well, or just the definition of
BOOST_ALIGNMENT in the header for the z/OS compiler?
I personally would leave the "escape" part in, because - as I already
wrote in the PR discussion - I think every definition in suffix.hpp
should have an escape. But I'll of course do whatever you think is best.

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

I'm not sure, I'll have to write a test to confirm that. (It parses it
without warning, but then again it also parses it without warning at the
beginning of variable definitions -- it just also ignores it there
*sigh*)

> I'm not sure BOOST_PP_COMMA would work.

I'm ~90% sure it should work, but it's so ugly, and since you suggested
two IMO superior alternatives I guess the point is moot.

> (...) 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.

I like that! I'm not sure if one exotic compiler warrants it though.
If you think it does, I'd be happy to prepare a PR for that too.

The nice thing about that solution is that, assuming the z compiler
handles it correctly, it would catch (almost?) all current usages of
BOOST_ALIGNMENT in Boost that are more than an optimization.

The two usages for variables that I could find in 1.63 are in atomic,
which could probably be extended to use the new type macro on the buffer
struct as well as BOOST_ALIGNMENT on the member, and one usage in
libs/log/src/timestamp.cpp which seems to be just an optimization
(probably to avoid cache line sharing with other global variables).

BTW: Does anyone know of any other compilers that don't support
BOOST_ALIGNMENT-style specifications for variables, but do support
BOOST_ALIGNMENT-style alignment specifications for types?
Because then it wouldn't be just one exotic compiler anymore :)

> 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.

Thanks! IMO that's the best immediate solution. The z compiler indeed
has an intrinsic for the z/Arch's "double-width CAS" instruction, which
would allow 128 bit atomics in 64 bit mode. But we don't need it.
And since there are plenty platforms that don't support double-width CAS
in 64 bit mode, not having it on z either should be acceptable.

----

Peter Dimov wrote:
>>> The obvious fourth option is to just #ifdef here for z/OS.
>>
>> BOOST_ALIGNMENT is used in quite a few places, including outside
>> Boost.Atomic. Adding preprocessor checks everywhere would be messy.
>
> Sure, but that's only going to be a problem if we want those places to
> compile on z/OS as well.

Right. As stated above I'm not sure one exotic compiler warrants a
change/extension to the general BOOST_ALIGNMENT mechanism. But if you/we
decide for it, the BOOST_TYPE_ALIGNMENT(n) thing would be my favorite
solution so far.

And if the decision is against it, I'm not sure that one exotic compiler
warrants #ifdef-ing the Boost.Atomic storage type implementation.

The contents of this e-mail are intended for the named addressee only. It contains information that may be confidential. Unless you are the named addressee or an authorized designee, you may not copy or use it, or disclose it to anyone else. If you received it in error please notify us immediately and then destroy it. Dynatrace Austria GmbH (registration number FN 91482h) is a company registered in Linz whose registered office is at 4040 Linz, Austria, Freistaedterstrasse 313

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
On 02/28/17 00:39, Groke, Paul via Boost wrote:
> Andrey Semashev wrote:
>> 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.
>
> Yes, I will. Should I undo the "make BOOST_ALIGNMENT definable in the
> compiler header" part (the "escape") as well, or just the definition of
> BOOST_ALIGNMENT in the header for the z/OS compiler?

Thanks. I'm ok if the escape is left as is now.

> BTW: Does anyone know of any other compilers that don't support
> BOOST_ALIGNMENT-style specifications for variables, but do support
> BOOST_ALIGNMENT-style alignment specifications for types?

Umm, no, I can't remember any other compilers with such problems. But
then I don't work with many compilers besides the "usual" ones.

> Thanks! IMO that's the best immediate solution. The z compiler indeed
> has an intrinsic for the z/Arch's "double-width CAS" instruction, which
> would allow 128 bit atomics in 64 bit mode. But we don't need it.
> And since there are plenty platforms that don't support double-width CAS
> in 64 bit mode, not having it on z either should be acceptable.

Ok, that's a good starting point. However, I think DCAS is used in
Boost.Lockfree, and maybe somewhere else, so you may want to revisit
this solution later.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
Everybody, I think I need to apologize for "not doing my homework" -
at least not as thoroughly as I probably should have.

I have now written the tests for explicit alignment support of the z/OS
compiler, and they revealed that it doesn't *reliably* support
explicit alignment at all. I'm sorry, I really didn't expect the
compiler to be *that* broken :(

In detail, my tests revealed that while the alignment specs for types
are parsed just fine where BOOST_ALIGNMENT should go, the compiler
"forgets" about them when repeatedly "nesting" the type that carries the
alignment requirement in other types that contain it as a member.
I.e. level 0 (=the type that's "annotated" with __aligned__) works fine,
leve 1 also works fine, but level 2 (e.g. a struct containing a struct
that contains the annotated type) falls back to alignment 8.
Current GCC, Clang and MSVC versions don't seem to have that problem,
they support nesting just fine - at least to a level of 8, which is
where I stopped.

I really thought that with Visual Studio 6.0 withering away in silence,
we wouldn't have to deal with compilers which are that severely broken
anymore.

Andrey Semashev wrote:

> On 02/28/17 00:39, Groke, Paul via Boost wrote:
>> Andrey Semashev wrote:
>>> 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.
>>
>> Yes, I will. Should I undo the "make BOOST_ALIGNMENT definable in the
>> compiler header" part (the "escape") as well, or just the definition
>> of BOOST_ALIGNMENT in the header for the z/OS compiler?
>
> Thanks. I'm ok if the escape is left as is now.

Will do. Tomorrow - it's getting (too) late today.

>> Thanks! IMO that's the best immediate solution. The z compiler indeed
>> has an intrinsic for the z/Arch's "double-width CAS" instruction,
>> which would allow 128 bit atomics in 64 bit mode. But we don't need it.
>> And since there are plenty platforms that don't support double-width
>> CAS in 64 bit mode, not having it on z either should be acceptable.
>
> Ok, that's a good starting point. However, I think DCAS is used in
> Boost.Lockfree, and maybe somewhere else, so you may want to revisit
> this solution later.

Seems this is the only possible solution now, so I guess that's what I'll
do. Thanks for the hint to Boost.Lockfree, I'll check that in time. I don't
think we currently use Boost.Lockfree though. Since the places where
BOOST_ALIGNMENT is used inside Boost itself are limited, I think I'll
have a look at all of them when I find the time.

I don't expect a big problem though, since IIRC not even the Boost.Atomic
implementation for MSVC/x86 supports double width CAS in 64 bit mode.

Sorry again and thanks for all your help!

The contents of this e-mail are intended for the named addressee only. It contains information that may be confidential. Unless you are the named addressee or an authorized designee, you may not copy or use it, or disclose it to anyone else. If you received it in error please notify us immediately and then destroy it. Dynatrace Austria GmbH (registration number FN 91482h) is a company registered in Linz whose registered office is at 4040 Linz, Austria, Freistaedterstrasse 313

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Boost - Dev mailing list
On Tue, Feb 28, 2017 at 2:45 AM, Groke, Paul via Boost
<[hidden email]> wrote:
>
> ...IIRC not even the Boost.Atomic
> implementation for MSVC/x86 supports double width CAS in 64 bit mode.

It does.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Loading...