Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577
On Mon, 11 Dec 2023 02:36:30 GMT, Guoxiong Li wrote: > Hi all, > > This patch fixes the building failure introduced by > [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC version > (linux & GCC 7.5.0 locally). > > Thanks for the review. > > Best Regards, > -- Guoxiong Just because a code change cause a compilation error, it does not mean that it is relevant for the build group. - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1850175802
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577
On Mon, 11 Dec 2023 02:36:30 GMT, Guoxiong Li wrote: > Hi all, > > This patch fixes the building failure introduced by > [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC version > (linux & GCC 7.5.0 locally). > > Thanks for the review. > > Best Regards, > -- Guoxiong Please hold off on this change while I follow up on JDK-8319577. This code requires C++17, which was turned on for a relevant small subset of the JDK by that change. There was some discussion there about version limiting that change, but that doesn't seem to have been done, with this breakage being a consequence. - PR Review: https://git.openjdk.org/jdk/pull/17047#pullrequestreview-1776491102
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577
On Tue, 12 Dec 2023 02:31:58 GMT, Kim Barrett wrote:
>> Hi all,
>>
>> This patch fixes the building failure introduced by
>> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC
>> version (linux & GCC 7.5.0 locally).
>>
>> Thanks for the review.
>>
>> Best Regards,
>> -- Guoxiong
>
> Please hold off on this change while I follow up on JDK-8319577. This code
> requires C++17, which was turned on for a relevant small subset of the JDK by
> that change. There was some discussion there about version limiting that
> change, but that doesn't seem to have been done, with this breakage being a
> consequence.
@kimbarrett There were a long discussion about the C++17 support. The end
result, which led me to approve the build changes, was the claims made
beginning with https://github.com/openjdk/jdk/pull/16534#discussion_r1414570644:
> [...] the updated code which added guards to check for GCC version >= 7.5 in
> src/java.base/linux/native/libsimdsort/{avx2-linux-qsort.cpp,
> avx512-linux-qsort.cpp}. GCC >= 7.5 is needed to compile libsimdsort using
> C++17 features. Made sure that OpenJDK builds without errors using both GCC
> 7.5 and GCC 6.4.
> The GCC complier for versions 6 (and even 5) silently ignores the flag
> -std=c++17
Somehow the JDK compiled with 7.5 for the author of that patch, but fails for
the author of this patch. I don't understand how this both can be true.
-
PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1852290498
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577
On Tue, 12 Dec 2023 15:40:19 GMT, Magnus Ihse Bursie wrote: > Somehow the JDK compiled with 7.5 for the author of that patch, but fails for > the author of this patch. I don't understand how this both can be true. I noticed my previous build is `slowdebug`. Today, I tested the `release` and `fastdebug` locally. These builds succeeded. So only the `slowdebug` build fails. What's the difference between `fastdebug` and `slowdebug`? It seems that `slow-debug` disables some optimizations, such as constant propagation, which may cause this issue. Should we adjust the concrete code (like this PR) or revise the build system to avoid the potential problem in the future? - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1853142192
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577
On Mon, 11 Dec 2023 02:36:30 GMT, Guoxiong Li wrote: > Hi all, > > This patch fixes the building failure introduced by > [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC version > (linux & GCC 7.5.0 locally). > > Thanks for the review. > > Best Regards, > -- Guoxiong The files being proposed for change are cloned copies of a 3rd-party library. We may want to take updates from that library in the future. Having local modifications makes that more difficult. Also, this library uses C++17. GCC support for C++17 was experimental until GCC 9. As such, it shouldn't be entirely surprising if an earlier version of GCC might run into problems with it. We've been discussing switching to using C++17 JDK-wide for some time, and I think that may happen soon. At that point the minimum supported version of GCC will be changed to at least GCC 9. So I think the answer here is that building this lbrary with GCC 7.5 is not supported and a newer version should be used. So the change should be here: https://github.com/openjdk/jdk/blob/b31454e36234091c3827c3b4d07f62345cb0cee4/src/java.base/linux/native/libsimdsort/simdsort-support.hpp#L34-L37 to require a later GCC version. That way, if building with an older version of GCC then the code in the library won't be built and the library won't be used by the JDK (because the relevant symbols won't be found). But GCC 7.5 will probably not be supported for building the JDK for much longer. - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1857502174
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577
On Fri, 15 Dec 2023 08:48:59 GMT, Kim Barrett wrote: >> Hi all, >> >> This patch fixes the building failure introduced by >> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC >> version (linux & GCC 7.5.0 locally). >> >> Thanks for the review. >> >> Best Regards, >> -- Guoxiong > > The files being proposed for change are cloned copies of a 3rd-party library. > We may want to take updates from that library in the future. Having local > modifications makes that more difficult. > > Also, this library uses C++17. GCC support for C++17 was experimental until > GCC 9. As such, it shouldn't be entirely surprising if an earlier version of > GCC might run into problems with it. > > We've been discussing switching to using C++17 JDK-wide for some time, and I > think that may happen soon. At that point the minimum supported version of > GCC will be changed to at least GCC 9. > > So I think the answer here is that building this lbrary with GCC 7.5 is not > supported and a newer version should be used. So the change should be here: > https://github.com/openjdk/jdk/blob/b31454e36234091c3827c3b4d07f62345cb0cee4/src/java.base/linux/native/libsimdsort/simdsort-support.hpp#L34-L37 > to require a later GCC version. That way, if building with an older version of > GCC then the code in the library won't be built and the library won't be used > by the JDK (because the relevant symbols won't be found). > > But GCC 7.5 will probably not be supported for building the JDK for much > longer. @kimbarrett Thanks for the guide. I updated the code just now. All the `release`, `fastdebug` and `slowdebug` builds succeeded locally with GCC7.5. - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1859172217
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
> Hi all, > > This patch fixes the building failure introduced by > [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC version > (linux & GCC 7.5.0 locally). > > Thanks for the review. > > Best Regards, > -- Guoxiong Guoxiong Li has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Bump the needed version of GCC. - Revert previous change. - Merge branch 'master' into JDK-8321688 - JDK-8321688 - Changes: - all: https://git.openjdk.org/jdk/pull/17047/files - new: https://git.openjdk.org/jdk/pull/17047/files/46858210..f04bcd55 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17047&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17047&range=00-01 Stats: 876 lines in 56 files changed: 520 ins; 166 del; 190 mod Patch: https://git.openjdk.org/jdk/pull/17047.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17047/head:pull/17047 PR: https://git.openjdk.org/jdk/pull/17047
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Sun, 17 Dec 2023 13:25:00 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch fixes the building failure introduced by >> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC >> version (linux & GCC 7.5.0 locally). >> >> Thanks for the review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Bump the needed version of GCC. > - Revert previous change. > - Merge branch 'master' into JDK-8321688 > - JDK-8321688 src/java.base/linux/native/libsimdsort/simdsort-support.hpp line 35: > 33: > 34: // GCC >= 9.1 is needed to build AVX2 portions of libsimdsort using C++17 > features > 35: #if defined(_LP64) && (defined(__GNUC__) && ((__GNUC__ > 9) || ((__GNUC__ > == 9) && (__GNUC_MINOR__ >= 1 Have you tested with gcc 9? Or is this just supposition based on gcc9 having removed the experimental status for C++17? - PR Review Comment: https://git.openjdk.org/jdk/pull/17047#discussion_r1430308457
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Sun, 17 Dec 2023 13:25:00 GMT, Guoxiong Li wrote:
>> Hi all,
>>
>> This patch fixes the building failure introduced by
>> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC
>> version (linux & GCC 7.5.0 locally).
>>
>> Thanks for the review.
>>
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains four additional
> commits since the last revision:
>
> - Bump the needed version of GCC.
> - Revert previous change.
> - Merge branch 'master' into JDK-8321688
> - JDK-8321688
Let us restrict the libsimdsort.so build to GCC 8.4.0 with the following change:
`diff --git a/src/java.base/linux/native/libsimdsort/simdsort-support.hpp
b/src/java.base/linux/native/libsimdsort/simdsort-support.hpp
index c73fd7920d8..2a2946cff82 100644
--- a/src/java.base/linux/native/libsimdsort/simdsort-support.hpp
+++ b/src/java.base/linux/native/libsimdsort/simdsort-support.hpp
@@ -31,9 +31,9 @@
#define assert(cond, msg) { if (!(cond)) { fprintf(stderr, "assert fails %s
%d: %s\n", __FILE__, __LINE__, msg); abort(); }}
-// GCC >= 7.5 is needed to build AVX2 portions of libsimdsort using C++17
features
-#if defined(_LP64) && (defined(__GNUC__) && ((__GNUC__ > 7) || ((__GNUC__ ==
7) && (__GNUC_MINOR__ >= 5
+// GCC >= 8.4 is needed to build AVX2 portions of libsimdsort using C++17
features
+#if defined(_LP64) && (defined(__GNUC__) && ((__GNUC__ > 8) || ((__GNUC__ ==
8) && (__GNUC_MINOR__ >= 4
#define __SIMDSORT_SUPPORTED_LINUX
#endif
-#endif //SIMDSORT_SUPPORT_HPP
\ No newline at end of file
+#endif //SIMDSORT_SUPPORT_HPP`
I have verified that with the above change the builds (release, fastdebug,
slowdebug) all succeed with GCC 8.4.0 as well as prior GCC like GCC7.5.0 and
the test/jdk/java/util/Arrays/Sorting.java passes successfully with these
builds.
-
PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1861889879
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Sun, 17 Dec 2023 13:25:00 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch fixes the building failure introduced by >> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC >> version (linux & GCC 7.5.0 locally). >> >> Thanks for the review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Bump the needed version of GCC. > - Revert previous change. > - Merge branch 'master' into JDK-8321688 > - JDK-8321688 > Have you tested with gcc 9? Or is this just supposition based on gcc9 having > removed the experimental status for C++17? I have not tested GCC 8 and 9. @sviswa7 seems to test them. > I have verified that with the above change the builds (release, fastdebug, > slowdebug) all succeed with GCC 8.4.0 as well as prior GCC like GCC7.5.0 and > the test/jdk/java/util/Arrays/Sorting.java passes successfully with these > builds. Thanks for your tests. But from the description of the [GCC document](https://gcc.gnu.org/projects/cxx-status.html), shown below, it may be good to skip GCC 8 and use GCC 9 directly if we want to switch to C++17. > Some C++17 features are available since GCC 5, but support was experimental > and the ABI of C++17 features was not stable until GCC 9. What do you think about it? - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1861998852
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Tue, 19 Dec 2023 02:22:05 GMT, Guoxiong Li wrote: >> Guoxiong Li has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Bump the needed version of GCC. >> - Revert previous change. >> - Merge branch 'master' into JDK-8321688 >> - JDK-8321688 > >> Have you tested with gcc 9? Or is this just supposition based on gcc9 having >> removed the experimental > status for C++17? > > I have not tested GCC 8 and 9. @sviswa7 seems to test them. > >> I have verified that with the above change the builds (release, fastdebug, >> slowdebug) all succeed with GCC 8.4.0 as well as prior GCC like GCC7.5.0 and >> the test/jdk/java/util/Arrays/Sorting.java passes successfully with these >> builds. > > Thanks for your tests. But from the description of the [GCC > document](https://gcc.gnu.org/projects/cxx-status.html), shown below, it may > be good to skip GCC 8 and use GCC 9 directly if we want to switch to C++17. > >> Some C++17 features are available since GCC 5, but support was experimental >> and the ABI of C++17 features was not stable until GCC 9. > > What do you think about it? @lgxbslgx We would like to keep GCC 8.4.0 as the minimum. - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1863127078
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Tue, 19 Dec 2023 02:22:05 GMT, Guoxiong Li wrote: >> Guoxiong Li has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Bump the needed version of GCC. >> - Revert previous change. >> - Merge branch 'master' into JDK-8321688 >> - JDK-8321688 > >> Have you tested with gcc 9? Or is this just supposition based on gcc9 having >> removed the experimental > status for C++17? > > I have not tested GCC 8 and 9. @sviswa7 seems to test them. > >> I have verified that with the above change the builds (release, fastdebug, >> slowdebug) all succeed with GCC 8.4.0 as well as prior GCC like GCC7.5.0 and >> the test/jdk/java/util/Arrays/Sorting.java passes successfully with these >> builds. > > Thanks for your tests. But from the description of the [GCC > document](https://gcc.gnu.org/projects/cxx-status.html), shown below, it may > be good to skip GCC 8 and use GCC 9 directly if we want to switch to C++17. > >> Some C++17 features are available since GCC 5, but support was experimental >> and the ABI of C++17 features was not stable until GCC 9. > > What do you think about it? > @lgxbslgx We would like to keep GCC 8.4.0 as the minimum. Why? That's likely going to be in conflict with https://github.com/openjdk/jdk/pull/14988. - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1863330523
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Tue, 19 Dec 2023 19:08:08 GMT, Kim Barrett wrote: >>> Have you tested with gcc 9? Or is this just supposition based on gcc9 >>> having removed the experimental >> status for C++17? >> >> I have not tested GCC 8 and 9. @sviswa7 seems to test them. >> >>> I have verified that with the above change the builds (release, fastdebug, >>> slowdebug) all succeed with GCC 8.4.0 as well as prior GCC like GCC7.5.0 >>> and the test/jdk/java/util/Arrays/Sorting.java passes successfully with >>> these builds. >> >> Thanks for your tests. But from the description of the [GCC >> document](https://gcc.gnu.org/projects/cxx-status.html), shown below, it may >> be good to skip GCC 8 and use GCC 9 directly if we want to switch to C++17. >> >>> Some C++17 features are available since GCC 5, but support was >>> experimental and the ABI of C++17 features was not stable until GCC 9. >> >> What do you think about it? > >> @lgxbslgx We would like to keep GCC 8.4.0 as the minimum. > > Why? That's likely going to be in conflict with > https://github.com/openjdk/jdk/pull/14988. @kimbarrett I meant to say that since the libsimdsort works with GCC 8.4.0, the #define guard in libsimdsort sources could be restricted to just that and we don't have to artificially raise it to GCC 9. Do you think that is an issue? - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1863367786
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Sun, 17 Dec 2023 13:25:00 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch fixes the building failure introduced by >> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC >> version (linux & GCC 7.5.0 locally). >> >> Thanks for the review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Bump the needed version of GCC. > - Revert previous change. > - Merge branch 'master' into JDK-8321688 > - JDK-8321688 Looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17047#pullrequestreview-1789856820
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Tue, 19 Dec 2023 19:08:08 GMT, Kim Barrett wrote: >>> Have you tested with gcc 9? Or is this just supposition based on gcc9 >>> having removed the experimental >> status for C++17? >> >> I have not tested GCC 8 and 9. @sviswa7 seems to test them. >> >>> I have verified that with the above change the builds (release, fastdebug, >>> slowdebug) all succeed with GCC 8.4.0 as well as prior GCC like GCC7.5.0 >>> and the test/jdk/java/util/Arrays/Sorting.java passes successfully with >>> these builds. >> >> Thanks for your tests. But from the description of the [GCC >> document](https://gcc.gnu.org/projects/cxx-status.html), shown below, it may >> be good to skip GCC 8 and use GCC 9 directly if we want to switch to C++17. >> >>> Some C++17 features are available since GCC 5, but support was >>> experimental and the ABI of C++17 features was not stable until GCC 9. >> >> What do you think about it? > >> @lgxbslgx We would like to keep GCC 8.4.0 as the minimum. > > Why? That's likely going to be in conflict with > https://github.com/openjdk/jdk/pull/14988. > @kimbarrett I meant to say that since the libsimdsort works with GCC 8.4.0, > the #define guard in libsimdsort sources could be restricted to just that and > we don't have to artificially raise it to GCC 9. Do you think that is an > issue? I don't think we should be depending on an experimental compiler feature when there are alternatives. - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1863608489
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Sun, 17 Dec 2023 13:25:00 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch fixes the building failure introduced by >> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC >> version (linux & GCC 7.5.0 locally). >> >> Thanks for the review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Bump the needed version of GCC. > - Revert previous change. > - Merge branch 'master' into JDK-8321688 > - JDK-8321688 Marked as reviewed by sviswanathan (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17047#pullrequestreview-1789880110
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Tue, 19 Dec 2023 23:23:28 GMT, Kim Barrett wrote: >>> @lgxbslgx We would like to keep GCC 8.4.0 as the minimum. >> >> Why? That's likely going to be in conflict with >> https://github.com/openjdk/jdk/pull/14988. > >> @kimbarrett I meant to say that since the libsimdsort works with GCC 8.4.0, >> the #define guard in libsimdsort sources could be restricted to just that >> and we don't have to artificially raise it to GCC 9. Do you think that is an >> issue? > > I don't think we should be depending on an experimental compiler feature when > there are alternatives. @kimbarrett @sviswa7 Thanks for your reviews. Integrating. - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1863808489
