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

JDK-8247795 allow factory methods for inline types to return j.l.Obje… #94

Closed
wants to merge 1 commit into from

Conversation

hseigel
Copy link
Member

@hseigel hseigel commented Jun 19, 2020

Expect the return type for the factory methods of hidden inline classes to return type java.lang.Object. Also, fix issue so that Reflection::new_constructor(), not new_method(), is called for factory methods.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8247795: [lworld] Fix VM support of factory methods for inline hidden and UAC classes ⚠️ Title mismatch between PR and JBS.

Reviewers

  • Frederic Parain (fparain - Committer)

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/94/head:pull/94
$ git checkout pull/94

…ct and fix issue with calls to Reflection::new_constructor()
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 19, 2020

👋 Welcome back hseigel! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request.

@openjdk
Copy link

openjdk bot commented Jun 19, 2020

@hseigel This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

8247795: allow factory methods for inline types to return j.l.Obje…

Reviewed-by: fparain
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

There are currently no new commits on the lworld branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate 9cdd26f6c38bbcd4414e0a93b71450ab2a01afde.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

@mlbridge
Copy link

mlbridge bot commented Jun 19, 2020

Webrevs

@mlchung
Copy link
Member

mlchung commented Jun 19, 2020

I consider this fix is a workaround to allow prototyping inline hidden class (for example to convert lambda proxy class to inline class). We need to sort out JVM spec issues - what return types should be allowed? JDK-8224057 is a companion issue.

As a workaround, I'd suggest to allow the static factory method to either allow its direct or indirect supertype as its return type (that will include java.lang.Object) or drop the return type check for inline hidden class. It'd allow to try producing a static factory method with an assignable type besides Object and cast an instance of an inline hidden class to such type.

Copy link
Collaborator

@fparain fparain left a comment

Looks good for now (it will probably be revisited after the spec update).

Fred

@hseigel
Copy link
Member Author

hseigel commented Jun 23, 2020

/integrate

@openjdk openjdk bot closed this Jun 23, 2020
@openjdk openjdk bot added integrated and removed ready labels Jun 23, 2020
@openjdk
Copy link

openjdk bot commented Jun 23, 2020

@hseigel
Pushed as commit e5eb253.

@openjdk openjdk bot removed the rfr label Jun 23, 2020
@mlchung
Copy link
Member

mlchung commented Jun 23, 2020

@hseigel - have you seen my feedback? You integrated it as is?

if (is_unsafe_anonymous()) {
// The original class name in the UAC byte stream gets changed. So
// using the original name in the return type is no longer valid.
if (is_hidden() || is_unsafe_anonymous()) {
Copy link
Member

@mlchung mlchung Jun 23, 2020

Choose a reason for hiding this comment

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

JDK no longer uses VM anonymous class. This check (when parsing the static factory method of an inline type) should be updated to if (is_hidden()) and drop is_unsafe_anonymous().

Copy link
Member Author

@hseigel hseigel Jun 23, 2020

Choose a reason for hiding this comment

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

VM anonymous classes need to be supported until they are obsoleted. Removing this call to is_unsafe_anonymous() can easily be done once that happens.

Copy link
Member

@mlchung mlchung Jun 23, 2020

Choose a reason for hiding this comment

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

I do not agree that we need to make VM anonymous classes to support inline types. This block is for inline types only. Please remove it.

@@ -1222,7 +1222,13 @@ oop Reflection::invoke_constructor(oop constructor_mirror, objArrayHandle args,
if (!method->signature()->is_void_method_signature()) {
assert(klass->is_inline_klass(), "inline classes must use factory methods");
Handle no_receiver; // null instead of receiver
return invoke(klass, method, no_receiver, override, ptypes, T_VALUETYPE, args, false, CHECK_NULL);
BasicType rtype;
if (klass->is_hidden() || klass->is_unsafe_anonymous()) {
Copy link
Member

@mlchung mlchung Jun 23, 2020

Choose a reason for hiding this comment

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

Drop is_unsafe_anonymous check. We should not support VM anonymous class be an inline type.

I suggest to add a comment to explain this workarounds the JVM spec issue for hidden classes.

Copy link
Member Author

@hseigel hseigel Jun 23, 2020

Choose a reason for hiding this comment

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

See above comment about is_unsafe_anonymous(). I can add a comment about the JVM Spec issue in a follow-on fix.

Copy link
Member

@mlchung mlchung Jun 23, 2020

Choose a reason for hiding this comment

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

Please do not make VM anonymous classes to support inline type.

String hello = "hello";

static byte[] readClassFile(String classFileName) throws Exception {
File classFile = new File(CLASSES_DIR + File.separator + classFileName);
Copy link
Member

@mlchung mlchung Jun 23, 2020

Choose a reason for hiding this comment

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

A simpler implementation is to use NIO:

    static final Path CLASSES_DIR = Paths.get(System.getProperty("test.classes", "."));

    static byte[] readClassFile(String cn) throws IOException {
        Path path = CLASSES_DIR.resolve(cn.replace('.', File.separatorChar) + ".class");
        return Files.readAllBytes(path);
    }

Copy link
Member Author

@hseigel hseigel Jun 23, 2020

Choose a reason for hiding this comment

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

Multiple tests could benefit by using the NIO change that you suggest. I'll enter an RFE for that.


// Define a hidden class that is an inline type.
Class<?> c = lookup.defineHiddenClass(bytes, true, NESTMATE).lookupClass();
Object hp = c.newInstance();
Copy link
Member

@mlchung mlchung Jun 23, 2020

Choose a reason for hiding this comment

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

Nit: No need to be a nestmate.

Copy link
Member Author

@hseigel hseigel Jun 23, 2020

Choose a reason for hiding this comment

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

Is it a problem that it's a nestmate?

Copy link
Member

@mlchung mlchung Jun 23, 2020

Choose a reason for hiding this comment

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

This is a copy-n-paste issue. I expect a test knows what it does and it should use it with a clear intention. I prefer to take it out or add a comment explaining why NESTMATE is used for this hidden inline test.

@mlchung
Copy link
Member

mlchung commented Jun 23, 2020

@hseigel - have you seen my feedback? You integrated it as is?

Sigh... I just realized I didn't click "submit review". You can follow up my comments in a separate patch. Sorry for not posting my review comments properly.

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