8335552: [8u] JDK-8303466 backport to 8u requires 3 ::Identity signature fixes

[franferrax]

Hi, here is a followup fix for #529, where @wkia has noted a problem with the MaxLNode::Identity, MinLNode::Identity and ConvI2LNode::Identity signatures: #529 (comment).

A complimentary JDK-8075511 backport can be found in #534.

---

Progress

• JDK-8335552 needs maintainer approval
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (3 reviews required, with at least 3 Reviewers)

Issue

• JDK-8335552: [8u] JDK-8303466 backport to 8u requires 3 ::Identity signature fixes (Bug - P4 - Approved)

Reviewers

• Martin Balao (@martinuy - Reviewer) ⚠️ Review applies to 85db200c
• Roland Westrelin (@rwestrel - Reviewer) ⚠️ Review applies to 85db200c
• Andrew John Hughes (@gnu-andrew - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 532

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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]

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

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

8335552: [8u] JDK-8303466 backport to 8u requires 3 ::Identity signature fixes

Reviewed-by: mbalao, roland, andrew

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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, @gnu-andrew) 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).

[mlbridge]

Webrevs

• 01: Full - Incremental (9f5b757c)
• 00: Full (85db200c)

[martinuy]

Looks good to me.

[openjdk]

⚠️ @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.

[rwestrel]

Looks good to me.

[jerboaa]

@franferrax Please apply for approval of this bug, thanks!

[franferrax]

/approval request jdk8u is affected by this bug after the JDK-8335472 integration. ::Identity methods are not being invoked, leading to sub-optimal code for MaxLNode, MinLNode and ConvI2LNode. The patch has been reviewed and GitHub Actions passed (except for macOS, where the jobs are still waiting for an available runner to pick them).

[openjdk]

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

[franferrax]

I just noticed that, given cbd483a, this is a clean backport of JDK 9's JDK-8075511 (openjdk/jdk9u@8efb3ab). For the sake of clarity, I will add the issue and apply for approval there too. Sorry @jerboaa for bothering you with one more approval.

[franferrax]

/issue add 8075511

[openjdk]

@franferrax
Adding additional issue to issue list: 8075511: Enable -Woverloaded-virtual C++ warning for HotSpot build.

[franferrax]

/approval 8075511 request JDK-8335552 could have been avoided with this warning (treated as error in GitHub Actions). A 8u backport is already part of the approved PR.

[openjdk]

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

[jerboaa]

I just noticed that, given cbd483a, this is a clean backport of JDK 9's JDK-8075511 (openjdk/jdk9u@8efb3ab). For the sake of clarity, I will add the issue and apply for approval there too. Sorry @jerboaa for bothering you with one more approval.

@franferrax Hmm, thinking some more about this and seeing the warnings on the added issue. Please create a separate backport for it and then merge that into this one.

[openjdk]

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

[franferrax]

@gnu-andrew: ok, sounds reasonable, no problem!

[gnu-andrew]

@gnu-andrew: ok, sounds reasonable, no problem!

Thanks for your understanding. You should just be able to backout the commit from this PR and drop the bug ID.

[franferrax]

/issue remove 8075511

[openjdk]

@franferrax
Removing additional issue from issue list: 8075511.

[franferrax]

/label remove backport

[openjdk]

@franferrax
The label backport is not a valid label.
These labels are valid:

[franferrax]

/backport disable jdk8u-dev

[openjdk]

@franferrax Backport for repo openjdk/jdk8u-dev on branch master was already disabled.

[franferrax]

@gnu-andrew: the only thing I could not do is to remove the backport label from this PR. I suspect that /label remove backport would have worked if issued by someone with enough privileges.

[gnu-andrew]

Patch looks fine now. Let's see what the commit message says to see if the backport label is an issue.

[gnu-andrew]

Patch looks fine now. Let's see what the commit message says to see if the backport label is an issue.

Doesn't look to be.

[franferrax]

/integrate

[openjdk]

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

[martinuy]

/sponsor

[openjdk]

Going to push as commit 16ea85b.

[openjdk]

@martinuy @franferrax Pushed as commit 16ea85b.

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

[franferrax]

@gnu-andrew

Patch looks fine now. Let's see what the commit message says to see if the backport label is an issue.

Doesn't look to be.

Hmmm, but the final commit message still included Backport-of: 8efb3ab7c1cf0164237c50d60c4cd564b3d6a897.

[gnu-andrew]

@gnu-andrew

Patch looks fine now. Let's see what the commit message says to see if the backport label is an issue.

Doesn't look to be.

Hmmm, but the final commit message still included Backport-of: 8efb3ab7c1cf0164237c50d60c4cd564b3d6a897.

Ugh. I don't see a way we could have removed it and this wasn't in the preview commit summary. Might be worth a SKARA bug.

[franferrax]

The original backport hash is obtained from a hidden comment in this message with a specific format (we can see the hidden part in the raw message body).

The /integrate command then passes this backport hash as original when creating the commit and, unlike the /clean command, the backport label isn't checked. The commit message's original finally ends up in the Backport-of: footer.

So even if we managed to remove the backport label, the Backport-of: footer would still have been generated, as long as this message is readable by the bot.

Apparently, a PR that starts or becomes a backport, is totally unexpected to return to a non-backport state. Knowing about this irreversible process is enough for me, to be more careful next time. In the unlikely case we need to return to a non-backport PR in the future, we can just start a new PR.