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

8311077: Fix -Wconversion warnings in jvmti code #14710

Closed
wants to merge 4 commits into from

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Jun 29, 2023

Please review change for mostly fixing return types in the constant pool and metadata to fix -Wconversion warnings in JVMTI code. The order of preference for changes are: 1. change the types to more distinct types (fields in the constant pool are u2 because that's their size in the classfile), 2. add direct int casts if the value has been checked in asserts above, and 3. checked_cast<> if not verified, and 4. added some pointer_delta_as_ints where needed.
Tested with tier1-4.


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-8311077: Fix -Wconversion warnings in jvmti code (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14710

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 29, 2023

👋 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 openjdk bot added the rfr Pull request is ready for review label Jun 29, 2023
@openjdk
Copy link

openjdk bot commented Jun 29, 2023

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

  • hotspot
  • serviceability

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.

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jun 29, 2023
@mlbridge
Copy link

mlbridge bot commented Jun 29, 2023

Webrevs

@coleenp coleenp changed the title 8311077: Fix -Wconversion warnings in jvmti code. 8311077: Fix -Wconversion warnings in jvmti code Jun 29, 2023
@openjdk
Copy link

openjdk bot commented Jun 29, 2023

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

8311077: Fix -Wconversion warnings in jvmti code

Reviewed-by: fparain, matsaave, dholmes

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

  • faf1b82: 8310656: RISC-V: __builtin___clear_cache can fail silently.
  • 0e3d91d: 8311215: [BACKOUT] JDK-8047998 Abort the vm if MaxNewSize is not the same as NewSize when MaxHeapSize is the same as InitialHeapSize
  • 09a4924: 8311145: Remove check_with_errno duplicates
  • 8abb9f5: 8047998: Abort the vm if MaxNewSize is not the same as NewSize when MaxHeapSize is the same as InitialHeapSize
  • d2e1159: 8311125: Remove unused parameter 'phase' in AllocateNode::Ideal_allocation
  • 8c8e9d9: 8309819: Clarify API note in Class::getName and MethodType::toMethodDescriptorString
  • 140b70f: 8311186: ProblemList javax/management/remote/mandatory/subjectDelegation/SubjectDelegation1Test.java on linux-aarch64
  • 971c2ef: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
  • f6bdccb: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
  • 456bf11: 8310173: Update --release 21 symbol information for JDK 21 build 29
  • ... and 29 more: https://git.openjdk.org/jdk/compare/ded137073ac1b2086a577508172626898999e275...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 29, 2023
Copy link
Contributor

@matias9927 matias9927 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, thanks for this change! I listed a few considerations below, but if you don't think they are necessary, what you have is just fine.

Copy link
Contributor Author

@coleenp coleenp 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 your comments Matias. Some of the changes that I didn't make were because Wconversion didn't complain and where I could avoid casting. I think more precise types for constant pool indices and cpCache indices might be more pervasive than we want and might cause more warnings down the line. So I avoided them.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Sorry Coleen but this raises a lot of questions for me. I expect there are a few ways to silence these conversion warnings but many of the proposed changes don't look semantically right to me. I realize that we have a lot of inconsistencies in the way we declare and use types and that the proposed changes may be the most minimal way to fix things, but it is better to type them correctly from the start IMO and only cast at the "boundaries" if it requires a change to propagate too far.

src/hotspot/share/oops/constMethod.hpp Show resolved Hide resolved
src/hotspot/share/oops/method.hpp Show resolved Hide resolved
src/hotspot/share/oops/method.hpp Show resolved Hide resolved
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvmtiRawMonitor.cpp Show resolved Hide resolved
src/hotspot/share/prims/jvmtiTrace.cpp Show resolved Hide resolved
src/hotspot/share/prims/jvmtiTrace.cpp Show resolved Hide resolved
src/hotspot/share/prims/methodComparator.cpp Show resolved Hide resolved
src/hotspot/share/runtime/jfieldIDWorkaround.hpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@coleenp coleenp left a comment

Choose a reason for hiding this comment

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

The task is NOT to just silence the conversion warnings but to correct the types to be more precise to not get conversion warnings. That's what this change does. I think I've answered your questions about the types chosen.

src/hotspot/share/oops/constMethod.hpp Show resolved Hide resolved
src/hotspot/share/oops/method.hpp Show resolved Hide resolved
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvmtiTrace.cpp Show resolved Hide resolved
src/hotspot/share/prims/jvmtiTrace.cpp Show resolved Hide resolved
src/hotspot/share/runtime/jfieldIDWorkaround.hpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvmtiRawMonitor.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvmtiRawMonitor.cpp Show resolved Hide resolved
src/hotspot/share/oops/method.hpp Show resolved Hide resolved
@coleenp
Copy link
Contributor Author

coleenp commented Jun 30, 2023

@dholmes-ora The reason that these changes are broken up like this is because there are a huge amount of -Wconversion warnings and to do it right, we need to examine where the warnings are to see if there are bugs, and to correct the code to either use the right types or allow the cast with an assertion check if necessary. The changes are not designed to be completely minimal but balanced so that people can review them and that changes that aren't necessary aren't made.
We pass around 'int' as a parameter type for values that are u2, for example. It really matters most when we're storing that value or using it in a context that requires the narrow type. If we're using it as an index which most constant pool and constant pool cache indices are used as, we can keep it as an int. A u2 parameter is promoted to int so it's the same thing.
I hope I've answered your questions about these specific types in this change.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

As a general rule I'd prefer to see the lowest-level functions use the types of the actual thing they are exposing - so for anything CP related we return u1/u2/u4 - and then have higher-level code do any convenience conversions. But I realize this is tricky to deal with and there are alternatives and trade-offs in where these warnings get silenced.

Thanks.

@mlbridge
Copy link

mlbridge bot commented Jul 3, 2023

Mailing list message from David Holmes on hotspot-dev:

On 3/07/2023 10:55 am, David Holmes wrote:

On Fri, 30 Jun 2023 13:10:06 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

Please review change for mostly fixing return types in the constant pool and metadata to fix -Wconversion warnings in JVMTI code. The order of preference for changes are: 1. change the types to more distinct types (fields in the constant pool are u2 because that's their size in the classfile), 2. add direct int casts if the value has been checked in asserts above, and 3. checked_cast<> if not verified, and 4. added some pointer_delta_as_ints where needed.
Tested with tier1-4.

Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:

David's suggestions.

src/hotspot/share/runtime/jfieldIDWorkaround.hpp line 92:

90: result &= small_offset_mask; // cut off the hash bits
91: }
92: return result;

Doesn't this trigger a warning as we go from unsigned to signed?

Weird: I was sure I deleted this comment. Please ignore. I can't even
find it in the PR so responding via email.

David

@coleenp
Copy link
Contributor Author

coleenp commented Jul 5, 2023

Thanks David for the review. I do change the types as low in the call stack as practical with these changes. I think your comment above disappeared because the last change reverted that code.
/integrate

@openjdk
Copy link

openjdk bot commented Jul 5, 2023

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

  • 00ac46c: 8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient
  • d6578bf: 8309531: Incorrect result with unwrapped iotaShuffle.
  • 7b3c2dc: 8311122: Fix typos in java.base
  • 607ddaa: 8310997: missing @SInCE tags in jdk.httpserver
  • 7655b48: 8311001: missing @SInCE info in jdk.net
  • 711cddd: 8311249: Remove unused MemAllocator::obj_memory_range
  • 514816e: 8309889: [s390] Missing return statement after calling jump_to_native_invoker method in generate_method_handle_dispatch.
  • 60544f9: 8309894: compiler/vectorapi/VectorLogicalOpIdentityTest.java fails on SVE system with UseSVE=0
  • 0916e6a: 8311092: Please disable runtime/jni/nativeStack/TestNativeStack.java on armhf
  • d8a0121: 8311109: tautological-compare warning in awt_Win32GraphicsDevice.cpp
  • ... and 49 more: https://git.openjdk.org/jdk/compare/ded137073ac1b2086a577508172626898999e275...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Jul 5, 2023

@coleenp Pushed as commit cf82e31.

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

@coleenp coleenp deleted the jvmti-conversion branch July 5, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
4 participants