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.

Sign up for GitHub

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

Jump to bottom

8295435: Build failure with GCC7 after JDK-8294314 due to strict-overflow warnings #10738

Closed
wants to merge 2 commits into from
Closed

8295435: Build failure with GCC7 after JDK-8294314 due to strict-overflow warnings #10738

wants to merge 2 commits into from

Conversation

DamonFool
Copy link
Member

@DamonFool DamonFool commented Oct 18, 2022 •
edited by openjdk bot
Loading

Hi all,

Let's disable -Werror=strict-overflow to get it build with gcc7 on Linux/x86.

bytecodeAssembler.cpp
instanceKlass.cpp
klassVtable.cpp

Thanks.
Best regards,
Jie

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8295435: Build failure with GCC7 after JDK-8294314 due to strict-overflow warnings

Reviewers

Contributors

  • Aleksey Shipilev <shade@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10738/head:pull/10738
$ git checkout pull/10738

Update a local copy of the PR:
$ git checkout pull/10738
$ git pull https://git.openjdk.org/jdk pull/10738/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10738

View PR using the GUI difftool:
$ git pr show -t 10738

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10738.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 18, 2022

👋 Welcome back jiefu! 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 Oct 18, 2022
@openjdk
Copy link

openjdk bot commented Oct 18, 2022

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

  • build

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 build build-dev@openjdk.org label Oct 18, 2022
@mlbridge
Copy link

mlbridge bot commented Oct 18, 2022 •
edited
Loading

Webrevs

shipilev
shipilev reviewed Oct 18, 2022
View reviewed changes
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.

My CI reports the mainline failures with GCC 6 in the following configs:

linux-x86_64-server-fastdebug
linux-aarch64-server-fastdebug
linux-arm-server-fastdebug
linux-ppc64le-server-fastdebug
linux-s390x-server-fastdebug
linux-ppc64-server-fastdebug

This PR solves part of the failures, but some require more work:

diff --git a/make/hotspot/lib/CompileJvm.gmk b/make/hotspot/lib/CompileJvm.gmk
index 77d1ff6e21c..816eb4d9996 100644
--- a/make/hotspot/lib/CompileJvm.gmk
+++ b/make/hotspot/lib/CompileJvm.gmk
@@ -166,2 +166,3 @@ $(eval $(call SetupJdkLibrary, BUILD_LIBJVM, \
     DISABLED_WARNINGS_gcc_loopnode.cpp := sequence-point, \
+    DISABLED_WARNINGS_gcc_objArrayKlass.cpp := strict-overflow, \
     DISABLED_WARNINGS_gcc_postaloc.cpp := address, \

Since the warning seems to be caused by the popular header, it is likely to break some other platforms/files through the different include paths from the different compilation units. The warning also looks fairly dubious, and probably is GCC bug, as it does not trigger with higher GCCs? Because of this, I suggest we add strict-overflow back to the global warning exclusion list:

diff --git a/make/hotspot/lib/CompileJvm.gmk b/make/hotspot/lib/CompileJvm.gmk
index 6d4e01d1aed..534c50cc77a 100644
--- a/make/hotspot/lib/CompileJvm.gmk
+++ b/make/hotspot/lib/CompileJvm.gmk
@@ -94,6 +94,8 @@ DISABLED_WARNINGS_clang := ignored-qualifiers sometimes-uninitialized \
 ifneq ($(DEBUG_LEVEL), release)
   # Assert macro gives warning
   DISABLED_WARNINGS_clang += tautological-constant-out-of-range-compare
+  # Some reasonable asserts produce warnings on GCC <= 7
+  DISABLED_WARNINGS_gcc += strict-overflow
 endif
 
 DISABLED_WARNINGS_xlc := tautological-compare shift-negative-value

@DamonFool
Copy link
Member Author

/contributor add shipilev

@DamonFool
Copy link
Member Author

Since the warning seems to be caused by the popular header, it is likely to break some other platforms/files through the different include paths from the different compilation units. The warning also looks fairly dubious, and probably is GCC bug, as it does not trigger with higher GCCs? Because of this, I suggest we add strict-overflow back to the global warning exclusion list

Good suggestion.
Updated.
Thanks.

@openjdk
Copy link

openjdk bot commented Oct 19, 2022

@DamonFool shipilev was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

@DamonFool
Copy link
Member Author

/contributor add shade

@openjdk
Copy link

openjdk bot commented Oct 19, 2022

@DamonFool
Contributor Aleksey Shipilev <shade@openjdk.org> successfully added.

shipilev
shipilev approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@shipilev shipilev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am good with this. Let's hear from @magicus too.

@openjdk
Copy link

openjdk bot commented Oct 19, 2022 •
edited
Loading

@DamonFool 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:

8295435: Build failure with GCC7 after JDK-8294314 due to strict-overflow warnings

Co-authored-by: Aleksey Shipilev <shade@openjdk.org>
Reviewed-by: shade, ihse

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

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 19, 2022
magicus
magicus approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

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

That's fine, I guess. We have no easy mechanism for testing a condition like "gcc 7 or older" in the makefiles. I hope we raise the bar for gcc soon, and then we can go back and remove this.

@DamonFool
Copy link
Member Author

Thanks @shipilev and @magicus for the review.
/integrate

@openjdk
Copy link

openjdk bot commented Oct 19, 2022

Going to push as commit f502ab8.
Since your change was applied there have been 35 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 19, 2022
@openjdk openjdk bot closed this Oct 19, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 19, 2022
@openjdk
Copy link

openjdk bot commented Oct 19, 2022

@DamonFool Pushed as commit f502ab8.

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

@DamonFool DamonFool deleted the JDK-8295435 branch October 19, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shipilev shipilev shipilev approved these changes

@magicus magicus magicus approved these changes

Assignees
No one assigned
Labels
build build-dev@openjdk.org integrated Pull request has been integrated
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DamonFool @shipilev @magicus