Ensure RUBY_ALIGNOF == the "alignment requirement" defined in C11 #3570
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Availability of `alignas` is checked in include/ruby/internal/stdalign.h now. That does not need this configure check. Also as commented in the header, we see `_Alignas` being inadequate for our purpose.
It is reported that on a system of i386 System V ABI, GCC returns 8 for __alignof__(double). OTOH the ABI defines alignments of double to be 4, and ISO/IEC 9899:2011 reads that _Alignof(double) shall return 4 on such machine. What we want in ruby is 4 instead of 8 there. We cannot use __alignof__. Additionally, both old GCC / old clang return 8 for _Alignof(double) on such platforms. They are their bugs, and already fixed in recent versions. But we have to support older compilers for a while. Shall check sanity of _Alignof.
Now that RUBY_ALIGNOF behaves like C11's _Alignof. This is not necessarily the best stack arrangement. We can just give up using __builtin_alloca_with_align(), and let alloca choose what is optimal.
Check if RUBY_ALIGNOF(double) is the alignment to store a double inside of a struct.
|
Am checking icc / sunc... |
Was definitely a bad idea to use constexpr. It is not ubiquitous.
shyouhei
force-pushed
the
RBIMPL_ALIGNOF
branch
from
1f9f728
to
c00bfb2
Compare
C++ compilers are optional. Skip C++ tests when they are absent.
shyouhei
force-pushed
the
RBIMPL_ALIGNOF
branch
from
c00bfb2
to
dae0b90
Compare
|
Both icc/sunc seems working _Alignof, no problem observed for me. |
|
It seems fine. I recommend some comment about a struct with flexible array member, though. |
mrkn
previously approved these changes
Sep 24, 2020
mrkn
reviewed
Sep 24, 2020
|
@mrkn I think so. There are other types that exhibit similar behaviour (e.g. |
mrkn
approved these changes
Sep 25, 2020
Added description and URL about nested flexible array member.
| # * querying the alignment of a type, you definitely should use that instead. | ||
| # * There are 2 known pitfalls for this fallback implementation: | ||
| # * | ||
| # * Fitst, it is either an undefined behaviour (C) or an explicit error (C++) |
matzbot
pushed a commit
that referenced
this pull request
Sep 29, 2020
Reported by Mau Magnaguagno See: #3570 (comment)
shyouhei
added a commit
to shyouhei/ruby
that referenced
this pull request
Oct 6, 2020
This was a workaround for RUBY_ALIGNOF's glitch, which has already been fixed. See also ruby#3570
shyouhei
added a commit
that referenced
this pull request
Oct 6, 2020
This was a workaround for RUBY_ALIGNOF's glitch, which has already been fixed. See also #3570
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: #3261 (comment)
GCC/clang have
__alignof__operator. This is very much like C11_Alignof, but can be different on a i386 System V ABI situation. The difference is troublesome. We would like to stick to what C11 says.This is how we achieve that.