Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8258857: Zero: non-PCH release build fails after JDK-8258074 #1894

Closed
wants to merge 2 commits into from

Conversation

shqking
Copy link
Contributor

@shqking shqking commented Dec 25, 2020

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'.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8258857: Zero: non-PCH release build fails after JDK-8258074

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1894/head:pull/1894
$ git checkout pull/1894

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'.
@bridgekeeper
Copy link

bridgekeeper bot commented Dec 25, 2020

👋 Welcome back shqking! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 25, 2020
@openjdk
Copy link

openjdk bot commented Dec 25, 2020

@shqking The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 25, 2020
@shqking
Copy link
Contributor Author

shqking commented Dec 25, 2020

/label add build

@openjdk openjdk bot added the build build-dev@openjdk.org label Dec 25, 2020
@openjdk
Copy link

openjdk bot commented Dec 25, 2020

@shqking
The build label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Dec 25, 2020

Webrevs

@DamonFool
Copy link
Member

Maybe, the title would be better with 'Zero: non-PCH release build fails after JDK-8258074'.

Just wondering why it gets passed with debug build?
Thanks.

@shqking
Copy link
Contributor Author

shqking commented Dec 26, 2020

Maybe, the title would be better with 'Zero: non-PCH release build fails after JDK-8258074'.

Hi Jie, thanks for your comment.
Yes. Agree. It's much more accurate to describe this issue with 'release' added.
Will rename it next Monday as I need ask my colleague to help me rename the title of the corresponding JBS issue (since I'm not a JDK author currently).

Just wondering why it gets passed with debug build?
Thanks.

Regarding the debug build, I guess it's because that flag 'pd_CICompileOSR' is not used for debug build.
Please refer to https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/globals_shared.hpp#L84

I suppose macro 'DECLARE_PD_DEVELOPER_FLAG' is defined at line 86 for release build where flags 'pd_XXX' would be used, while this macro is defined at line 90 for debug build where flags 'pd_XXX' are not used.

Please let me know if I misunderstand anything since I'm a beginner on OpenJDK.
Thanks. :)

@DamonFool
Copy link
Member

Regarding the debug build, I guess it's because that flag 'pd_CICompileOSR' is not used for debug build.
Please refer to https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/globals_shared.hpp#L84

I suppose macro 'DECLARE_PD_DEVELOPER_FLAG' is defined at line 86 for release build where flags 'pd_XXX' would be used, while this macro is defined at line 90 for debug build where flags 'pd_XXX' are not used.

Yes, you are right.
The change looks reasonable to me.
Thanks.

@shqking shqking changed the title 8258857: Zero: non-PCH build fails after JDK-8258074 8258857: Zero: non-PCH release build fails after JDK-8258074 Dec 28, 2020
@DamonFool
Copy link
Member

/test

@openjdk
Copy link

openjdk bot commented Dec 28, 2020

@DamonFool you need to get approval to run the tests in tier1 for commits up until 0cc564b

@openjdk openjdk bot added the test-request label Dec 28, 2020
Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iklam
Copy link
Member

iklam commented Jan 4, 2021

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

@@ -25,7 +25,7 @@
#ifndef SHARE_COMPILER_COMPILER_GLOBALS_HPP
#define SHARE_COMPILER_COMPILER_GLOBALS_HPP

#include "runtime/globals_shared.hpp"
#include "compiler/compiler_globals_pd.hpp"
#ifdef COMPILER1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include "runtime/globals_shared.hpp" should not be removed. compiler_globals.hpp uses the DECLARE_FLAGS macro, which is defined by globals_shared.hpp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 #1610 -- the vast majority of the 59 files changed in that PR were caused by relying on indirect inclusion of stubRoutines.hpp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shqking
Copy link
Contributor Author

shqking commented Jan 5, 2021

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 (in other PRs) and I suppose it's not related to our patch.

@shipilev
Copy link
Member

shipilev commented Jan 5, 2021

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.

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
Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me, but @iklam should approve.

Copy link
Member

@iklam iklam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openjdk
Copy link

openjdk bot commented Jan 5, 2021

@shqking This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8258857: Zero: non-PCH release build fails after JDK-8258074

Reviewed-by: jiefu, shade, iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 27 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@DamonFool, @shipilev, @iklam) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 5, 2021
@shqking
Copy link
Contributor Author

shqking commented Jan 5, 2021

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.

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 5, 2021
@openjdk
Copy link

openjdk bot commented Jan 5, 2021

@shqking
Your change (at version f3cd518) is now ready to be sponsored by a Committer.

@shipilev
Copy link
Member

shipilev commented Jan 5, 2021

Sponsoring to unbreak current jdk/jdk CI for me.

/sponsor

@openjdk openjdk bot closed this Jan 5, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 5, 2021
@openjdk
Copy link

openjdk bot commented Jan 5, 2021

@shipilev @shqking Since your change was applied there have been 29 commits pushed to the master branch:

  • f4122d6: 8258896: Remove the JVM ForceFloatExceptions option
  • fc3b45c: 8258643: javax/swing/JComponent/7154030/bug7154030.java failed with "Exception: Failed to hide opaque button"
  • a6c0881: 8256321: Some "inactive" color profiles use the wrong profile class
  • 9f15164: 8259049: Uninitialized variable after JDK-8257513
  • db6f393: 8251944: Add Shenandoah test config to compiler/gcbarriers/UnsafeIntrinsicsTest.java
  • 3817c32: 8258534: Epsilon: clean up unused includes
  • 17d1645: 8258751: Improve ExceptionHandlerTable dump
  • dd8996c: 8258946: Fix optimization-unstable code involving signed integer overflow
  • 5ea9607: 8258459: Decouple gc_globals.hpp from globals.hpp
  • 2499ac3: 8259069: Fields could be final
  • ... and 19 more: https://git.openjdk.java.net/jdk/compare/b575dd80b6e86821fcf3e065e31c6bddb85db6c4...master

Your commit was automatically rebased without conflicts.

Pushed as commit 82bdbfd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@shqking shqking deleted the zero-build branch January 6, 2021 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated test-request
4 participants