8308396: Fix offset_of conversion warnings in runtime code #14053

coleenp · 2023-05-18T22:37:57Z

Please review this change to use ByteSize and byte_offset_of() to refer to offsets to metadata and other types that are used in generated code. This avoids the int narrowing conversion warning for -Wconversion. There were a couple that I just added an (int) cast instead because these offsets are either being used in other code currently being changed (in oopDesc) or there are too many (like displaced_header_offset_in_bytes) and should be their own change.

This change takes a chunk out of the -Wconversion warnings - see CR for more info.

It might be easier and less tedious to review the commits separately. One commit renames blah_offset_in_bytes to blah_offset, since in_bytes(blah_offset()) is typically used (except in Address constructor which has an overload for ByteSize).

Tested with tier1-4, x86 and aarch64, and built linux-x64-zero linux-x64-zero-debug linux-aarch64-debug linux-s390x-open linux-arm32-debug linux-ppc64le-debug linux-riscv64-debug locally.

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-8308396: Fix offset_of conversion warnings in runtime code

Reviewers

Amit Kumar (@offamitkumar - Author) ⚠️ Review applies to 56f2b4ba
Johan Sjölen (@jdksjolen - Reviewer) ⚠️ Review applies to f629ebce
Frederic Parain (@fparain - Reviewer) ⚠️ Review applies to 1f78a6fe

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14053

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

Using diff file

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

Webrev

Link to Webrev Comment

… or add int cast in the cases where too much code downstream does arithmetic with the offset.

…e which require in_bytes() to get to int.

bridgekeeper · 2023-05-18T22:39:19Z

👋 Welcome back coleenp! 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 · 2023-05-18T22:41:20Z

@coleenp The following labels will be automatically applied to this pull request:

graal
hotspot

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

mlbridge · 2023-05-18T22:45:03Z

Webrevs

src/hotspot/cpu/s390/sharedRuntime_s390.cpp

offamitkumar

Testing for fastdebug build/ tier1 tests looks good on s390x.

I've given my suggestion for fixing the build-break, but I leave that upto you :-)

src/hotspot/cpu/s390/sharedRuntime_s390.cpp

jdksjolen

Hi,

This looks good to me, with one small nit.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

openjdk · 2023-05-19T12:39:12Z

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

8308396: Fix offset_of conversion warnings in runtime code

Reviewed-by: amitkumar, jsjolen, fparain

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

a5343fa: 8281149: (fs) java/nio/file/FileStore/Basic.java fails with java.lang.RuntimeException: values differ by more than 1GB
44218b1: 8308248: Revisit alignment of layout constants on 32-bit platforms
80ef5c2: 8308410: broken compilation of test\jdk\tools\launcher\exeJliLaunchTest.c
326d778: 8306057: False arguments calling dispatch_base for aarch64
690d396: 8308408: Build failure with -Werror=maybe-uninitialized in libjli/java.c with GCC8
fa14314: 8303669: SelectVersion indexes past the end of the argv array
25868b9: 8132995: Matcher$ImmutableMatchResult should be optimized to reduce space usage
6765761: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey
97ade57: 8307609: RISC-V: Added support for Extract, Compress, Expand and other nodes for Vector API
e520cdc: 8308277: RISC-V: Improve vectorization of Match.sqrt() on floats
... and 52 more: https://git.openjdk.org/jdk/compare/72294c5402dd11e3eb4922ad26e1b153098495ff...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.

fparain

Looks good to me, only a few comments on the style.

src/hotspot/cpu/ppc/macroAssembler_ppc.cpp

src/hotspot/cpu/x86/macroAssembler_x86.cpp

coleenp · 2023-05-19T17:15:21Z

Thank you for reviewing Amit, Johan and Fred.
/integrate

openjdk · 2023-05-19T17:16:09Z

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

a5343fa: 8281149: (fs) java/nio/file/FileStore/Basic.java fails with java.lang.RuntimeException: values differ by more than 1GB
44218b1: 8308248: Revisit alignment of layout constants on 32-bit platforms
80ef5c2: 8308410: broken compilation of test\jdk\tools\launcher\exeJliLaunchTest.c
326d778: 8306057: False arguments calling dispatch_base for aarch64
690d396: 8308408: Build failure with -Werror=maybe-uninitialized in libjli/java.c with GCC8
fa14314: 8303669: SelectVersion indexes past the end of the argv array
25868b9: 8132995: Matcher$ImmutableMatchResult should be optimized to reduce space usage
6765761: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey
97ade57: 8307609: RISC-V: Added support for Extract, Compress, Expand and other nodes for Vector API
e520cdc: 8308277: RISC-V: Improve vectorization of Match.sqrt() on floats
... and 52 more: https://git.openjdk.org/jdk/compare/72294c5402dd11e3eb4922ad26e1b153098495ff...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-19T17:16:22Z

@coleenp Pushed as commit 265f40b.

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

coleenp added 4 commits May 17, 2023 18:39

Change offset_of to byte_offset_of returning ByteSize for most cases,…

d23ae09

… or add int cast in the cases where too much code downstream does arithmetic with the offset.

Rename blah_in_bytes to blah because now the functions return ByteSiz…

612720c

…e which require in_bytes() to get to int.

Restore warnings are errors.

8133bdb

Change ObjectMonitor offset to return ByteSize

56f2b4b

openjdk bot added the rfr Pull request is ready for review label May 18, 2023

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels May 18, 2023

offamitkumar reviewed May 19, 2023

View reviewed changes

src/hotspot/cpu/s390/sharedRuntime_s390.cpp Outdated Show resolved Hide resolved

offamitkumar approved these changes May 19, 2023

View reviewed changes

src/hotspot/cpu/s390/sharedRuntime_s390.cpp Outdated Show resolved Hide resolved

Fix s390

f629ebc

jdksjolen approved these changes May 19, 2023

View reviewed changes

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label May 19, 2023

coleenp added 2 commits May 19, 2023 12:41

Fix indentation

90f9544

Fix indentation

1f78a6f

fparain approved these changes May 19, 2023

View reviewed changes

src/hotspot/cpu/ppc/macroAssembler_ppc.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/ppc/macroAssembler_ppc.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/x86/macroAssembler_x86.cpp Outdated Show resolved Hide resolved

Fix more indentation and fparain comments.

7736bb9

openjdk bot added the integrated Pull request has been integrated label May 19, 2023

openjdk bot closed this May 19, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 19, 2023

coleenp deleted the Wconversion branch May 19, 2023 17:17

8308396: Fix offset_of conversion warnings in runtime code #14053

8308396: Fix offset_of conversion warnings in runtime code #14053

Uh oh!

Conversation

coleenp commented May 18, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 18, 2023

Uh oh!

openjdk bot commented May 18, 2023

Uh oh!

mlbridge bot commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

offamitkumar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jdksjolen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openjdk bot commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fparain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coleenp commented May 19, 2023

Uh oh!

openjdk bot commented May 19, 2023

Uh oh!

openjdk bot commented May 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

coleenp commented May 18, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 18, 2023 •

edited

Loading

openjdk bot commented May 19, 2023 •

edited

Loading