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

8264918: [JVMCI] getVtableIndexForInterfaceMethod doesn't check that type and method are related #3396

Conversation

@iwanowww
Copy link

@iwanowww iwanowww commented Apr 8, 2021

getVtableIndexForInterfaceMethod should reject the case when a resolved class doesn't implement the holder interface.

Testing:

  • hs-tier1 - hs-tier4

Progress

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

Issue

  • JDK-8264918: [JVMCI] getVtableIndexForInterfaceMethod doesn't check that type and method are related

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3396/head:pull/3396
$ git checkout pull/3396

Update a local copy of the PR:
$ git checkout pull/3396
$ git pull https://git.openjdk.java.net/jdk pull/3396/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3396

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3396.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 8, 2021

👋 Welcome back vlivanov! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

@iwanowww
The hotspot-compiler label was successfully added.

Loading

@iwanowww iwanowww marked this pull request as ready for review Apr 8, 2021
@openjdk openjdk bot added the rfr label Apr 8, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 8, 2021

Webrevs

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good.
It needs review from Graal team.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

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

8264918: [JVMCI] getVtableIndexForInterfaceMethod doesn't check that type and method are related

Reviewed-by: kvn

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

  • 81d35e4: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.
  • 04fa1ed: 8264848: [macos] libjvm.dylib linker warning due to macOS version mismatch
  • 214d6e2: 8263506: Make sun.net.httpserver.UnmodifiableHeaders unmodifiable
  • af13c64: 8264711: More runtime TRAPS cleanups
  • 3aec2d9: 8264718: Shenandoah: enable string deduplication during root scanning
  • 255afbe: 8264672: runtime/ParallelLoad/ParallelSuperTest.java timed out
  • ec599da: 8264633: Add missing logging to PlatformRecording#stop
  • e89542f: 8264352: AArch64: Optimize vector "not/andNot" for NEON and SVE
  • 016db40: 8263907: Specification of CellRendererPane::paintComponent(..Rectangle) should clearly mention which method it delegates the call to
  • 78d1164: 8214455: Relocate CDS archived regions to the top of the G1 heap
  • ... and 2 more: https://git.openjdk.java.net/jdk/compare/6e2b82a45f2f86c45fe62dcc6cf5cb979b8a5366...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.

Loading

@openjdk openjdk bot added the ready label Apr 8, 2021
@dougxc
Copy link
Member

@dougxc dougxc commented Apr 8, 2021

The changes look good to me.
How did you discover this problem? If the detail is in https://bugs.openjdk.java.net/browse/JDK-8264918, I can wait until the JBS server comes back up.

Loading

@iwanowww
Copy link
Author

@iwanowww iwanowww commented Apr 8, 2021

Thanks for the reviews, Vladimir and Doug.

It was discovered by a new assert introduced in #3346.

Loading

@iwanowww
Copy link
Author

@iwanowww iwanowww commented Apr 9, 2021

/integrate

Loading

@openjdk openjdk bot closed this Apr 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 9, 2021

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

  • f7a6c63: 8259822: [PPC64] Support the prefixed instruction format added in POWER10
  • a45733f: 8264779: Fix doclint warnings in java/nio
  • 3e57924: 8264885: Fix the code style of macro in aarch64_neon_ad.m4
  • 051c117: 8264923: PNGImageWriter.write_zTXt throws Exception with a typo
  • 1c6b113: 8264513: Cleanup CardTableBarrierSetC2::post_barrier
  • 666fd62: 8264881: Remove the old development option MemProfiling
  • 951f277: 8264874: Build interim-langtools for HotSpot only if Graal is enabled
  • 719f95e: 8260693: Provide the support for specifying a signer in keytool -genkeypair
  • 77b1673: 8256245: AArch64: Implement Base64 decoding intrinsic
  • 57f1e7d: 8264696: Multi-catch clause causes compiler exception because it uses the package-private supertype
  • ... and 17 more: https://git.openjdk.java.net/jdk/compare/6e2b82a45f2f86c45fe62dcc6cf5cb979b8a5366...master

Your commit was automatically rebased without conflicts.

Pushed as commit b3782ea.

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

Loading

@dougxc
Copy link
Member

@dougxc dougxc commented Apr 22, 2021

Looking into https://bugs.openjdk.java.net/browse/JDK-8265689, I see that it's due to this PR missing these changes:

diff --git a/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java b/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java
index faf3cec709..945ec7ba1a 100644
--- a/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java
+++ b/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java
@@ -590,8 +590,9 @@ final class CompilerToVM {
      * {@link HotSpotVMConfig#invalidVtableIndex} if {@code method} is not in {@code type}'s
      * v-table.
      *
-     * @throws InternalError if {@code type} is an interface or {@code method} is not held by an
-     *             interface or class represented by {@code type} is not initialized
+     * @throws InternalError if {@code type} is an interface, {@code method} is not defined by an
+     *             interface, {@code type} does not implement the interface defining {@code method}
+     *             or class represented by {@code type} is not initialized
      */
     native int getVtableIndexForInterfaceMethod(HotSpotResolvedObjectTypeImpl type, HotSpotResolvedJavaMethodImpl method);
 
diff --git a/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java b/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java
index d494679aff..41dab25d0e 100644
--- a/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java
+++ b/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java
@@ -677,7 +677,7 @@ final class HotSpotResolvedJavaMethodImpl extends HotSpotMethod implements HotSp
             return config().invalidVtableIndex;
         }
         if (holder.isInterface()) {
-            if (resolved.isInterface() || !resolved.isLinked()) {
+            if (resolved.isInterface() || !resolved.isLinked() || !getDeclaringClass().isAssignableFrom(resolved)) {
                 return config().invalidVtableIndex;
             }
             return getVtableIndexForInterfaceMethod(resolved);

@iwanowww would you mind taking JDK-8265689 and using it to apply these missing changes?

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants