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

8221503: vmTestbase/nsk/jdb/eval/eval001/eval001.java fails with: com.sun.jdi.InvalidTypeException: Can't assign double[][][] to double[][][] #3658

Closed
wants to merge 3 commits into from

Conversation

@fmatte1
Copy link

@fmatte1 fmatte1 commented Apr 23, 2021

findComponentType() logic is wrong. In findComponentType() method, We always get vm.classesByName() retruns empty list
list = vm.classesByName(parser.typeName());
We have "parser.typeName()" retruns " double[][]"
vm.classesByName("") is expecting the fully qualified name example "java.lang.Double"
This always returns empty list, resulting into ClassNotLoadedException as it assumes the Component class has not yet been loaded, hence the test case fails.

There was a suggested fix from Egor Ushakov from JetBrains, I am proposing the same to get this fix. I have verified the patch with required testing it works fine.


Progress

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

Issue

  • JDK-8221503: vmTestbase/nsk/jdb/eval/eval001/eval001.java fails with: com.sun.jdi.InvalidTypeException: Can't assign double[][][] to double[][][]

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3658

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

Using diff file

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

….sun.jdi.InvalidTypeException: Can't assign double[][][] to double[][][]
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 23, 2021

👋 Welcome back fmatte! 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 label Apr 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 23, 2021

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

  • serviceability

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
Copy link

@mlbridge mlbridge bot commented Apr 23, 2021

Webrevs

// It's a primitive type
return vm.primitiveTypeMirror(sig.jdwpTag());
}
return findType(signature);
Copy link
Contributor

@plummercj plummercj Apr 27, 2021

Choose a reason for hiding this comment

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

Do we even need findComponentType() any more? Isn't ReferenceTypeImpl.findType() sufficient.

The comment above findComponentType() is kind of explicit as to why it was needed. Are you sure none of that still applies, and there isn't some edge case that findType() is not covering?

Copy link
Author

@fmatte1 fmatte1 Apr 28, 2021

Choose a reason for hiding this comment

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

Hi Chris,

Thanks for looking into this,

Do we even need findComponentType() any more? Isn't ReferenceTypeImpl.findType() sufficient.

We still need findComponentType(),
Difference between findType() and findComponentType() is that, findComponentType() tries to get the list of ReferenceType from the "vm.classesByName". In case list is empty, it explicitly throws ClassNotLoadedException.
This exception check is required in validateAssignment(ValueContainer destination) call from ObjectReferenceImpl.java.

https://github.com/openjdk/jdk/blob/master/src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java#L579

I have modified the method a bit to handle that scenario

diff --git a/src/jdk.jdi/share/classes/com/sun/tools/jdi/ArrayTypeImpl.java b/src/jdk.jdi/share/classes/com/sun/tools/jdi/ArrayTypeImpl.java
index e544c81ae3e..54deba43894 100644
--- a/src/jdk.jdi/share/classes/com/sun/tools/jdi/ArrayTypeImpl.java
+++ b/src/jdk.jdi/share/classes/com/sun/tools/jdi/ArrayTypeImpl.java
@@ -95,20 +95,13 @@ public class ArrayTypeImpl extends ReferenceTypeImpl
         JNITypeParser sig = new JNITypeParser(signature);
         if (sig.isReference()) {
             // It's a reference type
-            JNITypeParser parser = new JNITypeParser(componentSignature());
-            List<ReferenceType> list = vm.classesByName(parser.typeName());
-            Iterator<ReferenceType> iter = list.iterator();
-            while (iter.hasNext()) {
-                ReferenceType type = iter.next();
-                ClassLoaderReference cl = type.classLoader();
-                if ((cl == null)?
-                         (classLoader() == null) :
-                         (cl.equals(classLoader()))) {
-                    return type;
-                }
+           try {
+             Type componentType = findType(componentSignature());
+             return componentType;
+              // Component class has not yet been loaded
+           } catch (ClassNotLoadedException ex) {
+               throw new ClassNotLoadedException(componentTypeName());
             }
-            // Component class has not yet been loaded
-            throw new ClassNotLoadedException(componentTypeName());
         } else {
             // It's a primitive type
             return vm.primitiveTypeMirror(sig.jdwpTag());```

Thanks,

Copy link
Contributor

@plummercj plummercj Apr 29, 2021

Choose a reason for hiding this comment

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

Do we even need findComponentType() any more? Isn't ReferenceTypeImpl.findType() sufficient.

We still need findComponentType(),
Difference between findType() and findComponentType() is that, findComponentType() tries to get the list of ReferenceType from the "vm.classesByName". In case list is empty, it explicitly throws ClassNotLoadedException.
This exception check is required in validateAssignment(ValueContainer destination) call from ObjectReferenceImpl.java.

I'm not sure what you mean by this. After your changes, this is all findComponentType() does:

    Type findComponentType(String signature) throws ClassNotLoadedException {
        return findType(signature);
    }

And findType() has the exact same signature, including the throws:

Type findType(String signature) throws ClassNotLoadedException {

So my suggestion is to get rid of findComponentType() and just have current users call findType() instead.

Copy link
Author

@fmatte1 fmatte1 Apr 29, 2021

Choose a reason for hiding this comment

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

Thanks Chris, for the review comments.
I have modified the suggested changes, kindly review.

Thanks,

Copy link
Contributor

@plummercj plummercj left a comment

Looks good. Please make sure you have run all of the following tests:

test/hotspot/jtreg/vmTestbase/nsk/jdb
test/hotspot/jtreg/serviceability/jdwp
test/hotspot/jtreg/vmTestbase/nsk/jdwp
test/hotspot/jtreg/vmTestbase/nsk/jdi
test/jdk/com/sun/jdi

@openjdk
Copy link

@openjdk openjdk bot commented Apr 30, 2021

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

8221503: vmTestbase/nsk/jdb/eval/eval001/eval001.java fails with: com.sun.jdi.InvalidTypeException: Can't assign double[][][] to double[][][]

Reviewed-by: cjplummer, sspitsyn

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

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.

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 (@plummercj, @sspitsyn) 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).

@openjdk openjdk bot added the ready label Apr 30, 2021
@fmatte1
Copy link
Author

@fmatte1 fmatte1 commented May 3, 2021

Hi Chris, thanks for the rreview.

test/hotspot/jtreg/vmTestbase/nsk/jdb
test/hotspot/jtreg/serviceability/jdwp
test/hotspot/jtreg/vmTestbase/nsk/jdwp
test/hotspot/jtreg/vmTestbase/nsk/jdi
test/jdk/com/sun/jdi

I have verified tier1 to tier8 and didn't find any issues.
Could you please sponsor this push for me.

Thanks,

@fmatte1
Copy link
Author

@fmatte1 fmatte1 commented May 3, 2021

/integrate

@openjdk openjdk bot added the sponsor label May 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 3, 2021

@fmatte1
Your change (at version 37170d2) is now ready to be sponsored by a Committer.

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Hi Fairoz,
It looks good to me.
It is a good change to get rid of the findComponentType.
Thanks,
Serguei

@fmatte1
Copy link
Author

@fmatte1 fmatte1 commented May 5, 2021

Hi Serguei, thanks for the review.

@sspitsyn
Copy link
Contributor

@sspitsyn sspitsyn commented May 5, 2021

/sponsor

@openjdk openjdk bot closed this May 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 5, 2021

@sspitsyn @fmatte1 Since your change was applied there have been 208 commits pushed to the master branch:

  • b71f85a: 8264398: BevelBorderUIResource​(int, Color, Color) and BevelBoder(int, Color, Color) spec should clarify about usage of highlight and shadow color
  • b172555: 8266171: -Warray-bounds happens in imageioJPEG.c
  • 8bcebe7: 8265505: findsym does not work on remote debug server
  • b88785d: 8266038: Move newAddress() to JVMDebugger
  • 2c53654: 8266179: [macos] jpackage should specify architecture for produced pkg files
  • d282799: 8255566: Add size validation when parsing values from VersionProps
  • 61365d5: 8266465: Add wildcard to JTwork/JTreport exclude in jib-profiles.js
  • f00b70e: 8266527: RandomTestCoverage.java failing due to API removal
  • c53dee7: 8266227: Fix help text for --mac-signing-keychain
  • 80323b7: 8261169: Upgrade HarfBuzz to the latest 2.8.0
  • ... and 198 more: https://git.openjdk.java.net/jdk/compare/95f0fd6c4dfaea9c3e065485dd201decf2be98ba...master

Your commit was automatically rebased without conflicts.

Pushed as commit 82768d9.

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

@openjdk openjdk bot removed the rfr label May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants