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

Replaced getUnsafeInstance.defineClass by MethodHandles.privateLookupIn #19

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

judovana
Copy link

getUnsafeInstance().defineClass was removed in jdk11 in favour of MethodHandles.privateLookupIn
MethodHandles.privateLookupIn are nto available in jdk8

getUnsafeInstance().defineClass was removed in jdk11 in favour of MethodHandles.privateLookupIn
MethodHandles.privateLookupIn are nto available in jdk8
@judovana
Copy link
Author

This patch + #20 is making procyon jdk11 friendly.
The patch is used in feodra for a year now, and all seams to be working fine

@judovana
Copy link
Author

This patch is making it not buildable on jdk8.
If you wish to keep the jdk8 compatibility, I may move it to reflection or to some kind of if-def class. But want to discus it first.
This currently builds also on jdk16. If moved to reflection maybe the need to --add-opens or similarly may be hit.

_protectionDomain
);
MethodHandles.Lookup lookup = MethodHandles.lookup();
MethodHandles.Lookup privateLookup = MethodHandles.privateLookupIn(Class.forName(fullName), lookup);
Copy link
Owner

Choose a reason for hiding this comment

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

Did you test this? I don't see how Class.forName(fullName) could work, since the class doesn't exist until the next line.

Copy link
Author

Choose a reason for hiding this comment

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

hi!

it is definitely tested in usual workloads. Is there some cornercase you think is sure to hit those lines?

Copy link
Contributor

@nbauma109 nbauma109 Feb 2, 2022

Choose a reason for hiding this comment

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

@mstrobel : You have unit tests that cover that part of the code
@judovana: I'm confused how you can test this without changing JDK version from 1.7 to 11 in the build.gradle? I've run the build and I have 31 test failures (that can be reduced to 15 with the PR #35), failures such as variable renames that appear trivial at first but can cause collisions and loss of meaningful naming (x instead of b for boolean, etc...) and compiler methods such as makeConcatWithConstants not interpreted anymore (@mstrobel: was that supported?)

@judovana
Copy link
Author

judovana commented Aug 9, 2021

ping please?

@delafer
Copy link

delafer commented Feb 2, 2022

ping please?

I am sure, you haven't tested your code. It can't work -> Class.forName(fullName) will always throw "java.lang.ClassNotFoundException" on non-existing classes. You haven't even changed JDK Java version in your build and MethodHandles.privateLookupIn is also awailable only since java 9, it also can't work with java versions prior to 9.
I don't understand why you posted such complete meaningless code....
pong!

@judovana
Copy link
Author

judovana commented Feb 3, 2022

@delafer Hello, please thread whole issue, and timestamps on comments:

"it is definitely tested in usual workloads" : I'm using procyon with this patch on daily base, and had not hit the issue: https://src.fedoraproject.org/rpms/procyon/blob/rawhide/f/lookupPatch.patch . However I must admit, my usecase is not exactly straightforward, and am using procyon only on fields where other decompilers fails. As Procyon is one of the worse in ordinary code load, but can handle corner cases best of all.
"you haven't even changed JDK Java version in your build" no, because Gradle sucks, and I no longer use gradle to build it. I build it directly by javac https://src.fedoraproject.org/rpms/procyon/blob/rawhide/f/procyon.spec#_159; and same setup for local ide and usage.
"Is there some cornercase you think is sure to hit those lines?" is a question which was about to pin point what next. A complain about untitest trigering the issue would be most valuable, and I would immediately run the unittests and look into the issue.
"MethodHandles.privateLookupIn is also awailable only since java 9, it also can't work with java versions prior to 9." Of course they are. And I wrote it header of the PR and in first comment.
"complete meaningless code": sorry, it is not. It moved the codebase to build fine with jdk11, and it left procyon working. I tried that those liens are executed and are solving the issue. When I raised the question there was not exactly interest in it it. ow there were motre patches on this topic so it can move.

I never claimed the patch flawless. but got only minimal feedback until @nbauma109 just day ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants