JDK-8255544: Create a checked cast #904

theRealAph · 2020-10-28T15:50:52Z

In many places we've added C-style casts to silence compiler warnings, for example when truncating a size_t to an int when we know the size_t is a small struct. Such casts are inherently risky, because they effectively disable useful compiler warnings. We should add a form of cast that checks at runtime that a truncation does not overflow.

Progress

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

Issue

JDK-8255544: Create a checked cast

Reviewers

Andrew Dinn (@adinn - Reviewer)
Ioi Lam (@iklam - Reviewer)

Download

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

bridgekeeper · 2020-10-28T15:51:49Z

👋 Welcome back aph! 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 · 2020-10-28T15:53:36Z

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

hotspot

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.

mlbridge · 2020-10-28T15:56:56Z

Webrevs

01: Full - Incremental (a3c9516)
00: Full (5f8c47d)

theRealAph · 2020-10-28T16:04:11Z

One thing I should have said: the need for this was triggered by a recent patch to silence many warnings emitted the MSVC AArch64 compiler. It would have been possible to put the checks into the AArch64 back end, but I think we need a centralized way to do it.

adinn

This is great. All we need now is to get hotspot devs to use it :-)

openjdk · 2020-10-30T10:08:07Z

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

8255544: Create a checked cast

Reviewed-by: adinn, 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 59 new commits pushed to the master branch:

64feeab: 8255603: Memory/Performance regression after JDK-8210985
518ff51: 8233569: [TESTBUG] JTextComponent test bug6361367.java fails on macos
4c4b8f4: 8196302: javax/swing/JFileChooser/8041694/bug8041694.java
f61ce32: 8255576: (fs) Files.isHidden() throws ArrayIndexOutOfBoundsException (unix)
fe7672b: 8196099: javax/swing/text/CSSBorder/6796710/bug6796710.java fails
cacce84: 8169954: JFileChooser/8021253: java.lang.RuntimeException: Default button is not pressed
7597cba: 8143021: [TEST_BUG] Test javax/swing/JColorChooser/Test6541987.java fails
80380d5: 8255494: PKCS7 should use digest algorithm to verify the signature
9d5c9cc: 8254309: appcds GCDuringDump.java failed - class must exist
36c150b: 8255489: Unify the parsing of @lambda-proxy and @lambda-form-invokers tags in a classlist
... and 49 more: https://git.openjdk.java.net/jdk/compare/3bd5b807619b6e9f433301f4b39b5f21f6daf517...master

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.

iklam

Can we change one place in HotSpot to use this function (preferably in a frequently-used code path), to verity that it really works?

mlbridge · 2020-10-31T10:43:55Z

Mailing list message from Andrew Haley on hotspot-dev:

On 31/10/2020 03:17, Ioi Lam wrote:

On Wed, 28 Oct 2020 15:50:52 GMT, Andrew Haley <aph at openjdk.org> wrote:

In many places we've added C-style casts to silence compiler warnings, for example when truncating a size_t to an int when we know the size_t is a small struct. Such casts are inherently risky, because they effectively disable useful compiler warnings. We should add a form of cast that checks at runtime that a truncation does not overflow.

Can we change one place in HotSpot to use this function (preferably in a frequently-used code path), to verity that it really works?

Oh, sure. I have plenty in the AArch64 back end, but I'll look for some
others.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

theRealAph · 2020-10-31T13:59:34Z

Can we change one place in HotSpot to use this function (preferably in a frequently-used code path), to verity that it really works?

How about this?

iklam

LGTM

adinn

Still looking good

theRealAph · 2020-11-02T13:10:37Z

/integrate

theRealAph · 2020-11-02T13:13:04Z

/integrate

openjdk · 2020-11-02T13:15:53Z

@theRealAph Since your change was applied there have been 67 commits pushed to the master branch:

54c8813: 8255734: VM should ignore SIGXFSZ on ppc64, s390 too
ceab9f3: 6816284: Notepad class should be public
eb66418: 7124397: [macosx] JSpinner serialiazation - deserialization issue
79a010f: 8255697: LogTargetHandle::print should check if log level is enabled
98c91b6: 8233637: [TESTBUG] Swing ActionListenerCalledTwiceTest.java fails on macos
e97809d: 8233641: [TESTBUG] JMenuItem test bug4171437.java fails on macos
69f5235: 8255596: Mutex safepoint checking options and flags should be scoped enums
d05df7c: 8236842: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release
64feeab: 8255603: Memory/Performance regression after JDK-8210985
518ff51: 8233569: [TESTBUG] JTextComponent test bug6361367.java fails on macos
... and 57 more: https://git.openjdk.java.net/jdk/compare/3bd5b807619b6e9f433301f4b39b5f21f6daf517...master

Your commit was automatically rebased without conflicts.

Pushed as commit 3302d3a.

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

Reviewed-by: adinn, iklam

theRealAph · 2020-11-02T13:17:03Z

/integrate

coleenp

Looks good to me.

mrserb · 2020-12-03T23:20:21Z

I guess this PR should be closed?

bridgekeeper · 2021-01-01T00:32:06Z

@theRealAph This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2021-01-29T02:34:17Z

@theRealAph This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

openjdk · 2021-01-29T02:34:40Z

@theRealAph The command integrate can only be used in open pull requests.

openjdk · 2021-01-29T02:35:01Z

@theRealAph The command integrate can only be used in open pull requests.

JDK-8255544: Create a checked cast

5f8c47d

openjdk bot added the rfr Pull request is ready for review label Oct 28, 2020

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 28, 2020

adinn approved these changes Oct 30, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 30, 2020

iklam suggested changes Oct 31, 2020

View reviewed changes

JDK-8255544: Create a checked cast

a3c9516

iklam approved these changes Oct 31, 2020

View reviewed changes

adinn approved these changes Nov 2, 2020

View reviewed changes

theRealAph closed this Nov 2, 2020

openjdk bot added the integrated Pull request has been integrated label Nov 2, 2020

theRealAph reopened this Nov 2, 2020

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 2, 2020

openjdk-notifier bot referenced this pull request Nov 2, 2020

8255544: Create a checked cast

3302d3a

Reviewed-by: adinn, iklam

coleenp approved these changes Nov 18, 2020

View reviewed changes

bridgekeeper bot closed this Jan 29, 2021

JDK-8255544: Create a checked cast #904

JDK-8255544: Create a checked cast #904

Uh oh!

Conversation

theRealAph commented Oct 28, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Oct 28, 2020

Uh oh!

openjdk bot commented Oct 28, 2020

Uh oh!

mlbridge bot commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

theRealAph commented Oct 28, 2020

Uh oh!

adinn left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iklam left a comment

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Oct 31, 2020

Uh oh!

theRealAph commented Oct 31, 2020

Uh oh!

iklam left a comment

Choose a reason for hiding this comment

Uh oh!

adinn left a comment

Choose a reason for hiding this comment

Uh oh!

theRealAph commented Nov 2, 2020

Uh oh!

theRealAph commented Nov 2, 2020

Uh oh!

openjdk bot commented Nov 2, 2020

Uh oh!

theRealAph commented Nov 2, 2020

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

mrserb commented Dec 3, 2020

Uh oh!

bridgekeeper bot commented Jan 1, 2021

Uh oh!

bridgekeeper bot commented Jan 29, 2021

Uh oh!

openjdk bot commented Jan 29, 2021

Uh oh!

openjdk bot commented Jan 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

theRealAph commented Oct 28, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 28, 2020 •

edited

Loading

openjdk bot commented Oct 30, 2020 •

edited

Loading