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

[Backport] Delete native methods from java.lang.Thread #35

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

Karm
Copy link
Contributor

@Karm Karm commented Dec 19, 2024

'getStackTrace0' is only used in asyncGetStackTrace() which is deleted as well.
Thread.getStackTrace() is replaced by PlatformThreads.getStackTrace(). So it's not
reachable.

'getNextThreadIdOffset()' is only used in ThreadIdentifiers private class which is
substituted in JDKs better than JDK 19 (which includes 21). So won't be reachable either.

Fixes: #28

This is a partial backport of:
"Introudce a mode to unconditionally include classes" oracle/graal@369f0ff8
to address oracle/graal#9672

Only take the java.lang.Thread native method deletions as that's sufficient to fix
the issue reported in #28.

@Karm Karm added the backport Backport or backport request label Dec 19, 2024
@Karm Karm added this to the 23.1.6 milestone Dec 19, 2024
@Karm Karm requested review from jerboaa and zakkak December 19, 2024 08:26
@Karm Karm self-assigned this Dec 19, 2024
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Dec 19, 2024
@Karm Karm changed the title [Backport] commit/369f0ff8, native method deletions [Backport] commit/369f0ff, native method deletions Dec 19, 2024
Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

LGTM. The MethodHandles and MethodHandleNatives native methods deletions seem redundant but safe at the same time.

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

I'd prefer to only have the changes to java.lang.Thread, unless we have evidence that the other change is needed as well.

@jerboaa
Copy link
Contributor

jerboaa commented Dec 19, 2024

@Karm Please change the title of the PR to something clearer. Without looking up the commit id somewhere else it doesn't say what it's doing. I suggest this commit message (first line as PR title):

Delete native methods from java.lang.Thread

'getStackTrace0' is only used in asyncGetStackTrace() which is deleted as well.
Thread.getStackTrace()  is replaced by PlatformThreads.getStackTrace(). So it's not
reachable.

'getNextThreadIdOffset()' is only used in ThreadIdentifiers private class which is
substituted in JDKs better than JDK 19 (which includes 21). So won't be reachable either.

Fixes: #28

This is a partial backport of:
"Introudce a mode to unconditionally include classes" https://github.com/oracle/graal/commit/369f0ff8
to address https://github.com/oracle/graal/issues/9672

Only take the java.lang.Thread native method deletions as that's sufficient to fix
the issue reported in #28.

Thanks!

@Karm
Copy link
Contributor Author

Karm commented Dec 19, 2024

I'd prefer to only have the changes to java.lang.Thread, unless we have evidence that the other change is needed as well.

It is in no way related as far as my study of the original commit goes. The commit involves like 4 unrelated topics and cleanups.

The issue we witnessed was solely about people trying to use reflection to construct a virtual thread when a runtime offering that was detected.

I will remove all other unrelated parts then...

'getStackTrace0' is only used in asyncGetStackTrace() which is deleted as well.
Thread.getStackTrace()  is replaced by PlatformThreads.getStackTrace(). So it's not
reachable.

'getNextThreadIdOffset()' is only used in ThreadIdentifiers private class which is
substituted in JDKs better than JDK 19 (which includes 21). So won't be reachable either.

Fixes: graalvm#28

This is a partial backport of:
"Introudce a mode to unconditionally include classes" oracle/graal@369f0ff8
to address oracle/graal#9672

Only take the java.lang.Thread native method deletions as that's sufficient to fix
the issue reported in graalvm#28.
@Karm Karm changed the title [Backport] commit/369f0ff, native method deletions [Backport] Delete native methods from java.lang.Thread Dec 19, 2024
@Karm Karm requested a review from jerboaa December 19, 2024 14:10
Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM!

@Karm Karm merged commit 28028ec into graalvm:master Dec 20, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Backport or backport request OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Backport] Fails to register java.lang.Thread methods for reflection
3 participants