Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074 [v2]
On Tue, 5 Jan 2021 14:28:14 GMT, Hao Sun wrote: >> Marked as reviewed by jiefu (Committer). > > Thanks for your reviews! @DamonFool @iklam @shipilev > > The pre-submit tests were passed except the GC one which is not related to > this patch. I suppose this patch is ready to be merged now. Sponsoring to unbreak current jdk/jdk CI for me. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074 [v2]
On Tue, 5 Jan 2021 12:03:05 GMT, Jie Fu wrote: >> Hao Sun has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Header 'runtime/globals_shared.hpp' should be kept >> >> Header 'runtime/globals_shared.hpp' is kept even though >> 'compiler/compiler_globals_pd.hpp' already includes it, because >> 'compiler_globals.hpp' uses the DECLARE_FLAGS macro, which is defined by >> 'runtime/globals_shared.hpp', and it should be included directly. >> >> Besides, update the copyright year to 2021. >> >> Change-Id: Ia355f3b6e98b495dc265093e71b2d1fec1ca45ca >> CustomizedGitHooks: yes > > Marked as reviewed by jiefu (Committer). Thanks for your reviews! @DamonFool @iklam @shipilev The pre-submit tests were passed except the GC one which is not related to this patch. I suppose this patch is ready to be merged now. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074 [v2]
On Tue, 5 Jan 2021 12:06:18 GMT, Hao Sun wrote: >> From the error log we can see the root cause is that, develop_pd flag >> 'pd_CICompileOSR' is undeclared in zero build. >> >> Where this flag is used? >> Flag 'pd_CICompileOSR' is assigned to flag 'CICompileOSR'. See line 77 >> of 'compiler_globals.hpp' and further line 86 of 'globals_shared.hpp'. >> >> Where this flag can be declared? >> Header files 'c1_globals.hpp' or 'c2_globals.hpp' would be included if >> VM is built with compiler1 or compiler2. See lines 30 to 38 of >> 'complier_globals.hpp'. And further, flag 'pd_CICompileOSR' may get >> declared in the header files for specific arch, e.g., >> 'c1_globals_aarch64.hpp', 'c2_globals_aarch64.hpp'. >> However, regarding zero build (without compiler1 and compiler2 and >> jvmci) , this flag is undelcared. Hence, this patch gets header file >> 'compiler/compiler_globals_pd.hpp' included where this flag is declared >> for the case when neither COMPILER1 nor COMPILER2 are defined and >> INCLUDE_JVMCI is inactive. >> >> Note that 'compiler/compiler_globals_pd.hpp' already includes >> 'runtime/globals_shared.hpp'. >> >> Note that zero build with PCH succeeds because 'runtime/globals.hpp' is >> included in 'precompiled.hpp', and further 'compiler_globals_pd.hpp' is >> included in 'runtime/globals.hpp'. > > Hao Sun has updated the pull request incrementally with one additional commit > since the last revision: > > Header 'runtime/globals_shared.hpp' should be kept > > Header 'runtime/globals_shared.hpp' is kept even though > 'compiler/compiler_globals_pd.hpp' already includes it, because > 'compiler_globals.hpp' uses the DECLARE_FLAGS macro, which is defined by > 'runtime/globals_shared.hpp', and it should be included directly. > > Besides, update the copyright year to 2021. > > Change-Id: Ia355f3b6e98b495dc265093e71b2d1fec1ca45ca > CustomizedGitHooks: yes Marked as reviewed by shade (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074 [v2]
On Tue, 5 Jan 2021 12:06:18 GMT, Hao Sun wrote: >> From the error log we can see the root cause is that, develop_pd flag >> 'pd_CICompileOSR' is undeclared in zero build. >> >> Where this flag is used? >> Flag 'pd_CICompileOSR' is assigned to flag 'CICompileOSR'. See line 77 >> of 'compiler_globals.hpp' and further line 86 of 'globals_shared.hpp'. >> >> Where this flag can be declared? >> Header files 'c1_globals.hpp' or 'c2_globals.hpp' would be included if >> VM is built with compiler1 or compiler2. See lines 30 to 38 of >> 'complier_globals.hpp'. And further, flag 'pd_CICompileOSR' may get >> declared in the header files for specific arch, e.g., >> 'c1_globals_aarch64.hpp', 'c2_globals_aarch64.hpp'. >> However, regarding zero build (without compiler1 and compiler2 and >> jvmci) , this flag is undelcared. Hence, this patch gets header file >> 'compiler/compiler_globals_pd.hpp' included where this flag is declared >> for the case when neither COMPILER1 nor COMPILER2 are defined and >> INCLUDE_JVMCI is inactive. >> >> Note that 'compiler/compiler_globals_pd.hpp' already includes >> 'runtime/globals_shared.hpp'. >> >> Note that zero build with PCH succeeds because 'runtime/globals.hpp' is >> included in 'precompiled.hpp', and further 'compiler_globals_pd.hpp' is >> included in 'runtime/globals.hpp'. > > Hao Sun has updated the pull request incrementally with one additional commit > since the last revision: > > Header 'runtime/globals_shared.hpp' should be kept > > Header 'runtime/globals_shared.hpp' is kept even though > 'compiler/compiler_globals_pd.hpp' already includes it, because > 'compiler_globals.hpp' uses the DECLARE_FLAGS macro, which is defined by > 'runtime/globals_shared.hpp', and it should be included directly. > > Besides, update the copyright year to 2021. > > Change-Id: Ia355f3b6e98b495dc265093e71b2d1fec1ca45ca > CustomizedGitHooks: yes This looks fine to me, but @iklam should approve. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074 [v2]
On Tue, 5 Jan 2021 12:06:18 GMT, Hao Sun wrote: >> From the error log we can see the root cause is that, develop_pd flag >> 'pd_CICompileOSR' is undeclared in zero build. >> >> Where this flag is used? >> Flag 'pd_CICompileOSR' is assigned to flag 'CICompileOSR'. See line 77 >> of 'compiler_globals.hpp' and further line 86 of 'globals_shared.hpp'. >> >> Where this flag can be declared? >> Header files 'c1_globals.hpp' or 'c2_globals.hpp' would be included if >> VM is built with compiler1 or compiler2. See lines 30 to 38 of >> 'complier_globals.hpp'. And further, flag 'pd_CICompileOSR' may get >> declared in the header files for specific arch, e.g., >> 'c1_globals_aarch64.hpp', 'c2_globals_aarch64.hpp'. >> However, regarding zero build (without compiler1 and compiler2 and >> jvmci) , this flag is undelcared. Hence, this patch gets header file >> 'compiler/compiler_globals_pd.hpp' included where this flag is declared >> for the case when neither COMPILER1 nor COMPILER2 are defined and >> INCLUDE_JVMCI is inactive. >> >> Note that 'compiler/compiler_globals_pd.hpp' already includes >> 'runtime/globals_shared.hpp'. >> >> Note that zero build with PCH succeeds because 'runtime/globals.hpp' is >> included in 'precompiled.hpp', and further 'compiler_globals_pd.hpp' is >> included in 'runtime/globals.hpp'. > > Hao Sun has updated the pull request incrementally with one additional commit > since the last revision: > > Header 'runtime/globals_shared.hpp' should be kept > > Header 'runtime/globals_shared.hpp' is kept even though > 'compiler/compiler_globals_pd.hpp' already includes it, because > 'compiler_globals.hpp' uses the DECLARE_FLAGS macro, which is defined by > 'runtime/globals_shared.hpp', and it should be included directly. > > Besides, update the copyright year to 2021. > > Change-Id: Ia355f3b6e98b495dc265093e71b2d1fec1ca45ca > CustomizedGitHooks: yes LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074 [v2]
On Tue, 5 Jan 2021 00:12:58 GMT, Ioi Lam wrote: >>> `#include "runtime/globals_shared.hpp"` should not be removed. >>> compiler_globals.hpp uses the `DECLARE_FLAGS` macro, which is defined by >>> globals_shared.hpp. >> >> Since globals_shared.hpp is included in compiler_globals_pd.hpp, I think >> it's fine to remove it. > > We should not rely on indirect inclusion. Otherwise, compiler_globals.hpp > will break if compiler_globals_pd.hpp is changed to not include > globals_shared.hpp. > > In fact, I have been fixing a lot of such problems when restructuring the > header files. See https://github.com/openjdk/jdk/pull/1610 -- the vast > majority of the 59 files changed in that PR were caused by relying on > indirect inclusion of stubRoutines.hpp. Patch is updated: 1) keeping `runtime/globals_shared.hpp' included directly and 2) updating the copyright year to 2021. Could you please help to review it? @iklam @shipilev Thanks. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074 [v2]
On Tue, 5 Jan 2021 12:03:39 GMT, Hao Sun wrote: >> From the error log we can see the root cause is that, develop_pd flag >> 'pd_CICompileOSR' is undeclared in zero build. >> >> Where this flag is used? >> Flag 'pd_CICompileOSR' is assigned to flag 'CICompileOSR'. See line 77 >> of 'compiler_globals.hpp' and further line 86 of 'globals_shared.hpp'. >> >> Where this flag can be declared? >> Header files 'c1_globals.hpp' or 'c2_globals.hpp' would be included if >> VM is built with compiler1 or compiler2. See lines 30 to 38 of >> 'complier_globals.hpp'. And further, flag 'pd_CICompileOSR' may get >> declared in the header files for specific arch, e.g., >> 'c1_globals_aarch64.hpp', 'c2_globals_aarch64.hpp'. >> However, regarding zero build (without compiler1 and compiler2 and >> jvmci) , this flag is undelcared. Hence, this patch gets header file >> 'compiler/compiler_globals_pd.hpp' included where this flag is declared >> for the case when neither COMPILER1 nor COMPILER2 are defined and >> INCLUDE_JVMCI is inactive. >> >> Note that 'compiler/compiler_globals_pd.hpp' already includes >> 'runtime/globals_shared.hpp'. >> >> Note that zero build with PCH succeeds because 'runtime/globals.hpp' is >> included in 'precompiled.hpp', and further 'compiler_globals_pd.hpp' is >> included in 'runtime/globals.hpp'. > > Hao Sun has updated the pull request incrementally with one additional commit > since the last revision: > > Header 'runtime/globals_shared.hpp' should be kept > > Header 'runtime/globals_shared.hpp' is kept even though > 'compiler/compiler_globals_pd.hpp' already includes it, because > 'compiler_globals.hpp' uses the DECLARE_FLAGS macro, which is defined by > 'runtime/globals_shared.hpp', and it should be included directly. > > Besides, update the copyright year to 2021. > > Change-Id: Ia355f3b6e98b495dc265093e71b2d1fec1ca45ca > CustomizedGitHooks: yes Marked as reviewed by jiefu (Committer). - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074 [v2]
> From the error log we can see the root cause is that, develop_pd flag > 'pd_CICompileOSR' is undeclared in zero build. > > Where this flag is used? > Flag 'pd_CICompileOSR' is assigned to flag 'CICompileOSR'. See line 77 > of 'compiler_globals.hpp' and further line 86 of 'globals_shared.hpp'. > > Where this flag can be declared? > Header files 'c1_globals.hpp' or 'c2_globals.hpp' would be included if > VM is built with compiler1 or compiler2. See lines 30 to 38 of > 'complier_globals.hpp'. And further, flag 'pd_CICompileOSR' may get > declared in the header files for specific arch, e.g., > 'c1_globals_aarch64.hpp', 'c2_globals_aarch64.hpp'. > However, regarding zero build (without compiler1 and compiler2 and > jvmci) , this flag is undelcared. Hence, this patch gets header file > 'compiler/compiler_globals_pd.hpp' included where this flag is declared > for the case when neither COMPILER1 nor COMPILER2 are defined and > INCLUDE_JVMCI is inactive. > > Note that 'compiler/compiler_globals_pd.hpp' already includes > 'runtime/globals_shared.hpp'. > > Note that zero build with PCH succeeds because 'runtime/globals.hpp' is > included in 'precompiled.hpp', and further 'compiler_globals_pd.hpp' is > included in 'runtime/globals.hpp'. Hao Sun has updated the pull request incrementally with one additional commit since the last revision: Header 'runtime/globals_shared.hpp' should be kept Header 'runtime/globals_shared.hpp' is kept even though 'compiler/compiler_globals_pd.hpp' already includes it, because 'compiler_globals.hpp' uses the DECLARE_FLAGS macro, which is defined by 'runtime/globals_shared.hpp', and it should be included directly. Besides, update the copyright year to 2021. Change-Id: Ia355f3b6e98b495dc265093e71b2d1fec1ca45ca CustomizedGitHooks: yes - Changes: - all: https://git.openjdk.java.net/jdk/pull/1894/files - new: https://git.openjdk.java.net/jdk/pull/1894/files/0cc564bd..f3cd5181 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1894&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1894&range=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1894.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1894/head:pull/1894 PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Tue, 5 Jan 2021 04:54:32 GMT, Hao Sun wrote: > The tests were finished but one of them failed. I found that this failure > existed for several days (in other PRs) and I suppose it's not related to our > patch. Yes, the failure you caught does not look related to this PR. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Tue, 5 Jan 2021 00:12:58 GMT, Ioi Lam wrote: >>> `#include "runtime/globals_shared.hpp"` should not be removed. >>> compiler_globals.hpp uses the `DECLARE_FLAGS` macro, which is defined by >>> globals_shared.hpp. >> >> Since globals_shared.hpp is included in compiler_globals_pd.hpp, I think >> it's fine to remove it. > > We should not rely on indirect inclusion. Otherwise, compiler_globals.hpp > will break if compiler_globals_pd.hpp is changed to not include > globals_shared.hpp. > > In fact, I have been fixing a lot of such problems when restructuring the > header files. See https://github.com/openjdk/jdk/pull/1610 -- the vast > majority of the 59 files changed in that PR were caused by relying on > indirect inclusion of stubRoutines.hpp. Yes, if something is using the definitions from the header, that header should be included directly. So, the patch is just adding the `#include "compiler/compiler_globals_pd.hpp"` then, right? - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Mon, 4 Jan 2021 17:19:48 GMT, Ioi Lam wrote: >> From the error log we can see the root cause is that, develop_pd flag >> 'pd_CICompileOSR' is undeclared in zero build. >> >> Where this flag is used? >> Flag 'pd_CICompileOSR' is assigned to flag 'CICompileOSR'. See line 77 >> of 'compiler_globals.hpp' and further line 86 of 'globals_shared.hpp'. >> >> Where this flag can be declared? >> Header files 'c1_globals.hpp' or 'c2_globals.hpp' would be included if >> VM is built with compiler1 or compiler2. See lines 30 to 38 of >> 'complier_globals.hpp'. And further, flag 'pd_CICompileOSR' may get >> declared in the header files for specific arch, e.g., >> 'c1_globals_aarch64.hpp', 'c2_globals_aarch64.hpp'. >> However, regarding zero build (without compiler1 and compiler2 and >> jvmci) , this flag is undelcared. Hence, this patch gets header file >> 'compiler/compiler_globals_pd.hpp' included where this flag is declared >> for the case when neither COMPILER1 nor COMPILER2 are defined and >> INCLUDE_JVMCI is inactive. >> >> Note that 'compiler/compiler_globals_pd.hpp' already includes >> 'runtime/globals_shared.hpp'. >> >> Note that zero build with PCH succeeds because 'runtime/globals.hpp' is >> included in 'precompiled.hpp', and further 'compiler_globals_pd.hpp' is >> included in 'runtime/globals.hpp'. > > Changes requested by iklam (Reviewer). > It is unclear to me why the original change in JDK-8258074 included > `compiler_globals_pd.hpp` in `globals.hpp` at all, @iklam? It seems what > @shqking proposes is sound. > > I believe we should additionally change the `#include > compiler/compiler_globals_pd.hpp` to `#include compiler/compiler_globals.hpp` > in `globals.hpp`? > > @shqking, please go to https://github.com/shqking/jdk/actions -- and enable > GH Actions, then trigger the workflow manually on your branch to get this > tested. Thanks for your comment. @shipilev The tests were finished but one of them failed. I found that this failure existed for several days and I suppose it's not related to our patch. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Mon, 4 Jan 2021 23:39:18 GMT, Jie Fu wrote: >> src/hotspot/share/compiler/compiler_globals.hpp line 29: >> >>> 27: >>> 28: #include "compiler/compiler_globals_pd.hpp" >>> 29: #ifdef COMPILER1 >> >> `#include "runtime/globals_shared.hpp"` should not be removed. >> compiler_globals.hpp uses the `DECLARE_FLAGS` macro, which is defined by >> globals_shared.hpp. > >> `#include "runtime/globals_shared.hpp"` should not be removed. >> compiler_globals.hpp uses the `DECLARE_FLAGS` macro, which is defined by >> globals_shared.hpp. > > Since globals_shared.hpp is included in compiler_globals_pd.hpp, I think it's > fine to remove it. We should not rely on indirect inclusion. Otherwise, compiler_globals.hpp will break if compiler_globals_pd.hpp is changed to not include globals_shared.hpp. In fact, I have been fixing a lot of such problems when restructuring the header files. See https://github.com/openjdk/jdk/pull/1610 -- the vast majority of the 59 files changed in that PR were caused by relying on indirect inclusion of stubRoutines.hpp. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Mon, 4 Jan 2021 17:19:44 GMT, Ioi Lam wrote: > `#include "runtime/globals_shared.hpp"` should not be removed. > compiler_globals.hpp uses the `DECLARE_FLAGS` macro, which is defined by > globals_shared.hpp. Since globals_shared.hpp is included in compiler_globals_pd.hpp, I think it's fine to remove it. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Mon, 4 Jan 2021 10:09:23 GMT, Aleksey Shipilev wrote: > It is unclear to me why the original change in JDK-8258074 included > `compiler_globals_pd.hpp` in `globals.hpp` at all, @iklam? The reason is, for historical reasons, some PD flags related to the compiler, such as `BackgroundCompilation`, are declared in `globals.hpp`. As a result, `globals.hpp` must include `compiler_globals_pd.hpp`, which provides the platform-specific default value for `BackgroundCompilation`. This should eventually be fixed by moving the declaration of these flags to compiler_globals.hpp instead. > I believe we should additionally change the `#include > compiler/compiler_globals_pd.hpp` to `#include compiler/compiler_globals.hpp` > in `globals.hpp`? This is not necessary. `globals.hpp` does not use anything declared in `compiler_globals.hpp` - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Fri, 25 Dec 2020 10:11:13 GMT, Hao Sun wrote: > From the error log we can see the root cause is that, develop_pd flag > 'pd_CICompileOSR' is undeclared in zero build. > > Where this flag is used? > Flag 'pd_CICompileOSR' is assigned to flag 'CICompileOSR'. See line 77 > of 'compiler_globals.hpp' and further line 86 of 'globals_shared.hpp'. > > Where this flag can be declared? > Header files 'c1_globals.hpp' or 'c2_globals.hpp' would be included if > VM is built with compiler1 or compiler2. See lines 30 to 38 of > 'complier_globals.hpp'. And further, flag 'pd_CICompileOSR' may get > declared in the header files for specific arch, e.g., > 'c1_globals_aarch64.hpp', 'c2_globals_aarch64.hpp'. > However, regarding zero build (without compiler1 and compiler2 and > jvmci) , this flag is undelcared. Hence, this patch gets header file > 'compiler/compiler_globals_pd.hpp' included where this flag is declared > for the case when neither COMPILER1 nor COMPILER2 are defined and > INCLUDE_JVMCI is inactive. > > Note that 'compiler/compiler_globals_pd.hpp' already includes > 'runtime/globals_shared.hpp'. > > Note that zero build with PCH succeeds because 'runtime/globals.hpp' is > included in 'precompiled.hpp', and further 'compiler_globals_pd.hpp' is > included in 'runtime/globals.hpp'. Changes requested by iklam (Reviewer). src/hotspot/share/compiler/compiler_globals.hpp line 29: > 27: > 28: #include "compiler/compiler_globals_pd.hpp" > 29: #ifdef COMPILER1 `#include "runtime/globals_shared.hpp"` should not be removed. compiler_globals.hpp uses the `DECLARE_FLAGS` macro, which is defined by globals_shared.hpp. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Fri, 25 Dec 2020 10:11:13 GMT, Hao Sun wrote: > From the error log we can see the root cause is that, develop_pd flag > 'pd_CICompileOSR' is undeclared in zero build. > > Where this flag is used? > Flag 'pd_CICompileOSR' is assigned to flag 'CICompileOSR'. See line 77 > of 'compiler_globals.hpp' and further line 86 of 'globals_shared.hpp'. > > Where this flag can be declared? > Header files 'c1_globals.hpp' or 'c2_globals.hpp' would be included if > VM is built with compiler1 or compiler2. See lines 30 to 38 of > 'complier_globals.hpp'. And further, flag 'pd_CICompileOSR' may get > declared in the header files for specific arch, e.g., > 'c1_globals_aarch64.hpp', 'c2_globals_aarch64.hpp'. > However, regarding zero build (without compiler1 and compiler2 and > jvmci) , this flag is undelcared. Hence, this patch gets header file > 'compiler/compiler_globals_pd.hpp' included where this flag is declared > for the case when neither COMPILER1 nor COMPILER2 are defined and > INCLUDE_JVMCI is inactive. > > Note that 'compiler/compiler_globals_pd.hpp' already includes > 'runtime/globals_shared.hpp'. > > Note that zero build with PCH succeeds because 'runtime/globals.hpp' is > included in 'precompiled.hpp', and further 'compiler_globals_pd.hpp' is > included in 'runtime/globals.hpp'. It is unclear to me why the original change in JDK-8258074 included `compiler_globals_pd.hpp` in `globals.hpp` at all, @iklam? It seems what @shqking proposes is sound. I believe we should additionally change the `#include compiler/compiler_globals_pd.hpp` to `#include compiler/compiler_globals.hpp` in `globals.hpp`? @shqking, please go to https://github.com/shqking/jdk/actions -- and enable GH Actions, then trigger the workflow manually on your branch to get this tested. - Changes requested by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1894
