Re: RFR: 8305341: Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes
On Fri, 31 Mar 2023 06:07:39 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 I'm not sure what you mean by hotspot requiring a special review, but serviceability code does require two reviewers just like hotspot does. Also, you don't need to split the PR because of that. If you get one person to review the jdk changes and 2 to review the hostpot/svc changes, then you are good to go. - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1495369012
Re: RFR: 8305341: Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes
On Fri, 31 Mar 2023 06:07:39 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 globalDefinitions_visCPP.hpp (and the corresponding globalDefinitions file for gcc based compilers) are both hotspot code, which require special review and are done in another Pull Request: https://github.com/openjdk/jdk/pull/11431 It would be great if Reviewers would take a look at that PR though... - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1495357119
Re: RFR: 8305341: Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes
On Fri, 31 Mar 2023 06:07:39 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 > I don't think I can touch the freetype code since I think it's an external > library that was imported. HotSpot requires a special review for changes like > this, so it's not done here, but instead at #11431 (Which has dried up for a > long time now, would be great if someone finally looked at that PR...) Ok. What about the globalDefinitions_visCPP.hpp reference? - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1494864710
Re: RFR: 8305341: Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes
On Fri, 31 Mar 2023 06:07:39 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 Just checked, the pragmas in the freetype code now seems to be pointless since there is no alignment specified in the code it's supposed to guard. Maybe upstream will fix this someday :/ - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1493245182
Re: RFR: 8305341: Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes
On Fri, 31 Mar 2023 06:07:39 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 I don't think I can touch the freetype code since I think it's an external library that was imported. HotSpot requires a special review for changes like this, so it's not done here, but instead at https://github.com/openjdk/jdk/pull/11431 (Which has dried up for a long time now, would be great if someone finally looked at that PR...) - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1493242732
Re: RFR: 8305341: Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes
On Fri, 31 Mar 2023 06:07:39 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 I don't have any comments on this change in general (it's not something I've dealt with in the past), but I did notice that there are a couple of places you missed: src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:119:#define ATTRIBUTE_ALIGNED(x) __declspec(align(x)) src/java.desktop/share/native/libfreetype/include/freetype/internal/ftvalid.h:82: /* __declspec(align())' in order to compile cleanly with */ src/java.desktop/share/native/libfreetype/src/smooth/ftgrays.c:484: /* __declspec(align())' in order to compile cleanly with */ For the 2nd and 3rd ones you would want to remove all of the following: #if defined( _MSC_VER ) /* Visual C++ (and Intel C++) */ /* We disable the warning `structure was padded due to */ /* __declspec(align())' in order to compile cleanly with */ /* the maximum level of warnings.*/ #pragma warning( push ) #pragma warning( disable : 4324 ) #endif /* _MSC_VER */ ... #if defined( _MSC_VER ) #pragma warning( pop ) #endif - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1492522828
RFR: 8305341: Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes
C11 has been stable for a long time on all platforms, so native code can use the standard alignas operator for alignment requirements - Commit messages: - - GSSLibStub.c - ArrayReferenceImpl.c - Alignment outside of HotSpot should be enforced by alignas instead of compiler specific attributes Changes: https://git.openjdk.org/jdk/pull/13258/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13258&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8305341 Stats: 12 lines in 3 files changed: 3 ins; 0 del; 9 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
