[config] [atomic] Problem with placement of BOOST_ALIGNMENT - requesting opinions/suggestions
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
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.
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.
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.
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
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.
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
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...
participants (3)
-
Andrey Semashev -
Groke, Paul -
Peter Dimov