Re: RFR: 8305341: Alignment should be enforced by alignas instead of compiler specific attributes [v3]

2023-04-08 Thread Julian Waters
> C11 has been stable for a long time on all platforms, so native code can use 
> the standard alignas operator for alignment requirements

Julian Waters has updated the pull request incrementally with one additional 
commit since the last revision:

  Semicolon

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/13258/files
  - new: https://git.openjdk.org/jdk/pull/13258/files/7dc7f7d8..07f5c702

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=13258&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13258&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/13258.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13258/head:pull/13258

PR: https://git.openjdk.org/jdk/pull/13258


Re: RFR: 8305341: Alignment should be enforced by alignas instead of compiler specific attributes [v3]

2023-04-11 Thread Kim Barrett
On Sat, 8 Apr 2023 13:24:37 GMT, Julian Waters  wrote:

>> C11 has been stable for a long time on all platforms, so native code can use 
>> the standard alignas operator for alignment requirements
>
> Julian Waters has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Semicolon

I don't think the other bug/PR (JDK-8250269, PR#11431) to change HotSpot's
ATTRIBUTE_ALIGNED should be combined with the changes outside of HotSpot.
They are doing rather different things, despite the token "alignas" occuring
in both.  I've been meaning to review PR#11431, but have been swamped.

-

PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1504401317


Re: RFR: 8305341: Alignment should be enforced by alignas instead of compiler specific attributes [v3]

2023-04-11 Thread Kim Barrett
On Sat, 8 Apr 2023 13:24:37 GMT, Julian Waters  wrote:

>> C11 has been stable for a long time on all platforms, so native code can use 
>> the standard alignas operator for alignment requirements
>
> Julian Waters has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Semicolon

I was pretty confused by the C changes for a while, since I didn't know or had
forgotten that the 32bit Windows ABI only guarantees 4 byte stack alignment,
and permits "misaligned" local variables for types such as long long and
double.  (And I failed to find any definitive documentation for that.)  Yuck!

What is the purpose of removing `defined(_MSC_VER)` from the conditionals?  Is
this to allow for other compilers that similarly only ensure 4 byte alignment
of the stack?  If so, it would have been nice to mention that separate concern
in the PR description, rather than making reviewers guess.  And do such
compilers actually exist?  A bit of research suggests gcc (at least) maintains
16 byte alignment (and may warrant the use of -mstackrealign or other hoops).
See, for example, https://github.com/uTox/uTox/issues/1304.

Is `defined(_WIN32)` really the right conditional?  That's true for pretty
much any Visual Studio supported target, both 32bit and 64bit.  But the
alignment spec is effectively a nop for 64bit platforms, so harmless.

-

PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1504466526