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

8262017: C2: assert(n != __null) failed: Bad immediate dominator info. #528

Closed
wants to merge 3 commits into from

Conversation

franferrax
Copy link
Contributor

@franferrax franferrax commented Jun 26, 2024

JDK-8262017 backport

Hi, this is a backport of JDK-8262017: C2: assert(n != __null) failed: Bad immediate dominator info. As explained in the 11u backport (openjdk/jdk11u-dev#156), this also introduces parts from JDK-8244504.

This backport is not clean because of the changed paths (JDK-8187443) and the removal of the Compile* C parameter from the nodes allocation new operator (JDK-8034812). Besides the conflicts, new code in opto/addnode.cpp had to be adjusted to introduce the mentioned Compile* C parameter.

Finally, the opto/movenode.hpp include in opto/addnode.cpp had to be removed. opto/movenode.hpp doesn't exist in 8u, and CMoveLNode is defined in opto/connode.hpp instead, which is already included by opto/addnode.cpp.

Apart from those cosmetic adjustments, the change is not substantially different from openjdk/jdk11u-dev@293d44f.

Testing

  • hotspot_tier1 showed no regression with a Linux x86-64 slowdebug build
    • hotspot/test/compiler/ciReplay/TestSA.sh was the only failing test, with and without this PR changes
  • In addition, the new hotspot/test/compiler/rangechecks/TestRangeCheckLimits.java is passing

JDK-8205407 backport

As part of this patch, the following code caused warning C4800 in Windows x86, treated as error C2220:

bool is_int = gvn.type(a)->isa_int();

bool is_int = gvn.type(a)->isa_int();

Build log:

...\opto\addnode.cpp(838) : error C2220: warning treated as error - no 'object' file generated
...\opto\addnode.cpp(838) : warning C4800: 'const TypeInt *' : forcing value to bool 'true' or 'false' (performance warning)
...\opto\addnode.cpp(889) : warning C4800: 'const TypeInt *' : forcing value to bool 'true' or 'false' (performance warning)

My personal preference would have been to fix it in the code, but this didn't happen in neither the original code nor the 11u backport. The reason is that this warning is ignored since JDK 11's JDK-8205407 (openjdk/jdk11u-dev@8c5dfa2). We can see it's still ignored in mainline, where DISABLED_WARNINGS unifies WARNING_CFLAGS_JDK and WARNING_CFLAGS_JVM since JDK 12's JDK-8210988 (openjdk/jdk12u@09a967a).

In order to keep coherence with the newer code and ease any future backport facing the same issue, I decided to include a JDK-8205407 (openjdk/jdk11u-dev@8c5dfa2) backport in this pull request. Since WARNING_CFLAGS_JVM was introduced in JDK 11 by JDK-8198724 (openjdk/jdk11u-dev@b64d0ef), after JDK 9's JDK-8152666 (openjdk/jdk9u@e709aa2), the right placement for ignoring this warning tracks down to hotspot/make/windows/makefiles/compile.make.


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
  • JDK-8205407 needs maintainer approval
  • JDK-8262017 needs maintainer approval

Issues

  • JDK-8262017: C2: assert(n != __null) failed: Bad immediate dominator info. (Bug - P3 - Approved)
  • JDK-8205407: [windows, vs<2017] C4800 after 8203197 (Bug - P4 - Approved)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/528/head:pull/528
$ git checkout pull/528

Update a local copy of the PR:
$ git checkout pull/528
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/528/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 528

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/528.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 26, 2024

👋 Welcome back fferrari! 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
Copy link

openjdk bot commented Jun 26, 2024

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

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

8262017: C2: assert(n != __null) failed: Bad immediate dominator info.
8205407: [windows, vs<2017] C4800 after 8203197

Reviewed-by: mbalao, roland

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 2 new commits pushed to the master branch:

  • 44eac48: 8137329: [windows] Build broken on VS2010 after "8046148: JEP 158: Unified JVM Logging"
  • a0715ab: 8238274: (sctp) JDK-7118373 is not fixed for SctpChannel

Please see this link for an up-to-date comparison between the source branch of this pull request and 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 (@martinuy, @rwestrel) 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 changed the title Backport 2db9005c07585b580b3ec0889b8b5e3ed0d0ca6a 8262017: C2: assert(n != __null) failed: Bad immediate dominator info. Jun 26, 2024
@openjdk
Copy link

openjdk bot commented Jun 26, 2024

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Jun 26, 2024
@mlbridge
Copy link

mlbridge bot commented Jun 26, 2024

Webrevs

Copy link
Contributor

@martinuy martinuy left a comment

Choose a reason for hiding this comment

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

Thanks for proposing this backport. Looks good to me.

@openjdk
Copy link

openjdk bot commented Jun 26, 2024

⚠️ @franferrax This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

I had GitHub actions disabled (default when forking), triggering
execution by pushing an empty commit (so the review is not invalidated).
Attempt to backport JDK-8205407 in the 8u build system:
  Summary: [windows, vs<2017] C4800 after 8203197
      URL: https://bugs.openjdk.org/browse/JDK-8205407
@franferrax
Copy link
Contributor Author

/issue add 8205407

@openjdk
Copy link

openjdk bot commented Jun 26, 2024

@franferrax
Adding additional issue to issue list: 8205407: [windows, vs<2017] C4800 after 8203197.

@jerboaa
Copy link
Contributor

jerboaa commented Jun 27, 2024

@rwestrel Could you please help review this? Thanks!

Copy link
Contributor

@rwestrel rwestrel left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@franferrax
Copy link
Contributor Author

/approval 8262017 request jdk8u is affected by this bug and would benefit from its fix. The patch does not apply cleanly but has been reviewed.

@franferrax
Copy link
Contributor Author

/approval 8205407 request This issue is a dependency for the JDK-8262017 backport in jdk8u.

@openjdk
Copy link

openjdk bot commented Jun 27, 2024

@franferrax
8262017: The approval request has been created successfully.

@openjdk
Copy link

openjdk bot commented Jun 27, 2024

@franferrax
8205407: The approval request has been created successfully.

@jerboaa
Copy link
Contributor

jerboaa commented Jun 28, 2024

/approve yes

@openjdk
Copy link

openjdk bot commented Jun 28, 2024

@jerboaa
8262017: The approval request has been approved.
8205407: The approval request has been approved.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed approval labels Jun 28, 2024
@franferrax
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 28, 2024
@openjdk
Copy link

openjdk bot commented Jun 28, 2024

@franferrax
Your change (at version 590ef75) is now ready to be sponsored by a Committer.

@martinuy
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Jun 28, 2024

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

  • 44eac48: 8137329: [windows] Build broken on VS2010 after "8046148: JEP 158: Unified JVM Logging"
  • a0715ab: 8238274: (sctp) JDK-7118373 is not fixed for SctpChannel

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 28, 2024
@openjdk openjdk bot closed this Jun 28, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 28, 2024
@openjdk
Copy link

openjdk bot commented Jun 28, 2024

@martinuy @franferrax Pushed as commit 385e34d.

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

@franferrax franferrax deleted the backport-JDK-8262017 branch June 28, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants